From 8bf5138582c6c3772fcece8e9defc7ae2cab7179 Mon Sep 17 00:00:00 2001 From: clawbot Date: Fri, 4 Sep 2026 10:05:38 +0000 Subject: [PATCH] WIP: follow Makefile prerequisites and $(MAKE) in lint-once --- test/packaging/lint-once.test.ts | 221 ++++++++++++++++++++++++++++--- 1 file changed, 199 insertions(+), 22 deletions(-) diff --git a/test/packaging/lint-once.test.ts b/test/packaging/lint-once.test.ts index 16fbad1..e90c67b 100644 --- a/test/packaging/lint-once.test.ts +++ b/test/packaging/lint-once.test.ts @@ -16,10 +16,21 @@ // against one file. Starting from an entrypoint, it follows every edge the repo // actually uses to reach another command — `run:` steps in the CI workflow, // `"$SCRIPT_DIR/"` and `script/` into other scripts, `make ` -// through the Makefile shims, `yarn run ` through the `package.json` -// scripts, and `docker build -f ` into that Dockerfile's `RUN` steps — and -// counts the prettier invocations it finds. A prettier call added anywhere in -// that graph is therefore caught, wherever it is added. +// and `$(MAKE) ` through the Makefile shims, the prerequisites of a +// Makefile target, `yarn run ` through the `package.json` scripts, and +// `docker build -f ` into that Dockerfile's `RUN` steps, heredoc bodies +// included — and counts the prettier invocations it finds. +// +// What that covers is worth stating exactly rather than as "anywhere", because +// a comment claiming more than the code delivers is the same defect as an +// assertion that cannot fail — and an earlier draft of this header did claim +// it, while `check: fmt-check` and `@$(MAKE) fmt-check` both walked straight +// past it. Command names are read as `[a-z][a-z0-9_-]*` for Makefile targets +// and `script/` files, and `[A-Za-z][A-Za-z0-9_:-]*` for `package.json` +// scripts. A command reached by some other means — `sh -c`, a shell alias, an +// `include`d makefile, a name outside those charsets, a generated file — is +// not followed. Within those edges, a prettier call is caught wherever it is +// added. // // Two entrypoints are walked, because they cover different graphs: `make check` // is what a developer runs, and `.gitea/workflows/check.yml` is what CI runs. @@ -83,20 +94,56 @@ const countPrettier = (line: string): number => // Makefile targets are thin shims (`check:` / tab / `@script/check`), so a // `make ` edge has to resolve through them to keep "per `make check`" // meaning what it says. Recipe lines are the tab-indented ones. -const makeRecipes = (): Map => { - const recipes = new Map(); +// +// The prerequisite list is read as well, and it is not decoration: make runs a +// target's prerequisites before its recipe, so `check: fmt-check` invokes +// fmt-check every bit as much as a `@script/fmt-check` line in the recipe +// would. An earlier version of this parser captured the target name and threw +// the rest of the line away, which made `check: fmt-check` — a one-token edit, +// and the most natural way for someone to wire the host formatting check back +// into `make check` — a second prettier pass that this file scored as one. The +// shipped Makefile already relies on prerequisites (`install: build-bin`), so +// this is the file's own house style rather than a contrived evasion. +interface MakeTarget { + prerequisites: string[]; + recipe: string[]; +} + +// Names are lowercase-with-dashes here, plus digits and underscores. The +// leading character is deliberately not uppercase: `YARN := yarn run` is a +// variable assignment, not a rule, and `(?!=)` alone would not say so on a +// line spelled `YARN: yarn run`. +const MAKE_TARGET_NAME = "[a-z][a-z0-9_-]*"; + +const makeRecipes = (): Map => { + const recipes = new Map(); let current: string | null = null; for (const raw of read("Makefile").split("\n")) { if (raw.startsWith("\t")) { if (current !== null) { - recipes.get(current)?.push(raw.trim().replace(/^[@-]+/, "")); + recipes + .get(current) + ?.recipe.push(raw.trim().replace(/^[@-]+/, "")); } continue; } - const target = /^([a-z][a-z-]*)\s*:(?!=)/.exec(raw); - current = target === null ? null : target[1]; - if (current !== null && !recipes.has(current)) { - recipes.set(current, []); + const target = new RegExp(`^(${MAKE_TARGET_NAME})\\s*:(?!=)(.*)$`).exec( + raw, + ); + current = target === null ? null : (target[1] ?? null); + if (target === null || current === null) continue; + // Trailing `# comment` is not a prerequisite; neither is the empty + // string a split leaves behind. + const prerequisites = (target[2] ?? "") + .replace(/#.*$/, "") + .trim() + .split(/\s+/) + .filter((name) => name !== ""); + const existing = recipes.get(current); + if (existing === undefined) { + recipes.set(current, { prerequisites, recipe: [] }); + } else { + existing.prerequisites.push(...prerequisites); } } return recipes; @@ -104,6 +151,18 @@ const makeRecipes = (): Map => { const recipes = makeRecipes(); +// Only prerequisites that name a target of this Makefile are followed: the +// rest are filenames, or expansions of variables this parser does not +// evaluate, and neither is an invocation of anything it could read. +const prerequisitesOf = (node: string): string[] => { + if (!node.startsWith("make:")) return []; + const target = recipes.get(node.slice("make:".length)); + if (target === undefined) return []; + return target.prerequisites + .filter((name) => recipes.has(name)) + .map((name) => `make:${name}`); +}; + const packageScripts = (): Record => { const pkg = JSON.parse(read("package.json")) as { scripts?: Record; @@ -113,14 +172,40 @@ const packageScripts = (): Record => { const scripts = packageScripts(); +// A `RUN < { + const commands: string[] = []; + let terminator: string | null = null; + for (const line of executable(text)) { + if (terminator !== null) { + if (line === terminator) terminator = null; + else commands.push(line); + continue; + } + if (!line.startsWith("RUN ")) continue; + const command = line.slice("RUN ".length); + terminator = HEREDOC_OPEN.exec(command)?.[2] ?? null; + commands.push(command); + } + if (terminator !== null) { + throw new Error(`unterminated heredoc: ${terminator}`); + } + return commands; +}; + // Node keys: `script/`, `docker:`, `make:`, // `yarn:`, `workflow:`. const resolve = (node: string): string[] => { if (node.startsWith("script/")) return executable(read(node)); if (node.startsWith("docker:")) { - return executable(read(node.slice("docker:".length))) - .filter((line) => line.startsWith("RUN ")) - .map((line) => line.slice("RUN ".length)); + return dockerRunCommands(read(node.slice("docker:".length))); } // The `run:` steps of a workflow, in file order. `uses:` steps are actions, // not commands, and have no edges into this repo's graph. A `run: |` block @@ -140,7 +225,7 @@ const resolve = (node: string): string[] => { if (recipe === undefined) { throw new Error(`no such Makefile target: ${target}`); } - return recipe; + return recipe.recipe; } if (node.startsWith("yarn:")) { const name = node.slice("yarn:".length); @@ -156,9 +241,13 @@ const resolve = (node: string): string[] => { // Same reasoning as the missing-target error, applied to every node kind: a // node that resolves to no commands contributes zero prettier invocations and // zero edges, which is indistinguishable from a clean result. Fail instead. +// +// A Makefile target with prerequisites and an empty recipe is the one case +// that is genuinely not vacuous — it runs its prerequisites — so it is not +// caught here. const commandsOf = (node: string): string[] => { const commands = resolve(node); - if (commands.length === 0) { + if (commands.length === 0 && prerequisitesOf(node).length === 0) { throw new Error(`node resolved to no commands: ${node}`); } return commands; @@ -170,21 +259,48 @@ const edgesOf = (line: string): string[] => { // `"$SCRIPT_DIR/lint"`, `"$ROOT/script/lint"` and a bare `script/lint` are // all the same edge. for (const match of line.matchAll( - /(?:\$SCRIPT_DIR|\$\{SCRIPT_DIR\}|script)\/([a-z][a-z-]*)/g, + new RegExp( + `(?:\\$SCRIPT_DIR|\\$\\{SCRIPT_DIR\\}|script)/(${MAKE_TARGET_NAME})`, + "g", + ), )) { edges.push(`script/${match[1]}`); } // Only real targets: `pkg_install gnumake make make make` in // script/bootstrap is a package name, not an invocation of this Makefile. - for (const match of line.matchAll(/\bmake\s+([a-z][a-z-]*)/g)) { + // + // `$(MAKE)` and `${MAKE}` count as `make`. Recursive make is spelled that + // way by convention rather than as a literal `make`, and this Makefile + // already expands variables into recipes (`@$(YARN) tsc --watch`), so a + // case-sensitive literal-only match left `@$(MAKE) fmt-check` — the other + // one-token way to put the host prettier pass back into `make check` — + // unfollowed. Valueless flags between the command and the target (`make + // -n check`, `make -j4 check`) are skipped. A flag that takes a separate + // argument is not: `make -C sub build` runs `sub/Makefile`'s target, not + // this one's, and resolving it against these recipes would be wrong + // rather than merely incomplete. + for (const match of line.matchAll( + new RegExp( + `(?:\\$\\(MAKE\\)|\\$\\{MAKE\\}|\\bmake)(?:\\s+-\\S+)*\\s+(${MAKE_TARGET_NAME})`, + "g", + ), + )) { if (recipes.has(match[1] ?? "")) edges.push(`make:${match[1]}`); } // Same rule for yarn: `yarn run prettier` is the linter itself (counted, // not followed), `yarn run fmt-check` would be a package.json script that // runs it indirectly. - for (const match of line.matchAll(/\byarn(?:\s+run)?\s+([a-z][a-z-]*)/g)) { + // + // The name charset is wider than the Makefile's because `package.json` + // script names conventionally carry colons, digits and underscores + // (`lint:fmt`, `test:e2e`). Under the narrower charset `yarn run lint:fmt` + // read as `yarn run lint`, which is not a script, so it produced no edge + // and no count. + for (const match of line.matchAll( + /\byarn(?:\s+run)?\s+([A-Za-z][A-Za-z0-9_:-]*)/g, + )) { if ((match[1] ?? "") in scripts) edges.push(`yarn:${match[1]}`); } @@ -217,6 +333,11 @@ const walk = (node: string, path: string[] = [], into?: Walk): Walk => { } result.reached.add(node); + // Before the recipe, exactly as make does. + for (const edge of prerequisitesOf(node)) { + walk(edge, [...path, node], result); + } + for (const line of commandsOf(node)) { result.prettier += countPrettier(line); for (const edge of edgesOf(line)) { @@ -333,9 +454,13 @@ const installBranches = (): { withoutYarn: string[]; withYarn: string[] } => { } const close = lines.indexOf("}", open); const body = lines.slice(open + 1, close === -1 ? undefined : close); - const guard = body.findIndex((line) => - /^if\b.*\bmissing yarn\b/.test(line), - ); + // The guard has to be a plain positive `missing yarn` test at the front of + // the condition. `if ! missing yarn ...` is the same shape to a looser + // regex but swaps which branch is which, and since the two branches are + // asserted separately below, that would silently relabel them — a failing + // test would then name the wrong branch. Anything else throws the named + // error below, which is the loud failure this file prefers. + const guard = body.findIndex((line) => /^if\s+missing yarn\b/.test(line)); const otherwise = body.indexOf("else", guard); const end = body.indexOf("fi", otherwise); if (guard === -1 || otherwise === -1 || end === -1) { @@ -488,6 +613,52 @@ describe("the resolver reads what the shell would run", () => { ).toEqual(["script/lint", "script/test", "script/fmt"]); }); + // The two edges the previous version of this file could not see, each + // pinned directly as well as through the graph. Both were reproduced as + // Makefile mutations that gave `make check` two prettier passes with the + // whole suite green. + it("follows a Makefile prerequisite as an invocation", () => { + // Not hypothetical: this is the shipped Makefile's own `install`. + expect(prerequisitesOf("make:install")).toEqual(["make:build-bin"]); + expect(prerequisitesOf("make:check")).toEqual([]); + }); + + it("treats $(MAKE) and ${MAKE} as make", () => { + expect(edgesOf("$(MAKE) fmt-check")).toContain("make:fmt-check"); + expect(edgesOf("${MAKE} fmt-check")).toContain("make:fmt-check"); + expect(edgesOf("make fmt-check")).toContain("make:fmt-check"); + expect(edgesOf("make -n fmt-check")).toContain("make:fmt-check"); + }); + + it("reads a package.json script name that contains a colon", () => { + // The lookup against `scripts` is what turns this into an edge, and no + // script in this repo is colon-named, so the charset is asserted on + // the extraction itself. + expect( + [ + ..."yarn run lint:fmt".matchAll( + /\byarn(?:\s+run)?\s+([A-Za-z][A-Za-z0-9_:-]*)/g, + ), + ].map((match) => match[1]), + ).toEqual(["lint:fmt"]); + }); + + it("reads the body of a BuildKit heredoc RUN step", () => { + expect( + dockerRunCommands( + ["FROM scratch", "RUN < { + expect(() => + dockerRunCommands("FROM scratch\nRUN < { expect(edgesOf("docker build .")).toContain("docker:Dockerfile"); expect(edgesOf("docker build -f Dockerfile.lint .")).toContain( @@ -495,6 +666,12 @@ describe("the resolver reads what the shell would run", () => { ); }); + // This exact-equality assertion is also the block-scalar guard, which is + // not obvious from its name: a `run: |` step resolves to the bare `|`, + // which reaches nothing, so the prettier count would stay 1 no matter what + // the block contains. Pinning the resolved list is what makes such a step + // red. The cost is that any legitimate second `run:` step — a cache step, + // an `echo` — fails this test for a reason unrelated to prettier. it("reads the run steps of the CI workflow and not its uses steps", () => { expect(commandsOf("workflow:.gitea/workflows/check.yml")).toEqual([ "script/cibuild",