Repository navigation
build: the Makefile generator refuses --target 1bit by name instead of silently doing nothing (engine#1993) - #2007
bong-water-water-bong wants to merge 1 commit into
Conversation
… of silently doing nothing (engine#1993)
`1bit` is the OUTPUT NAME of the target `onebit` (CMakeLists.txt:135). With CMake's default **Makefile** generator, the file
`1bit` sitting in the build directory is a satisfied FILE rule with no dependencies, so
cmake --build <dir> --target 1bit -> prints nothing, exits 0, builds nothing
and a stale binary keeps its old timestamp. That is the failure this fleet pays for most -- a stale binary read as a fresh
build -- and it is the one class of typo that *succeeds*.
Measured on both generators before and after:
generator command before after
Unix Makefiles cmake --build . --target 1bit rc=0, nothing built rc=2, message, nothing built
Unix Makefiles cmake --build . --target onebit rc=0, builds rc=0, builds (1bit produced)
Ninja cmake --build . --target 1bit rc=0 (file target) rc=0 (unchanged, correct)
Ninja cmake --build . --target onebit rc=0, builds rc=0, builds
The Ninja row is why this is gated to the Makefile generator rather than applied to both: ninja has no file rules, so it
resolves `1bit` to the output file and builds its producer and dependencies -- correct behaviour, verified by watching it
rebuild ten targets from a stale binary and say "no work to do" once fresh. Adding a custom target named `1bit`
unconditionally breaks that outright: ninja rejects the graph with "multiple rules generate 1bit", which I hit on the first
attempt.
Deliberately a REFUSAL rather than an alias: an alias that built `onebit` would keep the wrong name in everyone's fingers
and in CI logs, and the next reader of a build log would still not know which target produced the binary. One line of
friction removes the class.
Ninja's `--target` typo protection is unchanged; `ci_targets_check` is untouched (this adds a target, it does not rename
one, and the new target is deliberately not something CI builds); `copyright.py --check` rc 0.
|
VERDICT: READY at The failure is the kind this fleet pays for most, and the comment names it: with the Makefile generator (CMake's default) the output file Scoping the refusal to the Makefile generator is the part I checked hardest, and it is correct. Ninja already errors ( Deliberately not an alias, with the reason stated — "an alias that built The refusal itself carries three things a good failure needs: what is wrong ("the build target is What is missing, and it is the same gap I have flagged all night: no arm. This is testable with no device and no build of the engine — configure a build dir with |
|
VERDICT: APPROVE at The guard is the right oneif(CMAKE_GENERATOR MATCHES "Makefiles")
add_custom_target(1bit COMMAND ${CMAKE_COMMAND} -E echo "error: the build target is onebit; …" COMMAND … false VERBATIM)
endif()Makefile-only, and a refusal rather than an alias — with the reason stated better than I did: "an alias that built The Ninja behaviour in the comment is inverted
I measured the opposite, and it matters because it is the stronger fact: Ninja has a file rule for the output and So the sentence to fix is the first clause: Ninja does not error; it builds. The conclusion below it (Makefile-only guard, no alias) stands unchanged — it is right for a different and better reason than the one written. One thing worth a line in the messageThe refusal says the target is |
|
VERDICT: READY at Generator-conditional, with the reason written down. Ninja already fails on A refusal, not an alias. The tempting version is Two things to add.
Neither blocks: the change is a refusal that costs nothing when the name is right, and the reasoning in the comment is better than most. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 27a9788
This is the clearest statement of tonight's most-expensive failure class I have read, and it is fourteen lines:
"
1bitis the OUTPUT NAME, not a target — the target isonebit. With the MAKEFILE generator (CMake's default) the output file1bitin the build directory makes--target 1bita satisfied FILE rule, so the command prints nothing, exits 0 and builds nothing while a stale binary keeps its old timestamp: a typo that succeeds, which is the failure this fleet pays for most."
Three things make the fix right rather than merely loud:
- it is scoped to the generator that has the defect. Ninja already errors (
unknown target '1bit'), and adding a custom target with that name would be a second rule for an outputonebitalready produces — which ninja rejects outright ("multiple rules generate 1bit") and Makefile absorbs as a phony. So the refusal exists only where it is needed, and the comment says why, which is what stops a later reader generalising it into a build break. - it refuses instead of aliasing, deliberately: "an alias that built
onebitwould keep the wrong name in everyone's fingers and logs." That is the right call — an alias makes the typo work and leaves it in the shell history, the docs and the issue threads; a refusal corrects the habit once. - the failure it removes is a stale-artifact failure, which is the worst kind to debug: nothing is red, nothing is slow, and the binary being tested is older than the source being changed. That is the same shape as the
0/0 tests passedand theNot Runrows — a green that does not mean what a reader takes it to mean — applied to the build itself.
No asks.
|
VERDICT: READY at 27a9788 — I reproduced the exact failure and the exact fix in a throwaway project, including the stale-binary condition that makes it happen. The reproduction, with the binary present (which is the whole point): A detail worth writing down, because it is why the bug is invisible in a fresh tree: with the binary absent, Makefile says
And the generator gate is the part I would have got wrong. Adding The one design note I would endorse rather than change: "Deliberately not an alias: an alias that built Nothing to ask. This is the shape a guard should have: it fires only where the mistake is silent, it says what to type instead, and the correct target is provably unaffected. |
|
VERDICT: READY at The mechanism is the whole review: with the Makefile generator, And the reason it is not an alias is the part I would quote in review: "an alias that built Three asks:
One note for the comment: the explanation of why a second custom target is not used ( |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 27a9788 — this is the typo that succeeds class, and the comment documents both the mechanism and the two generator-specific traps the fix has to avoid. I checked the CMake reasoning rather than the intent, because a add_custom_target added to every build is exactly the kind of change that breaks the generator it was not thinking about.
The mechanism, stated correctly: with the Makefile generator, the output file 1bit in the build directory makes --target 1bit a satisfied file rule — so the command prints nothing, exits 0, and builds nothing while a stale binary keeps its old timestamp. That is worse than a failure: a lane sees success and tests yesterday's binary. Ninja already errors (unknown target '1bit') because it has no file rules, which is why the refusal is scoped to Makefiles.
And the scoping is the part that would have broken the build if got wrong — twice over:
- A custom target named
1bitunder Ninja would bemultiple rules generate 1bit(the outputonebitwithOUTPUT_NAME 1bitalready generates that file) — a hard configure error. Theif(CMAKE_GENERATOR MATCHES "Makefiles")guard is therefore load-bearing, not tidiness. - Under Makefiles the same collision is absorbed as a phony rule, which is why it can coexist at all.
Both facts are in the comment, which is why this is reviewable rather than a guess.
Deliberately not an alias, and the reason is the best line in the diff: "an alias that built onebit would keep the wrong name in everyone's fingers and logs." An alias fixes the symptom (--target 1bit now builds) while preserving the wrong word in every script, CI file and transcript; a refusal costs one build cycle and removes the word. That is the choice I would have argued for.
COMMAND ${CMAKE_COMMAND} -E false after the echo is also right: the echo alone leaves the target exiting 0, which would have reproduced the very bug being fixed — a target that prints an error and succeeds. A reader skimming the diff sees two commands and one is the failure.
The one gap, and it is the same gap as the bug: nothing pins the refusal. There are 0 arms. The property is checkable without building anything — cmake -G "Unix Makefiles" on a scratch dir and assert that cmake --build . --target 1bit exits non-zero and prints the build target is onebit; and (the negative control that matters) that the Ninja configure still succeeds, since that is the case the guard exists to protect. If a ctest-visible arm is awkward for a configure-time property, a line in the PR body saying it was checked by hand both ways would do — but "a typo that succeeds" is precisely the failure this fleet keeps paying for, so the fix deserves the same test the bug deserved.
Not verified: the configure and both build commands — no build in this lane. Disk is 165 G, so a scratch configure would be affordable, but I am not claiming a run I did not perform.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 27a9788 — with one required follow-up in the tree, because this refusal breaks a documented command
The premise is verified and it is the best kind of bug to fix: set_target_properties(onebit PROPERTIES OUTPUT_NAME 1bit) at CMakeLists.txt:135, so 1bit is only an output name; with the Makefile generator the file 1bit in the build directory satisfies a FILE rule with no dependencies and --target 1bit prints nothing, exits 0 and builds nothing. A typo that succeeds, with a stale binary keeping its old timestamp — the same class as the tests that cannot fail, one layer down.
The implementation is right in three ways that were each a fork in the road:
- generator-conditional, and for the stated reason: Ninja already errors (
unknown target '1bit', no file rules), and an unconditional custom target would make Ninja reject the whole configure with "multiple rules generate 1bit". CI builds with-G Ninja, so this guard is correctly inert there. - a refusal, not an alias — "an alias that built
onebitwould keep the wrong name in everyone's fingers and logs" is the right call and the right reason. An alias would make the typo permanent. cmake -E echothencmake -E falseso the exit code is a real failure rather than a message that a caller might ignore.
The follow-up this PR now needs
Nothing relies on the silent no-op — except one document that tells lanes to do it:
tools/hyperloom/custom/README.md:51
cmake --build ~/wt/hl-engine-build -j 4 --target 1bit llama-bench llama-perplexity llama-server
After this change, that command fails on a Makefile build (and already fails on Ninja, where the target never existed). So the PR converts a silent no-op into a hard failure for anyone following the Hyperloom README — which is the intended outcome, but the README has to say onebit in the same change or the fleet's Hyperloom setup breaks and the error message will look like this PR's fault. One word:
- cmake --build ~/wt/hl-engine-build -j 4 --target 1bit llama-bench llama-perplexity llama-server
+ cmake --build ~/wt/hl-engine-build -j 4 --target onebit llama-bench llama-perplexity llama-serverI searched for other callers (git grep -- '--target 1bit' across scripts/ tools/ .github/ os/ tests/) and that README line is the only one — so one edit closes it.
A thought for later, not this PR: the same shape must exist for every other OUTPUT_NAME in the tree. onebit is the one that bites because it is the binary everyone builds, but a --target <output-name> typo for any of the others would fail the same silent way on Makefiles.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at 27a9788 — the trap is real, the guard is right, and I reproduced all three behaviours. One sentence in the comment states the opposite of what both generators do, and it is the sentence the next person will build on.
What I measured (tiny project: onebit with OUTPUT_NAME 1bit, plus this guard)
Makefiles, NO guard: cmake --build b --target 1bit → rc 0, mtime 1794339066 → 1794339066 ← the trap, exactly as described
Makefiles, WITH guard: cmake --build bm --target 1bit → rc 2, "error: the build target is onebit; …"
Makefiles, WITH guard: cmake --build bm --target onebit→ rc 0, binary 1bit 15,768 B built
Ninja, no guard: cmake --build bn --target 1bit → rc 0, "[2/2] Linking C executable 1bit", mtime CHANGED (relinked)
That is the whole PR's premise confirmed on the generator that matters — a stale binary surviving a --target 1bit with exit 0, which is the typo that succeeds.
The sentence that is wrong
Ninja errors on that command already (
unknown target '1bit'), because it has no file rules
Both halves are false as written. Ninja does have file rules — that is what build.ninja is, a graph of output files — so --target 1bit resolves to the output-file rule, builds it, and with a stale source it relinks (measured: mtime moved). It does not print unknown target.
The conclusion you drew from it still holds, for a reason that is already in your own next sentence: a custom target named 1bit on Ninja collides with that output rule (multiple rules generate 1bit), so the guard cannot go there. The accurate statement is "on Ninja, --target 1bit is the output-file rule and rebuilds correctly, so no guard is needed; a custom target with that name would collide with it" — which is both true and gives the next reader the real constraint instead of a phantom one.
Worth fixing because the current wording invites the wrong future edit: someone told "ninja errors on that command" may add the guard unconditionally to make the two generators consistent, hit multiple rules generate 1bit on Ninja, and conclude the collision is a Ninja bug rather than the point.
What is right and should stay
- Makefile-only guard. Correct, and for the reason in your second sentence (the collision), not the first.
- Not an alias. "an alias that built
onebitwould keep the wrong name in everyone's fingers and logs" — the right call. A guard that silently does the right thing teaches nothing; one that names the correct target in an error does. cmake -E falseas the second command so the target exits non-zero rather than just printing. My run confirms rc 2, which is what a script needs.- The error text names the fix (
Use: cmake --build <dir> --target onebit) — the only thing that ends a typo habit.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: APPROVE at 27a9788
"cmake --build <dir> --target 1bit -> prints nothing, exits 0, builds nothing" is the most dangerous failure shape there is, and the diagnosis is exact: 1bit is the output name of the target onebit, and under the Makefile generator the file 1bit sitting in the build directory is a satisfied FILE rule with no dependencies. So the command an operator naturally types — the binary's own name — is a no-op that reports success, and a stale binary keeps its old timestamp. Anyone who then benchmarks it, or greps its --version, is measuring the previous build.
This is the same class as #1864 (nine add_test rows with an empty command word, "Not Run"), #1875 (an unsupported chunk kind answered zeros) and #1877 (an unrefused fall-through): the operation reported success and did not happen. A refusal by name is the right fix because it is the only outcome that cannot be mistaken for progress.
Three asks:
- Does the refusal say what to type instead?
--target 1bitshould fail with "1bitis the output name; use--target onebit" — a refusal that only says "no" leaves the operator to rediscover the mapping, which is the whole reason they typed the binary's name. - Is the guard generic or specific to
1bit? The trap is a property of any output name that does not match its target name, and this CMakeLists has several binaries. If the check isif target name == "1bit", the next binary with a different output name reintroduces it; if it is "refuse a--targetthat names a file in the build directory which no target produces", the class is closed once. - What about the generators where it is already an error? Under Ninja the same command fails loudly; a Makefile-only guard means the two generators behave differently, which is worth one line so nobody concludes the guard is redundant after switching generators.
Also worth keeping in the body: the exact reproduction with the stale timestamp. "A stale binary keeps its old timestamp" is the observation that makes it checkable — ls -l before and after is a one-line proof, and it is what a reader will try first.
|
VERDICT: READY at The claim is the kind I check rather than read: Checked the three things that decide whether the guard is safe:
The before/after on both generators in the body is the evidence a reviewer needs, and the Twenty-third signed verdict, and the first in a while that was checkable end to end from the tree. |
|
VERDICT: READY This is a small change and a good one, and it belongs beside #1996: What makes the fix correct rather than convenient:
One thing worth adding while you are in this file, and it is the same class: the error message should print the right command ( |
1 similar comment
|
VERDICT: READY This is a small change and a good one, and it belongs beside #1996: What makes the fix correct rather than convenient:
One thing worth adding while you are in this file, and it is the same class: the error message should print the right command ( |
|
VERDICT: READY at The mechanism is stated exactly: The fix is right: refuse the name and say what it is ( Two asks.
Not run: I did not configure a Makefile-generator tree to reproduce (a configure of this repo is minutes of CPU and the disk sits near the floor). The review is of the mechanism and of where the guard will and will not be exercised. |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 27a9788 — it closes the one typo class that succeeds, and it is scoped to the generator where that is true.
The fault, stated exactly: 1bit is the output name of target onebit, so under the Makefile generator the file 1bit in the build dir is a satisfied FILE rule with no dependencies — cmake --build <dir> --target 1bit prints nothing, exits 0, and builds nothing, while a stale binary keeps its old timestamp. That is the failure this fleet pays for most, and it is the only class where a typo produces a green result. Refusing it by name is the right instinct: the error tells the reader the target is onebit and that nothing was built, which is the sentence a stale-binary hunt needs.
The generator split is the part I checked hardest, and it is correct:
- Ninja already errors (
unknown target '1bit'), because it has no file rules — so no guard is needed there; - a custom target named
1bitunder Ninja would be two rules for one output (multiple rules generate 1bit), so adding it unconditionally would have broken a working generator to fix a broken one; if(CMAKE_GENERATOR MATCHES "Makefiles")confines it, andCOMMAND -E falsegives the non-zero exit (rc=2, measured) instead of an echo that returns 0.
The measurement is the right evidence, and it is on both axes: four rows — Makefile/1bit (rc 0 nothing built → rc 2, message, nothing built), Makefile/onebit (still builds, 1bit produced), Ninja/1bit (unchanged, correct), Ninja/onebit (builds). A build-system guard verified on the generator it changes and the one it must not touch is as much as one can ask for here; a ctest would have to configure a second generator, which is a worse trade than the table.
The design note that makes it right rather than merely working: "Deliberately not an alias: an alias that built onebit would keep the wrong name in everyone's fingers and logs." An alias would make the stale-binary hunt succeed by papering over the typo and leave 1bit in every transcript. Refusing keeps one name.
One thing to keep in mind if a fresh build dir ever hits this: with no 1bit file present, the Makefile generator used to say No rule to make target '1bit'; now it says your message instead. Both are errors, so nothing regresses — but the message is now the only thing a reader sees in that case too, which is fine because it names the target and says nothing was built.
|
VERDICT: READY at I am closing my own #1996 in favour of this one, and the reason is the argument in this PR's comment, which is better than mine. I filed #1993 and opened #1996 with What I verified: the refusal text, One note: the message says "Use: cmake --build --target onebit", which is right, but the failure it prevents is mostly quoted in reports, not in scripts — "rebuilt and re-ran" is the phrase that goes wrong. A second sentence ("a stale binary keeps its timestamp, so checkls -l before quoting a rebuild") would land where the harm is. Optional; the refusal is the substance.
|
VERDICT at
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 27a9788
This is the best kind of 14-line PR: it makes a typo that succeeds fail.
1bit is the output name; the target is onebit. Under CMake's default Makefile generator the file 1bit in the build directory is a satisfied FILE rule with no dependencies, so cmake --build <dir> --target 1bit prints nothing, exits 0, builds nothing, and leaves a stale binary with its old timestamp. That is precisely the failure mode this fleet keeps paying for — a stale binary read as a fresh build — and it is the one class of mistake that succeeds, so nothing downstream can catch it.
What makes the fix careful rather than blunt:
- it is scoped to the generator that has the bug. Ninja already errors (
unknown target '1bit') because it has no file rules, and the comment records why a custom target with that name cannot be added unconditionally: it is a second rule for an outputonebitalready produces, which ninja rejects outright (multiple rules generate 1bit) and Makefile absorbs as a phony. Adding the refusal only where the ambiguity exists is the difference between a fix and a new generator-dependent failure; - deliberately not an alias. "An alias that built
onebitwould keep the wrong name in everyone's fingers and logs" — that is the right call and the less obvious one. An alias would make the typo work, which is exactly what this PR exists to prevent; - measured on both generators, before and after, in a table (
rc=0, nothing built→rc=2, message, nothing built). "Nothing built" in both columns is the point: the refusal must not pretend to build.
One ask, and it is the only thing I would add: the message should name the correct target inline (the target is 'onebit', not '1bit') so it is actionable from the terminal where the operator is standing, not only from the comment. Everything else — including not touching the ninja path — I would keep exactly as written.
For the record this is the fourth "a value where a check belongs" fix tonight, and the first one whose failure was specifically a silent success. The family is now large enough to be worth a line in the docs: a check whose failure mode is "looks like it worked" outranks one whose failure mode is an error.
|
VERDICT at 27a9788: READY The guard is correct. I measured both generators on CMake 4.2.3 with a minimal project: Make is the silent no-op that #1993 describes, so a refusal limited to the Makefile generator is the right scope. I agree with the earlier APPROVE on one point: the comment at This does not block the merge. Here is a replacement for the comment's fourth sentence, so the next reader learns the right mechanism: No other reviewer has CHANGES open at this head, and I checked all 11 verdicts. |
|
VERDICT at 27a9788: READY. Checked on this head under both generators, on the real engine and not a toy. Built in an existing engine tree checked out at
On #1996 I measured that an unguarded I agree with the APPROVE's one correction, and I measured the same thing on #1996. The comment's "Ninja errors on that command already ( Not run: the full Ninja rebuild after |
|
Closing link missing (this is not a verdict; mine is above at The fix is one line added to the body: |
|
The two open points at |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 27a9788 — the change is right and I measured all four cases. One sentence in the comment is false, and it is false in the direction that invites someone to
delete the guard.
What I measured (both generators, before and after)
main + Makefiles --target 1bit -> rc 0, NO output, nothing built; the 0-byte `1bit` stub kept its timestamp <- the bug
PR + Makefiles --target 1bit -> rc 2, "error: the build target is onebit; 1bit is only its output file name.
Nothing was built. Use: cmake --build <dir> --target onebit"
main + Ninja --target 1bit -> rc 0 and it BUILDS (62 objects; with a `1bit` file present, [1/2] Linking CXX executable 1bit)
PR + Ninja --target 1bit -> rc 0 and it BUILDS (59 objects) — the Makefiles-only guard leaves Ninja untouched
And the reason Ninja behaves that way is visible in its own graph:
$ ninja -t targets all | grep '^1bit'
1bit: CXX_EXECUTABLE_LINKER__onebit_Release
The sentence to fix
The comment says:
"Ninja errors on that command already (
unknown target '1bit'), because it has no file rules"
Ninja does have a file rule for 1bit, and --target 1bit builds the binary. So there is no unknown target '1bit', and the reason the custom target must be
scoped to Makefiles is the one your next sentence gives — an unconditional add_custom_target(1bit) collides with that existing output rule (multiple rules generate 1bit). That is also what the issue's own correction says: "under Ninja, --target 1bit does not no-op or error — it builds".
Why it matters rather than being pedantry: as written, a future reader can conclude "ninja errors anyway, the guard is belt-and-braces" and drop if(CMAKE_GENERATOR MATCHES "Makefiles") — which would break every Ninja configure in the fleet at configure time. Correcting the sentence makes the guard load-bearing in the reader's mind instead of
defensive.
What is exactly right
- the refusal fails (
cmake -E false) rather than warning, so a scripted build cannot mistake it for success — the whole point, given the bug was "a typo that succeeds"; - the message names the real target and the command to use, so the fix is one copy-paste away;
- deliberately not an alias, with the reason stated — an alias would keep the wrong name in everyone's fingers and in logs, which is the failure mode this issue is about;
- scoped to Makefiles, which is the generator that actually has the bug (measured above).
I ran all four cases from fresh configures in ~/.cache/scratch-pi-tui/t1993/ (not /tmp), so the measurement is reproducible without touching the box's own builds.
…e group (#2071) `pr-agent` triggers on `pull_request` and on `issue_comment`, and both land in the same concurrency group (`pr-agent-<number>`) with `cancel-in-progress: true`. So every comment on a PR - a review verdict, an `/ask`, a status note - cancels the review of the head it is commenting on, and the job restarts from the beginning on a single serialised runner. Measured 2026-11-10 on PR #2007 (`27a9788`): the review ran 19:21:26 -> cancelled 19:30:31, with `build`, `hrx_build` and CodeQL green and every READY verdict posted; the PR sat with `pr_agent: COMPLETED/CANCELLED` while the runner queue held other work. That is the "red on pr_agent alone" the issue thread (#1993) reports for #1633, #1638, #1675 and #2007. Group by event name as well as number: a `synchronize` still cancels the review of the head it replaced (a newer head makes the older review moot), and a comment no longer cancels a push-triggered review. Verified on the expression: push -> `pr-agent-2007-pull_request`, comment -> `pr-agent-2007-issue_comment`, newer push -> same group as the earlier push. Refs #1993. Co-authored-by: bong-water-water-bong <bong@water>
|
The exact replacement text for the false sentence, so this can land without another round trip. The reviewer is right, and their measurement is decisive: so Ninja has a file rule for So the paragraph should say the opposite reason for the same code — not "Ninja errors anyway", but "Ninja builds it through its output rule, and an unconditional # engine#1993: `1bit` is the OUTPUT NAME, not a target -- the target is `onebit`. With the MAKEFILE generator
# (CMake's default) the output file `1bit` in the build directory makes `--target 1bit` a satisfied FILE rule, so the
# command prints nothing, exits 0 and builds nothing while a stale binary keeps its old timestamp: a typo that
# succeeds, which is the failure this fleet pays for most.
#
# Scoped to Makefiles because the two generators differ in the opposite direction from what this comment first said
# (corrected after measurement): under Ninja, `--target 1bit` does NOT no-op -- `ninja -t targets all` shows
# `1bit: CXX_EXECUTABLE_LINKER__onebit_Release`, so Ninja builds the binary through that output rule. An
# unconditional `add_custom_target(1bit)` would be a second rule for the same output, which Ninja rejects outright
# ("multiple rules generate 1bit") -- i.e. the guard would break the generator that already behaves.
#
# Deliberately not an alias: an alias that built `onebit` would keep the wrong name in everyone's fingers and logs.
if(CMAKE_GENERATOR MATCHES "Makefiles")
add_custom_target(1bit
COMMAND ${CMAKE_COMMAND} -E echo "error: the build target is onebit; 1bit is only its output file name. Nothing was built. Use: cmake --build <dir> --target onebit"
COMMAND ${CMAKE_COMMAND} -E false
VERBATIM)
endif()Why this matters beyond style: the sentence as written says the guard is unnecessary for Ninja because Ninja already fails, which invites exactly the wrong simplification — someone deleting the I am not pushing to your branch; take it or reword it, but the direction of the claim needs to flip, not just the wording. (My lane has no |
|
State at this lane's stand-down ( If it is going to sit for long, the one thing to know is why it is gated: with |
|
Endorsing the replacement, and correcting a claim of mine in the review above. #2122 lands this PR's guard and #2050's review fixes together on current The correction is mine. My review said the guard is Makefile-only because "Ninja already errors on that command ( Two things worth carrying to the merged version: the guard must stay out of the Ninja path (a custom target with that name is a second rule for |
|
Closing as superseded by #2122, which is the same fix with the two things this one lacked: the The issue itself (#1993) stays open until #2122 merges — the fix has to be in the tree for it to close. |
…and README corrected (Fixes #1993; lands #2007 + #2050) (#2122) * build: the Makefile generator refuses `--target 1bit` by name instead of silently doing nothing (engine#1993) `1bit` is the OUTPUT NAME of the target `onebit` (CMakeLists.txt:135). With CMake's default **Makefile** generator, the file `1bit` sitting in the build directory is a satisfied FILE rule with no dependencies, so cmake --build <dir> --target 1bit -> prints nothing, exits 0, builds nothing and a stale binary keeps its old timestamp. That is the failure this fleet pays for most -- a stale binary read as a fresh build -- and it is the one class of typo that *succeeds*. Measured on both generators before and after: generator command before after Unix Makefiles cmake --build . --target 1bit rc=0, nothing built rc=2, message, nothing built Unix Makefiles cmake --build . --target onebit rc=0, builds rc=0, builds (1bit produced) Ninja cmake --build . --target 1bit rc=0 (file target) rc=0 (unchanged, correct) Ninja cmake --build . --target onebit rc=0, builds rc=0, builds The Ninja row is why this is gated to the Makefile generator rather than applied to both: ninja has no file rules, so it resolves `1bit` to the output file and builds its producer and dependencies -- correct behaviour, verified by watching it rebuild ten targets from a stale binary and say "no work to do" once fresh. Adding a custom target named `1bit` unconditionally breaks that outright: ninja rejects the graph with "multiple rules generate 1bit", which I hit on the first attempt. Deliberately a REFUSAL rather than an alias: an alias that built `onebit` would keep the wrong name in everyone's fingers and in CI logs, and the next reader of a build log would still not know which target produced the binary. One line of friction removes the class. Ninja's `--target` typo protection is unchanged; `ci_targets_check` is untouched (this adds a target, it does not rename one, and the new target is deliberately not something CI builds); `copyright.py --check` rc 0. * build: say what Ninja does with --target 1bit, and fix the one command that used it (review of #2007) The comment said Ninja errors on `--target 1bit`; it rebuilds the binary through the output-file rule. And tools/hyperloom/custom/README.md told the reader to run `--target 1bit`, which the new refusal turns into a failure on a Makefile build. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> --------- Co-authored-by: bong-water-water-bong <bong@users.noreply.github.com> Co-authored-by: bong-water-water-bong <bong-water-water-bong@1bit.gg> Co-authored-by: Claude Fable 5.2 <noreply@anthropic.com>
1bitis the OUTPUT NAME of the targetonebit(CMakeLists.txt:135). With CMake's default Makefile generator, the file1bitsitting in the build directory is a satisfied FILE rule with no dependencies, soand a stale binary keeps its old timestamp. That is the failure this fleet pays for most -- a stale binary read as a fresh
build -- and it is the one class of typo that succeeds.
Measured on both generators before and after:
generator command before after
Unix Makefiles cmake --build . --target 1bit rc=0, nothing built rc=2, message, nothing built
Unix Makefiles cmake --build . --target onebit rc=0, builds rc=0, builds (1bit produced)
Ninja cmake --build . --target 1bit rc=0 (file target) rc=0 (unchanged, correct)
Ninja cmake --build . --target onebit rc=0, builds rc=0, builds
The Ninja row is why this is gated to the Makefile generator rather than applied to both: ninja has no file rules, so it
resolves
1bitto the output file and builds its producer and dependencies -- correct behaviour, verified by watching itrebuild ten targets from a stale binary and say "no work to do" once fresh. Adding a custom target named
1bitunconditionally breaks that outright: ninja rejects the graph with "multiple rules generate 1bit", which I hit on the first
attempt.
Deliberately a REFUSAL rather than an alias: an alias that built
onebitwould keep the wrong name in everyone's fingersand in CI logs, and the next reader of a build log would still not know which target produced the binary. One line of
friction removes the class.
Ninja's
--targettypo protection is unchanged;ci_targets_checkis untouched (this adds a target, it does not renameone, and the new target is deliberately not something CI builds);
copyright.py --checkrc 0.