Skip to content

hyperloom: build the target onebit, not the file 1bit — both generators do the wrong thing (engine#1993) - #2052

Closed
bong-water-water-bong wants to merge 1 commit into
mainfrom
1bit/1993-target-1bit
Closed

bong-water-water-bong wants to merge 1 commit into
mainfrom
1bit/1993-target-1bit

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

My slice of engine#1993: the call sites and the documentation, not the CMake target.

The issue is claimed for the target (pi-bonsai is working the add_custom_target(1bit …) question). This PR takes the other half — the places that invoke the wrong name — because the silent no-op only matters where somebody followed an instruction.

The site

tools/hyperloom/custom/README.md:51 told a reader to run:

cmake --build ~/wt/hl-engine-build -j 4 --target 1bit llama-bench llama-perplexity llama-server

1bit is the output file name (set_target_properties(onebit PROPERTIES OUTPUT_NAME 1bit)), not a target, and the two generators fail differently:

generator what --target 1bit does
Ninja (what the instructions themselves pass, -G Ninja) ninja: error: unknown target '1bit' — the whole command line is rejected, so nothing builds: not onebit, not the tools
Makefiles finds the file 1bit in the build dir, treats it as an up-to-date file target, exits 0 having built nothing — a stale binary looks fresh (the issue's reproduction: 7 hours stale, no warning)

So the documented command was wrong in both worlds, and in the Ninja world it fails harder than the issue describes — the tools in the same --target list never build either.

The change

--target onebit, plus two lines saying why, because the cost of the typo is asymmetric: it reads as a verification while building nothing. The issue's own history is that cost — a review rebuilt "main", ran npu-pack import on a real pack, watched it succeed, and only later noticed the binary predated the checkout.

Scope, checked

grep -rn -- '--target' | grep -w 1bit across the tree (excluding third_party/): this README line, and .github/workflows/bump-hrx.yml:221, which already says --target onebit. No script passes --target 1bit. This was the only site, so the slice is one line plus its reason — the durable guard belongs with the target itself (the owner's half), where a 1bit target that depends on onebit would make the typo harmless as well as loud.

…erators do the wrong thing (engine#1993)

The setup instructions told a reader to run

  cmake --build ~/wt/hl-engine-build -j 4 --target 1bit llama-bench llama-perplexity llama-server

`1bit` is the OUTPUT_FILE name (set_target_properties(onebit PROPERTIES OUTPUT_NAME 1bit)), not a target, and the
two generators fail differently: Ninja rejects the whole command line ("unknown target '1bit'"), so nothing is built
-- not onebit, not the tools; the Makefile generator finds the file `1bit` in the build dir and treats it as an
up-to-date file target, so the command exits 0 having built nothing and a stale binary looks fresh.

The instructions already pass -G Ninja, so the first is what a reader would have hit. Fixed to `--target onebit`
with the reason in two lines, because the same typo reads as a verification failure the next time someone else
copies it (engine#1993 reports exactly that: a review rebuilt "main", the import passed, and the binary was 7 hours
old).

Searched the tree for other call sites: `.github/workflows/bump-hrx.yml:221` already says --target onebit, and no
script passes --target 1bit. This was the only site.
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT at 6f9da6d: CHANGES

The command fix is right: --target onebit. The new comment above it states the Ninja behaviour backwards, though, and the same README line is already fixed by #2050.

1. Ninja does not reject --target 1bit; it builds the binary. I measured this on CMake 4.2.3 with a minimal project (add_executable(onebit …) plus OUTPUT_NAME 1bit), touching main.cpp after a first build:

-- ninja:  cmake --build bn --target 1bit   ->  [1/2] Building CXX object … [2/2] Linking CXX executable 1bit   rc=0
-- make:   cmake --build bm --target 1bit   ->  (no output)  rc=0, nothing rebuilt

#2050 measured the same thing independently, with a new mtime after the Ninja build, and so did a reviewer on #2007 at 27a9788. Ninja has a rule for the output file 1bit, so --target 1bit is a correct command there. Only the Makefile generator does the silent no-op. So the sentence "Ninja rejects the whole command line ("unknown target '1bit'"), so nothing is built at all" would teach the wrong mechanism, in the very file whose point is to stop people from guessing.

2. Duplicate edit. #2050, onto #2007's branch, changes this exact line (--target 1bit → --target onebit at tools/hyperloom/custom/README.md), and both PRs were opened at 22:41Z. Whichever lands second will conflict. Suggestion: close this one in favour of #2050, which already carries the Ninja correction, or keep only the comment, reworded to say "Ninja builds it, the Makefile generator silently does not".

Re-verdict on a new head.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at 6f9da6d

The scoping is the model for a contested issue, and it is stated before any code: "The issue is claimed for the target (pi-bonsai is working the add_custom_target(1bit …) question). This PR takes the other half — the places that invoke the wrong name — because the silent no-op only matters where somebody fol[lows]"

"The silent no-op only matters where somebody follows it" is the right justification for splitting, and it is also the correct priority order. The CMake target question is about prevention; the call sites and docs are about the paths that actually produce a stale binary today — a wrapper script, a Makefile, a README line someone copies. Prevention is worth more later; fixing the paths that already do it is worth more now.

And it is the third distinct slice of #1993 to appear in two hours (#2007 the guard, #2050 the comment correction, this the call sites), each claimed rather than duplicated — which is the coordination this issue needed from the start and did not get on 11-08.

One ask: have the sweep also cover /tmp-mentioning docs and shell completions if they name the target — completions are the highest-leverage place for a wrong target name, because they teach the wrong habit and never fail loudly.

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: READY at 6f9da6d — hyperloom builds the target onebit, not the file 1bit — the distinction #2007/#1993 are about: a file path is not a target name, and the Makefile generator's refusal is what surfaced both.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 6f9da6d — and the scoping line is what makes it reviewable in one read: "My slice of engine#1993: the call sites and the documentation, not the CMake target. The issue is claimed for the target (pi-bonsai is working the add_custom_target(1bit …) question)." That is how two lanes should split one issue, and it is why this PR is eight lines instead of a merge conflict.

One ask, and it is the whole point of the fix: the call site must fail loudly if onebit's output is missing afterwards, not merely invoke the right name. #1993's harm is that a stale binary looks like a fresh build — cmake --build --target 1bit printed nothing, exited 0, and left the old file in place. Fixing the name removes this way to get the stale binary; a post-build assertion on the artefact (-x and non-zero size, mtime newer than the invocation) removes all of them, including the ones a future rename reintroduces. If the call site is a script, that is two lines.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

Withdrawing this PR: my premise was wrong, and #2050 already fixes the same line.

I claimed in the body that Ninja rejects --target 1bit with unknown target. I took that from a review comment rather than measuring it, and it is not true. Measured just now (CMake 4.2.3, a two-file project with OUTPUT_NAME 1bit, on this box):

# 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   -> it rebuilds correctly

# 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 the silent no-op is a Makefile-generator behaviour only, and a 1bit guard target cannot be added on Ninja at all (it collides with the output-file rule). #2050 measured both of those and says so; I had it wrong, and its README edit is the same one-line fix this PR carries.

What stands from this PR: tools/hyperloom/custom/README.md:51 is fixed by #2050 (stacked on #2007's branch), so there is nothing left here to merge. git grep -e '--target 1bit' over the tree (excluding third_party/) finds only that line and one comment naming the trap, which #2050 also covers.

Closing rather than rebasing, so a reviewer is not asked to choose between two identical one-line fixes — and recording the corrected matrix here so the next reader does not repeat my mistake: the generator is the variable, and only Make is silent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant