Leahnaya/sprites
1
0
Fork 0
mirror of https://github.com/smogon/sprites.git synced 2026-08-18 07:16:46 -05:00
Code Issues Actions Packages Projects Releases Wiki Activity

Fix review findings in tools/build

Browse Source 
- 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 <noreply@anthropic.com>
This commit is contained in:
Christopher Monsanto
2026-08-15 22:32:59 -04:00
parent 02194b17f0
commit 2d83f4a16f
9 changed files with 255 additions and 66 deletions
Download Patch File Download Diff File Expand all files Collapse all files

12
Buildfile.ts
View File

@@ -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: [

23
tools/build/api.ts
View File

@@ -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;
}

20
tools/build/db.ts
View File

@@ -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<bigint, StoredRule>();
const ruleRows = this.db.prepare<[], {
id : bigint, key : string, command : string, display : string | null,

27
tools/build/exec.ts
View File

@@ -8,6 +8,17 @@ export interface ExecResult {
durationMs : number;
}
const livePids = new Set<number>();
// 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<T>(items : readonly T[], jobs : number,
fn : (item : T, index : number) => Promise<void>) : Promise<void> {
let next = 0;
@@ -74,5 +94,10 @@ export async function workerPool<T>(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;
}
}
}

116
tools/build/index.ts
View File

@@ -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<number> {
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<number> {
}
// 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<number> {
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<number> {
let interrupted = false;
const onSignal = () => {
if (interrupted) {
killAllProcessGroups();
process.exit(130);
}
interrupted = true;
@@ -214,51 +237,60 @@ async function main() : Promise<number> {
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<number> {
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`);

25
tools/build/plan.ts
View File

@@ -49,7 +49,10 @@ export function computePlan(opts : {
const renameIndex = new Map<string, StoredRule[]>();
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});

29
tools/build/test/exec.test.ts Normal file
View File

@@ -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);
});

67
tools/build/test/plan.test.ts
View File

@@ -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');

2
tools/build/test/sprites.test.ts
View File

@@ -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(