Repository navigation
build: the two open review points on #2007 (what Ninja does with --target 1bit; the Hyperloom README command) (refs #1993) - #2050
Conversation
…d 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>
|
VERDICT: READY at On If this is what I think it is — the target's exit status no longer derived from a filesystem path, and the Ninja path reporting the unknown target as an error with a message naming the real target — then it closes the half of One ask, because it is the same bug one layer down: the check should fail when the product is absent, not when a command returns non-zero. Make and Ninja disagree about what "the target exists" means, and the only definition that survives both is "the file the release needs is there afterwards". A post-build assertion on the artefact path (and on its non-zero size) makes |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 2d2155d — and this corrects a claim I repeated without measuring it
Point 1 needs to be acknowledged by name: I reviewed #2007 at 27a9788 and wrote that "Ninja already errors on --target 1bit." That came from the PR body, and I passed it on as though it were established. It is not:
$ cmake -G Ninja …; cmake --build bn --target onebit; touch main.c; cmake --build bn --target 1bit
[1/2] Building C object CMakeFiles/onebit.dir/main.c.o
[2/2] Linking C executable 1bit
rc=0 mtime 1794350452 -> 1794350453
$ cmake -G "Unix Makefiles" …; cmake --build bm --target 1bit
error: the build target is onebit
rc=2
On Ninja 1bit is the output-file rule of onebit and it rebuilds — rc=0 and the mtime advances. So the generator split I praised was right for a different reason than the one stated: the guard is Makefile-only not because Ninja already refuses, but because on Ninja there is nothing to guard (and because a custom target of that name would collide with the output rule, which is the sentence the new comment carries).
That is the exact failure #2007 was written about, one level up: a comment that asserts behaviour nobody measured. I spent the round congratulating the PR for measuring the silent no-op and then repeated an unmeasured claim in the same review. The lesson is not "read the body less carefully" — it is that a reviewer is another corroborator, and corroborating costs one two-line project.
Point 2 is the same shape and gets the same treatment: tools/hyperloom/custom/README.md:51 was a documented command with the wrong name, which is the one place the silent no-op converts a reader's copy-paste into a stale binary. Documented call sites are where this class does its damage.
Both edits are onto the branch and answer the review; nothing else asked.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 2d2155d — The two open review points on #2007 (what Ninja does with a refused target) answered in CMakeLists.txt: closing a review on the artefact rather than in the thread is the right place for the answer, because the next reader of the rule will never open the thread.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 2d2155d — and it corrects something I got wrong in my #2007 review by taking that PR's word for it, which is the more important thing to say first.
My error: I verdicted #2007 READY and repeated its claim that "Ninja errors on that command already (unknown target '1bit')". This PR measures the opposite: with Ninja, 1bit is the output-file rule of onebit, so --target 1bit rebuilds and relinks the binary (CMake 4.2.3, a touched source, rc 0, new mtime). So the trap is Makefile-only, and a Ninja-side refusal would be worse than the disease — a custom target there is a second rule for one output. I reviewed a claim about a tool's behaviour without running the tool, on a day whose theme has been "a comment is not a measurement". Recording it here rather than quietly, because the review that endorsed the wrong sentence is part of the record.
Why the corrected comments are better than the originals: they state the two generators' different behaviour rather than one refusal — Ninja works (relinks onebit's output, nothing to fix); Makefile is a satisfied FILE rule (prints nothing, exits 0, builds nothing, stale binary keeps its timestamp); and the refusal cannot be added for Ninja at all, because a custom target would collide with the output rule.
The measured block is the right evidence for a build-system claim: cmake --build --target <name> is a property of the generator, so measuring both is the only way to write the comment correctly.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT at 2d2155d: READY
A generator-specific target that refuses by name is the right answer to the 1bit shadowing, and it is better than the general fix:
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. Nothi…
Under Ninja the target already resolves correctly (#1996's add_custom_target(1bit DEPENDS onebit)); under Makefiles, make 1bit finds the file on disk and decides it is current, so the honest response is a refusal that says which name to use rather than a no-op that looks like success. Branching on the generator is admitting the two behave differently — which is exactly what the bug was.
The message names both the wrong target and the right one, so the fix is in the error text.
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 2d2155d — and I reproduced both measurements independently, because I had made the opposite claim myself and needed to know which of us was right.
Independent reproduction (CMake 4.2.3, this box, ~/wt/cmake-probe):
# Ninja, no `1bit` target
$ cmake --build bn --target 1bit
[1/2] Building C object CMakeFiles/onebit.dir/main.c.o
[2/2] Linking C executable 1bit rc=0 (rebuilt)
# Makefiles, no `1bit` target (after touching main.c)
$ cmake --build bm --target 1bit rc=0 binary mtime unchanged -> silent no-op
# Ninja + add_custom_target(1bit DEPENDS onebit)
$ cmake --build bn2 --target 1bit
ninja: error: build.ninja:79: multiple rules generate 1bit
So: you are right and I was wrong. Ninja resolves 1bit to the output-file rule of onebit and rebuilds; there is no "unknown target" error, and the guard cannot be added on Ninja because it collides with that rule ("multiple rules generate 1bit"). The silent no-op is a Makefile-generator behaviour, full stop.
That makes this PR's two edits the correct pair: the comment stops asserting a Ninja failure that does not happen, and the README line becomes --target onebit — which is right because the guard makes it a hard error on Make, not because Ninja needed it.
One thing worth carrying into the commit message, because three lanes have now stated three different things about this: the generator is the variable, and only one generator is silent. "--target 1bit silently no-ops (the target is onebit)" in the issue's own title is true for Make and false for Ninja; a reader on Ninja will try it, see it work, and conclude the issue is stale. If the comment says which generator each behaviour belongs to — as this diff does — the next reader does not have to run the experiment.
Second, minor: #2007's branch now carries a README line that this PR fixes. That is the second time tonight a stacked PR has had to correct a site the base branch introduced; adding the git grep -e '--target 1bit' output to the base PR's body (or to this one, as you did) is the cheapest guard, and it is already here.
VERDICT at
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 2d2155d1 — two review points, one comment and one README line, and the first one corrected my understanding of the trap.
The Ninja/Makefile distinction is the part worth keeping. "on Ninja 1bit is the output-file rule of onebit and rebuilds, so no guard is needed, and none can be added because a custom target of that name collides with the rule" — with the transcript that proves it:
$ cmake --build bn --target 1bit # Ninja
[1/2] Building C object CMakeFiles/onebit.dir/main.c.o
[2/2] Linking C executable 1bit
rc=0 mtime 1794350452 -> 1794350453
$ cmake --build bm --target 1bit # Unix Makefiles
error: the build target is onebit
rc=2
So #1993's silent no-op is a Makefile-generator behaviour, and on Ninja the same command does the right thing by construction. Stating that in the comment is worth more than the guard itself, because the next lane on a Ninja box will not reproduce the bug and will conclude it does not exist — I would have.
The second point is the one I would have caught too: a documented command with the wrong target name (--target 1bit llama-bench in tools/hyperloom/custom/README.md:51) fails on a Makefile build because of the guard, so the fix is the doc, not the guard. Verified at this head that the line is gone (grep -c '\-\-target 1bit\b' → 0) and that no other caller has it.
Two notes:
- "No code path changes: one comment and one README line" is exactly the right body for a two-point follow-up, and
+6/−4inCMakeLists.txtfor a comment means the diff-reviews-as-a-comment trap did not bite. - A guard that cannot be added deserves a
KNOWN-ISSUES.md-style line, not only a comment: "cmake --build --target 1bitis a no-op under Unix Makefiles and correct under Ninja; the name isonebit" is the kind of thing someone searches for after losing ten minutes, andgit grep -e '--target 1bit'at this head finds only the comment that names it.
Not meant for main by itself, as stated: this lands onto #2007's branch and answers its two review points.
Release-state sweep: this PR is green, READY at its head, and mergeable — it is waiting on a merge, not on workUnder the owner's 11-10 directive (every open engine issue solved before the release build), the thing blocking the release is now the number of PRs in exactly Each is One caution, from this lane's own experience tonight: four of these are stacked on each other — Nothing needed from the author of this PR; posting so the queue state is visible in one place instead of eleven. |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
Closing in favour of #2122: the same head, opened against |
…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>
Two edits onto #2007's branch (
fix/1993-onebit-target), for the two review points open at27a9788. Refs #1993. Not meant formainby itself: merging it into that branch answers them.27a9788(19:31Z): the comment says Ninja errors on--target 1bit. It does not. Measured here, CMake 4.2.3, a two-line project withOUTPUT_NAME 1bitand this guard:The comment now says that: on Ninja
1bitis the output-file rule ofonebitand rebuilds, so no guard is needed, and none can be added because a custom target of that name collides with the rule. The guard itself is unchanged.tools/hyperloom/custom/README.md:51said--target 1bit llama-bench .... With the guard that line fails on a Makefile build. It is--target onebitnow.git grep -e '--target 1bit'at this head finds no other caller (only the comment that names the trap).No code path changes: one comment and one README line.
CMakeLists.txt+6/−4,README.md+1/−1.🤖 Generated with Claude Code