Skip to content

keep-going build deletes outputs when a Tupfile fails to EVALUATE (error directive, bad macro, broken include) #128

Description

@typeless

Sibling of #126. #126/#127 cover a Tupfile that fails to parse syntactically. This is the broader class: a Tupfile that parses fine but fails to evaluate — an error directive, an unknown bang macro (!nope), a broken included file. Under --keep-going such a directory's built outputs are deleted.

Pre-existing on main; not a regression from #127 (which deliberately scopes to the syntactic case). Found by adversarial review of #127.

Repro A — the basic deletion (verified)

# fully-built alpha/beta project, then break beta by evaluation:
$ printf 'error broken\n: b.c |> cp %%f %%o |> b.out\n' > beta/Tupfile
$ putup -B build -k
[build] Nothing to do.
$ ls build/beta/b.out          # DELETED

Same with an unknown bang macro (: b.c |> !nope |> b.out) or a broken included file.

Repro B — verbose makes it worse (verified)

add_tupfile's statement loop (src/graph/builder.cpp:2433-2441) returns early on error only if (!verbose); under -v it accumulates into state.errors and then returns success (:2454). state.errors has no consumer, so the diagnostics also vanish:

$ putup -B build -k -v
... Parsed 2 Tupfiles ...
[build]   Removed stale: build/beta/b.out      # deleted, and NO error ever printed

So any fix keyed off add_tupfile's Result is bypassed under -v.

Why the naive fix is wrong (a trap for the fixer)

The obvious fix — "only record the dir as parsed when add_tupfile succeeds" — introduces a graph-corruption regression. An eval-failed dir then stays out of the parsed set, so the skip-guard in parse_directory (src/cli/context.cpp:419) no longer short-circuits, and a demand-driven re-parse (cross-dir group ref or input, src/graph/builder.cpp:179-190) runs add_tupfile a second time on a graph that already holds the first attempt's partial state. add_command_node never dedups, so this creates a phantom output-less command:

  • No -k: build aborts with a misleading Unable to create output '...' because it is already owned by command: ... instead of the user's actual error.
  • With -k: the phantom executes garbage (cp: missing file operand); and a second -k run reports Nothing to do (up to date) — the broken Tupfile goes invisible and CI-green.

(All verified against the built binary during review.)

Correct direction

Two changes, together:

  1. Make evaluation failure reliably detectable regardless of -v — either have add_tupfile return failure when state.errors grew during the call (keeping the verbose "collect all errors" behavior but not lying about the result), or check the error delta at the call site.
  2. Prevent the re-parse of a failed dir — record failed dirs in a set the parse_directory skip-guard consults, so add_tupfile never runs twice, while keeping failed dirs OUT of the authoritative parsed_dirs the stale-removal gate uses (Gate stale-output removal on authoritative directories #127).

Then the authoritative gate from #127 preserves eval-failed dirs the same way it already preserves syntactic-failed ones. Needs tests for: the -v variant, an eval-failed dir referenced cross-directory (the phantom scenario), and error/bad-macro/broken-include variants.

Root cause family

Shares the #122/#124/#125/#126 root: a command absent from the graph is assumed removed regardless of why. #127 closes the out-of-scope and syntactic-parse reasons structurally; this is the evaluation-failure reason.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions