From 2d83f4a16f3edabfd1f7a5a06baf5b020952eaf9 Mon Sep 17 00:00:00 2001 From: Christopher Monsanto Date: Sat, 15 Aug 2026 22:32:59 -0400 Subject: [PATCH] Fix review findings in tools/build - Verify rename sources against recorded stats at copy time; colliding rename destinations/sources fall back to running the rule - Restrict rename detection to single-input, no-deps rules (sheet-style rules embed input names in output bytes) - Include input extensions in the rename template (magick picks output format from extensions) - Adopt only rules with no stored record, never known-dirty ones - Skip dotfiles in glob (tup parity) - Complete sheet dep closures (data/lib, lib/root) - Validate --jobs; reject non-numeric values instead of silently no-oping - Worker pool drains all workers before rethrowing; per-rule internal errors abort scheduling instead of racing db.close() - SIGKILL all live process groups on second Ctrl-C - Transactional loadStoredRules snapshot; dry run without existing state uses an in-memory db; idempotent DDL - rule() rejects % placeholders in literal outputs Co-Authored-By: Claude Fable 5 --- Buildfile.ts | 12 +++- tools/build/api.ts | 23 ++++-- tools/build/db.ts | 20 ++++-- tools/build/exec.ts | 27 ++++++- tools/build/index.ts | 116 ++++++++++++++++++++----------- tools/build/plan.ts | 25 ++++--- tools/build/test/exec.test.ts | 29 ++++++++ tools/build/test/plan.test.ts | 67 ++++++++++++++++++ tools/build/test/sprites.test.ts | 2 +- 9 files changed, 255 insertions(+), 66 deletions(-) create mode 100644 tools/build/test/exec.test.ts diff --git a/Buildfile.ts b/Buildfile.ts index b38263cd..ff48aec3 100644 --- a/Buildfile.ts +++ b/Buildfile.ts @@ -66,6 +66,8 @@ rule("ps-pokemon.sheet.mjs", { "src/minisprites/pokemon/gen6/*", "data/species.json", "data/items.json", + "data/lib/index.ts", + "lib/root/index.ts", "tools/sheet/index.ts", ], cmds: ["node tools/sheet/index.ts %f %o", compresspng({config: "SPRITESHEET"})], @@ -83,6 +85,8 @@ rule("ps-items.sheet.mjs", { "src/minisprites/items/*", "data/species.json", "data/items.json", + "data/lib/index.ts", + "lib/root/index.ts", "tools/sheet/index.ts", ], cmds: ["node tools/sheet/index.ts %f %o", compresspng({config: "SPRITESHEET"})], @@ -115,7 +119,13 @@ forEachRule(spriteglob(["src/minisprites/pokemon/gen6/*", "src/minisprites/items rule(spriteglob(["src/minisprites/pokemon/gen6/*", "src/minisprites/items/*"], {a: false}), { display: "smogdex sheet", - deps: ["data/species.json", "data/items.json", "tools/smogdexspritesheet/index.ts"], + deps: [ + "data/species.json", + "data/items.json", + "data/lib/index.ts", + "lib/root/index.ts", + "tools/smogdexspritesheet/index.ts", + ], // build/smogon/spritesheet.png is an undeclared temporary; it is removed // before the rule finishes. cmds: [ diff --git a/tools/build/api.ts b/tools/build/api.ts index c9d6b013..61a143ea 100644 --- a/tools/build/api.ts +++ b/tools/build/api.ts @@ -73,6 +73,9 @@ function globOne(pat : string) : string[] { continue; } const name = ent.name; + if (name.startsWith('.')) { + continue; // tup.glob ignored dotfiles + } if (name.length >= prefix.length + suffix.length && name.startsWith(prefix) && name.endsWith(suffix)) { results.push(dir === '.' ? name : `${dir}/${name}`); @@ -172,10 +175,15 @@ function normalizeSpec(spec : CmdSpec | Cmd[]) : CmdSpec { // The rename-detection template deliberately excludes output directories // (basename only) so that content-preserving moves across directories with -// identical processing still match, but extension changes (which change the -// bytes tools like magick produce) do not. -function templateOf(cmds : string[], outputTemplates : string[]) : string { - return cmds.join('\n') + '\0' + outputTemplates.map(t => pathlib.basename(t)).join('\0'); +// identical processing still match. Input extensions are included because +// tools like magick pick their output format from file extensions, so an +// extension-only rename must not match. +function templateOf(cmds : string[], outputTemplates : string[], inputs : string[]) : string { + return [ + cmds.join('\n'), + outputTemplates.map(t => pathlib.basename(t)).join('\0'), + inputs.map(p => pathlib.extname(p)).join('\0'), + ].join('\x01'); } function keyOf(command : string, inputs : string[], deps : string[], outputs : string[]) : string { @@ -197,7 +205,7 @@ function makeRule(inputs : string[], deps : string[], spec : CmdSpec, outputs, command, display: spec.display !== undefined ? substitute(spec.display, inputs, outputs) : null, - template: templateOf(flattenCmds(spec.cmds), outputTemplates), + template: templateOf(flattenCmds(spec.cmds), outputTemplates, inputs), key: keyOf(command, inputs, deps, outputs), }; rules.push(decl); @@ -208,6 +216,11 @@ export function rule(input : string | string[], spec : CmdSpec | Cmd[], output : string | string[]) : string[] { const s = normalizeSpec(spec); const outputs = astable(output); + for (const out of outputs) { + if (out.includes('%')) { + throw new Error(`rule() outputs are literal paths, no substitutions: ${out}`); + } + } const decl = makeRule(glob(input), glob(astable(s.deps)), s, outputs, outputs); return decl.outputs; } diff --git a/tools/build/db.ts b/tools/build/db.ts index a175ab68..4a93f95d 100644 --- a/tools/build/db.ts +++ b/tools/build/db.ts @@ -38,14 +38,14 @@ export interface RecordedOutput { } const DDL = ` -CREATE TABLE file_cache ( +CREATE TABLE IF NOT EXISTS file_cache ( path TEXT PRIMARY KEY, size INTEGER NOT NULL, mtime_ns INTEGER NOT NULL, hash BLOB NOT NULL ) WITHOUT ROWID; -CREATE TABLE rules ( +CREATE TABLE IF NOT EXISTS rules ( id INTEGER PRIMARY KEY, key TEXT NOT NULL UNIQUE, command TEXT NOT NULL, @@ -54,9 +54,9 @@ CREATE TABLE rules ( input_sig BLOB NOT NULL, ok INTEGER NOT NULL DEFAULT 0 ); -CREATE INDEX rules_rename ON rules(template, input_sig); +CREATE INDEX IF NOT EXISTS rules_rename ON rules(template, input_sig); -CREATE TABLE rule_inputs ( +CREATE TABLE IF NOT EXISTS rule_inputs ( rule_id INTEGER NOT NULL REFERENCES rules(id) ON DELETE CASCADE, ord INTEGER NOT NULL, path TEXT NOT NULL, @@ -64,9 +64,9 @@ CREATE TABLE rule_inputs ( hash BLOB NOT NULL, PRIMARY KEY (rule_id, ord) ); -CREATE INDEX rule_inputs_path ON rule_inputs(path); +CREATE INDEX IF NOT EXISTS rule_inputs_path ON rule_inputs(path); -CREATE TABLE rule_outputs ( +CREATE TABLE IF NOT EXISTS rule_outputs ( rule_id INTEGER NOT NULL REFERENCES rules(id) ON DELETE CASCADE, ord INTEGER NOT NULL, path TEXT NOT NULL, @@ -74,7 +74,7 @@ CREATE TABLE rule_outputs ( mtime_ns INTEGER, PRIMARY KEY (rule_id, ord) ); -CREATE INDEX rule_outputs_path ON rule_outputs(path); +CREATE INDEX IF NOT EXISTS rule_outputs_path ON rule_outputs(path); `; export class BuildDb { @@ -135,6 +135,12 @@ export class BuildDb { } loadStoredRules() : StoredRule[] { + // One transaction: the three queries must see a single snapshot, or a + // concurrent writer (e.g. a build racing a dry run) tears the view. + return this.db.transaction(() => this.loadStoredRulesInner())(); + } + + private loadStoredRulesInner() : StoredRule[] { const byId = new Map(); const ruleRows = this.db.prepare<[], { id : bigint, key : string, command : string, display : string | null, diff --git a/tools/build/exec.ts b/tools/build/exec.ts index 578831e8..f2cf4266 100644 --- a/tools/build/exec.ts +++ b/tools/build/exec.ts @@ -8,6 +8,17 @@ export interface ExecResult { durationMs : number; } +const livePids = new Set(); + +// Emergency stop (e.g. second Ctrl-C): SIGKILL every live process group. +export function killAllProcessGroups() : void { + for (const pid of livePids) { + try { + process.kill(-pid, 'SIGKILL'); + } catch {} + } +} + // The command script is fed to sh via stdin rather than -c: a single argv // entry is capped by the kernel (MAX_ARG_STRLEN, ~128KB) and the largest %f // expansion is already 80KB+. @@ -17,6 +28,9 @@ export function runShell(command : string, opts : {cwd : string, signal : AbortS // detached: own process group, so an abort kills grandchildren // (magick, optipng, ...) with one signal const child = spawn('sh', [], {cwd: opts.cwd, detached: true, stdio: ['pipe', 'pipe', 'pipe']}); + if (child.pid !== undefined) { + livePids.add(child.pid); + } const chunks : Buffer[] = []; child.stdout.on('data', c => chunks.push(c)); child.stderr.on('data', c => chunks.push(c)); @@ -41,6 +55,9 @@ export function runShell(command : string, opts : {cwd : string, signal : AbortS } const cleanup = () => { + if (child.pid !== undefined) { + livePids.delete(child.pid); + } opts.signal.removeEventListener('abort', onAbort); if (killTimer !== undefined) { clearTimeout(killTimer); @@ -62,6 +79,9 @@ export function runShell(command : string, opts : {cwd : string, signal : AbortS }); } +// A worker that throws stops its own loop, but the pool always waits for +// every other worker to finish before rethrowing: failing fast here would +// return control (and e.g. close the database) while rules are still running. export async function workerPool(items : readonly T[], jobs : number, fn : (item : T, index : number) => Promise) : Promise { let next = 0; @@ -74,5 +94,10 @@ export async function workerPool(items : readonly T[], jobs : number, } })()); } - await Promise.all(workers); + const results = await Promise.allSettled(workers); + for (const result of results) { + if (result.status === 'rejected') { + throw result.reason; + } + } } diff --git a/tools/build/index.ts b/tools/build/index.ts index 60e4a547..b6ad28bb 100644 --- a/tools/build/index.ts +++ b/tools/build/index.ts @@ -9,7 +9,7 @@ import debugfn from 'debug'; import {getRules, type RuleDecl, setConfig} from './api.ts'; import {loadConfig} from './config.ts'; import {acquireLock, BuildDb, type RecordedOutput} from './db.ts'; -import {runShell, workerPool} from './exec.ts'; +import {killAllProcessGroups, runShell, workerPool} from './exec.ts'; import {BuildError, checkGraph} from './graph.ts'; import {reconcileHashes} from './hash.ts'; import {computePlan, type OutputStat, ruleInputSig} from './plan.ts'; @@ -47,9 +47,16 @@ function indent(text : string) : string { } async function main() : Promise { + const jobs = Number(opts.jobs); + if (!Number.isInteger(jobs) || jobs < 1) { + throw new BuildError(`Invalid --jobs value: ${opts.jobs}`); + } const dryRun = Boolean(opts.dryRun); const releaseLock = dryRun ? null : acquireLock('.build/lock.sqlite'); - const db = new BuildDb('.build/db.sqlite'); + // A dry run must not create state; without an existing db it reads from + // an empty in-memory one. + const dbPath = dryRun && !fs.existsSync('.build/db.sqlite') ? ':memory:' : '.build/db.sqlite'; + const db = new BuildDb(dbPath); try { // Phase 1: evaluate the rule set setConfig(loadConfig(opts.config)); @@ -137,7 +144,21 @@ async function main() : Promise { } // Phase 4: renames (before stale deletion: sources must still exist) + let renamed = 0; for (const {decl, from} of plan.renames) { + // An earlier copy in this loop may have overwritten this rename's + // source (rename destinations can collide with rename sources); + // re-verify every source against its recorded stat before copying + // so only verified bytes ever propagate. + const intact = from.outputs.every(o => { + const st = statPath(o.path); + return o.size !== null && st !== null + && st.size === o.size && st.mtimeNs === o.mtimeNs; + }); + if (!intact) { + plan.run.push({decl, reason: 'new'}); + continue; + } const recorded : RecordedOutput[] = []; for (let i = 0; i < decl.outputs.length; i++) { const src = from.outputs[i]!.path; @@ -153,6 +174,7 @@ async function main() : Promise { recorded.push({path: dst, size: st.size, mtimeNs: st.mtimeNs}); } db.recordRuleResult(decl, hashes, ruleInputSig(decl, hashes), recorded); + renamed++; } // Phase 5: delete outputs of removed rules, prune empty dirs @@ -203,6 +225,7 @@ async function main() : Promise { let interrupted = false; const onSignal = () => { if (interrupted) { + killAllProcessGroups(); process.exit(130); } interrupted = true; @@ -214,51 +237,60 @@ async function main() : Promise { const failures : RuleDecl[] = []; let done = 0; - await workerPool(runList, parseInt(opts.jobs, 10), async ({decl}) => { + await workerPool(runList, jobs, async ({decl}) => { if (ac.signal.aborted) { return; } - for (const out of decl.outputs) { - fs.mkdirSync(pathlib.dirname(out), {recursive: true}); - } - const result = await runShell(decl.command, {cwd: root, signal: ac.signal}); - if (ac.signal.aborted && result.code !== 0) { - return; // killed by the abort, not a real failure; stays dirty - } - let recorded : RecordedOutput[] | null = null; - const missingOutputs = []; - if (result.code === 0) { - recorded = []; - for (const path of decl.outputs) { - const st = statPath(path); - if (st === null) { - missingOutputs.push(path); - recorded = null; - break; + try { + for (const out of decl.outputs) { + fs.mkdirSync(pathlib.dirname(out), {recursive: true}); + } + const result = await runShell(decl.command, {cwd: root, signal: ac.signal}); + if (ac.signal.aborted && result.code !== 0) { + return; // killed by the abort, not a real failure; stays dirty + } + let recorded : RecordedOutput[] | null = null; + const missingOutputs = []; + if (result.code === 0) { + recorded = []; + for (const path of decl.outputs) { + const st = statPath(path); + if (st === null) { + missingOutputs.push(path); + recorded = null; + break; + } + recorded.push({path, size: st.size, mtimeNs: st.mtimeNs}); } - recorded.push({path, size: st.size, mtimeNs: st.mtimeNs}); } - } - db.recordRuleResult(decl, hashes, ruleInputSig(decl, hashes), recorded); - done++; - if (recorded !== null) { - console.log(`[${done}/${runList.length}] ${label(decl)}`); - if (result.output !== '') { - console.log(indent(result.output)); + db.recordRuleResult(decl, hashes, ruleInputSig(decl, hashes), recorded); + done++; + if (recorded !== null) { + console.log(`[${done}/${runList.length}] ${label(decl)}`); + if (result.output !== '') { + console.log(indent(result.output)); + } + } else { + failures.push(decl); + console.error(`[${done}/${runList.length}] FAILED: ${label(decl)}`); + console.error(` command: ${decl.command}`); + if (result.output !== '') { + console.error(indent(result.output)); + } + if (missingOutputs.length > 0) { + console.error(` command succeeded but did not produce: ${missingOutputs.join(' ')}`); + } + if (opts.failFast) { + ac.abort(); + } } - } else { + } catch (err) { + // Unexpected (infrastructure) error: count the rule failed and + // stop scheduling; something systemic is wrong. failures.push(decl); - console.error(`[${done}/${runList.length}] FAILED: ${label(decl)}`); - console.error(` command: ${decl.command}`); - if (result.output !== '') { - console.error(indent(result.output)); - } - if (missingOutputs.length > 0) { - console.error(` command succeeded but did not produce: ${missingOutputs.join(' ')}`); - } - if (opts.failFast) { - ac.abort(); - } + console.error(`FAILED (internal error): ${label(decl)}`); + console.error(indent(err instanceof Error ? err.stack ?? err.message : String(err))); + ac.abort(); } }); process.off('SIGINT', onSignal); @@ -269,8 +301,8 @@ async function main() : Promise { if (runList.length > 0) { parts.push(`${done - failures.length} ran`); } - if (plan.renames.length > 0) { - parts.push(`${plan.renames.length} renamed`); + if (renamed > 0) { + parts.push(`${renamed} renamed`); } if (plan.adopt.length > 0) { parts.push(`${plan.adopt.length} adopted`); diff --git a/tools/build/plan.ts b/tools/build/plan.ts index 30f712c0..ae8a6282 100644 --- a/tools/build/plan.ts +++ b/tools/build/plan.ts @@ -49,7 +49,10 @@ export function computePlan(opts : { const renameIndex = new Map(); for (const s of stored) { storedByKey.set(s.key, s); - if (s.ok) { + // Rename detection is restricted to single-input, no-deps rules: + // sheet-style rules embed input *names* in their output bytes, so + // identical input content does not imply identical outputs there. + if (s.ok && s.inputs.length === 1 && !s.inputs[0]!.isDep) { const key = renameKey(s.template, s.inputSig); let list = renameIndex.get(key); if (list === undefined) { @@ -112,16 +115,20 @@ export function computePlan(opts : { } } else { reason = 'new'; - const sig = ruleInputSig(decl, hashes); - const candidates = renameIndex.get(renameKey(decl.template, sig)) ?? []; - const from = candidates.find(c => - c.outputs.length === decl.outputs.length && outputsIntact(c)); - if (from !== undefined) { - plan.renames.push({decl, from}); - continue; + if (decl.inputs.length === 1 && decl.deps.length === 0) { + const sig = ruleInputSig(decl, hashes); + const candidates = renameIndex.get(renameKey(decl.template, sig)) ?? []; + const from = candidates.find(c => + c.outputs.length === decl.outputs.length && outputsIntact(c)); + if (from !== undefined) { + plan.renames.push({decl, from}); + continue; + } } } - if (adopt && decl.outputs.every(p => statOutput(p) !== null)) { + // Adoption only covers rules we know nothing about; a known-dirty + // rule (input-changed, tampered, failed) must actually run. + if (adopt && reason === 'new' && decl.outputs.every(p => statOutput(p) !== null)) { plan.adopt.push(decl); } else { plan.run.push({decl, reason}); diff --git a/tools/build/test/exec.test.ts b/tools/build/test/exec.test.ts new file mode 100644 index 00000000..d3599c92 --- /dev/null +++ b/tools/build/test/exec.test.ts @@ -0,0 +1,29 @@ + +import assert from 'node:assert/strict'; +import {test} from 'node:test'; + +import {runShell, workerPool} from '../exec.ts'; + +test('runShell runs && chains from stdin and reports exit status', async () => { + const signal = new AbortController().signal; + const ok = await runShell('echo one && echo two', {cwd: process.cwd(), signal}); + assert.equal(ok.code, 0); + assert.equal(ok.output, 'one\ntwo\n'); + const bad = await runShell('echo partial && false && echo never', {cwd: process.cwd(), signal}); + assert.equal(bad.code, 1); + assert.equal(bad.output, 'partial\n'); +}); + +test('workerPool waits for every worker before rethrowing', async () => { + let finished = 0; + await assert.rejects( + workerPool([1, 2, 3, 4], 2, async item => { + if (item === 1) { + throw new Error('boom'); + } + await new Promise(resolve => setTimeout(resolve, 20)); + finished++; + }), + /boom/); + assert.equal(finished, 3); +}); diff --git a/tools/build/test/plan.test.ts b/tools/build/test/plan.test.ts index 612d6bec..080f0b27 100644 --- a/tools/build/test/plan.test.ts +++ b/tools/build/test/plan.test.ts @@ -132,6 +132,73 @@ test('rename does not match differing content or tampered source outputs', () => assert.deepEqual(plan.run.map(r => r.reason), ['new']); }); +test('rename is restricted to single-input, no-deps rules', () => { + // multi-input: same output basename, same content, but never rename-matched + resetRules(); + rule(['src/a.png', 'src/b.png'], {cmds: ['c %f %o']}, 'out1/x.png'); + rule(['src/a2.png', 'src/b2.png'], {cmds: ['c %f %o']}, 'out2/x.png'); + const [multiOld, multiNew] = getRules() as [RuleDecl, RuleDecl]; + const multiHashes = new Map([ + ['src/a.png', hashOf('A')], ['src/b.png', hashOf('B')], + ['src/a2.png', hashOf('A')], ['src/b2.png', hashOf('B')], + ]); + let plan = computePlan({ + current: [multiNew], + stored: [stored(multiOld, multiHashes)], + hashes: multiHashes, + statOutput: statFrom({'out1/x.png': GOOD}), + adopt: false, + }); + assert.deepEqual(plan.run.map(r => r.reason), ['new']); + + // dep-bearing: same template and sig, but never rename-matched + resetRules(); + forEachRule('src/a.png', {deps: 'src/d.json', cmds: ['c %f %o']}, 'out/%b'); + forEachRule('src/b.png', {deps: 'src/d.json', cmds: ['c %f %o']}, 'out/%b'); + const [depOld, depNew] = getRules() as [RuleDecl, RuleDecl]; + const depHashes = new Map([ + ['src/a.png', hashOf('SAME')], ['src/b.png', hashOf('SAME')], + ['src/d.json', hashOf('D')], + ]); + plan = computePlan({ + current: [depNew], + stored: [stored(depOld, depHashes)], + hashes: depHashes, + statOutput: statFrom({'out/a.png': GOOD}), + adopt: false, + }); + assert.deepEqual(plan.run.map(r => r.reason), ['new']); +}); + +test('rename does not match across input extension changes', () => { + const oldDecl = makeForeach('src/a.png', 'build/out'); + resetRules(); + forEachRule('src/a.gif', {cmds: ['convert %f %o']}, 'build/out/%b'); + const newDecl = getRules()[0]!; + const hashes = new Map([['src/a.png', hashOf('SAME')], ['src/a.gif', hashOf('SAME')]]); + const plan = computePlan({ + current: [newDecl], + stored: [stored(oldDecl, hashes)], + hashes, + statOutput: statFrom({'build/out/a.png': GOOD}), + adopt: false, + }); + assert.deepEqual(plan.run.map(r => r.reason), ['new']); +}); + +test('adopt does not bless known-dirty rules', () => { + const decl = makeForeach('src/a.png', 'build/out'); + const plan = computePlan({ + current: [decl], + stored: [stored(decl, new Map([['src/a.png', hashOf('OLD')]]))], + hashes: new Map([['src/a.png', hashOf('NEW')]]), + statOutput: statFrom({'build/out/a.png': GOOD}), + adopt: true, + }); + assert.equal(plan.adopt.length, 0); + assert.deepEqual(plan.run.map(r => r.reason), ['input-changed']); +}); + test('adopt records existing outputs instead of running', () => { resetRules(); rule('src/a.png', {cmds: ['convert %f %o']}, 'build/present.png'); diff --git a/tools/build/test/sprites.test.ts b/tools/build/test/sprites.test.ts index 00b92510..a7d45d37 100644 --- a/tools/build/test/sprites.test.ts +++ b/tools/build/test/sprites.test.ts @@ -23,7 +23,7 @@ test('base strips directory and final extension', () => { test('spriteglob filters by flag truthiness', () => { const dir = fs.mkdtempSync(pathlib.join(os.tmpdir(), 'spriteglob-')); try { - for (const name of ['1.png', '1-a.png', '2-b.png', '2-b-s.png', '3-g.png']) { + for (const name of ['1.png', '1-a.png', '2-b.png', '2-b-s.png', '3-g.png', '.hidden.png']) { fs.writeFileSync(pathlib.join(dir, name), ''); } assert.deepEqual(