Repository navigation
fix(build): the lift key is taken again only if the lift's other inputs did not change - #2686
Open
jordanella wants to merge 2 commits into
Open
jordanella wants to merge 2 commits into
jordanella wants to merge 2 commits into
Conversation
Contributor
|
The run you asked for on #2674, of the fixture with its two commit ids set to current main and this head (Linux, Python 3.12, no game, no compiler):
So the window is closed and #2674's gain is kept. Your own two test files pass here on this head ( One case neither version nor the code before #2674 sees, for the record and not for this pull request: a file changed and changed back during one lift (a branch switched and switched back while it runs) compares equal before and after. Not tested: Windows, and a real lift (the fixture stands in for the lift and the split; everything around them is the repository's code). |
SimonKimDev
reviewed
Nov 12, 2026
SimonKimDev
left a comment
SimonKimDev
left a comment
Contributor
There was a problem hiding this comment.
Small window left in the check: the full key is taken after the comparison, so a translator file edited between the second lift_key(..., planning=False) and lift_key(exefs, work, env) still stamps C from the old translator with the new key.
Taking the full key first closes it:
after = lift_key(exefs, work, env)
if lift_key(exefs, work, env, planning=False) == inputs:
key = afterAn edit before after is caught by the comparison; an edit after it doesn't matter, since after already hashed the old file.
Contributor
Author
|
@SimonKimDev right, thanks: the order left exactly that gap. 54e2652 takes the full key first and keeps it only if the @mooki000 thank you for running the fixture on both sides; that's the confirmation I couldn't get here. Noted on a file changed and changed back within one lift (a branch switched and back): no snapshot of hashes sees that. It's out of this PR, as you say. |
…nputs did not change NotRexed#2674 stores the key taken after the lift, so that the files the lift writes for the next one (image/hot_pieces.txt, image/chunks.txt) are in it. Taken whole, that key also covered a translator file edited while the lift ran, and certified C made from the old file under the new file's key: the next build skipped a lift it needed (mooki000's fixture on NotRexed#2674). translate() now takes lift_key(planning=False), everything but those two files, as the lift starts and again after it, and takes the key again only if that is unchanged. Otherwise the key from before stays, and the next build translates again, as before NotRexed#2674. tests/test_lift_cleanup_error.py adds the case of an input changed during the lift, and tests/test_lift_cache.py checks the order and what planning=False leaves out and keeps. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Contributor
|
The same fixture on |
hckrman101
pushed a commit
that referenced
this pull request
Nov 12, 2026
) **What this is for:** contributors rebuilding from source should not spend another minute drawing the same button prompts. **What is measured:** on an Intel Core i9-13900K running Arch Linux, the finish stage at main `c544828f` took 67.707 and 66.898 seconds; two unchanged runs took 0.603 and 0.651 seconds, and the final head took 0.616 seconds, clearing the run-time files as well. Other work was running throughout. **Not measured:** Windows, a packaged Builder update, a complete compile/lift, or gameplay performance. Refs #2636. ## Problem `nx2/make_game.py` removes the game folder's `prompts` directory and runs `prompt_assets.py` on every build when fontTools is available. Most source edits do not change anything those assets depend on. This avoids drawing unchanged assets; it does not make the drawing itself faster. #2636's A1 plans to measure the finish stage, but its A1–A10 items do not already propose this prompt cache. It complements #2674 and #2675, which address unnecessary retranslation rather than prompt generation. ## What changed - **`nx2/make_game.py`:** source-checkout builds request incremental prompt generation. Packaged builders still regenerate every time, using the finish stage's existing launcher distinction. `RX_RELEASE` cannot distinguish them: make_game sets it for source builds too. Missing fontTools still removes the replacement prompts, keeping the existing fallback to the game's icons. - **`nx2/prompt_assets.py`:** fingerprint the inputs before generating, draw into a fresh sibling directory, and record the successful outputs only if a second fingerprint still matches. Reuse requires both matching inputs and every recorded output's size and SHA-256. **A failed generation still removes the prompts, as before**, so the game shows its own icons. An interrupted builder can leave the previous complete set without a stamp; the next build regenerates it. The drawing, encoding and asset formats are unchanged. - **`tests/test_prompt_skip.py`:** game-free fixtures run the actual finish-stage prompt call and production cache/publication functions, with small authored files and a stand-in for expensive drawing. - **`docs/development.md`:** explain reuse, its limits, and `HOOKSHOT_FORCE_PROMPTS=1`. The force switch follows `env_on`: unset, empty, `0`, `false`, `off` and `no` are off, ignoring case and surrounding spaces. ### What prevents a wrong skip The input record covers: - The four prompt source modules, including controller, keyboard and binding tables. - The game's icon font and optional bold label font, compression dictionary, common texture/layout archives, and matching resource-size table names and contents. - The repository's controller and keyboard fonts, and added, changed or removed PNG overrides. - Python/platform; fontTools, Pillow, ASTC encoder, texture decoder, NumPy, zstandard and archspec availability and versions; Pillow's native library versions and the selected ASTC backend. - Generation parameters and relevant library switches, including `SOURCE_DATE_EPOCH`, `FREETYPE_PROPERTIES` and the encoder's custom CPU definitions. Languages and saved bindings are not build inputs here: the generator uses its source tables, and the runtime applies the selected controls. The output record includes files **and directories**: the keyboard directory itself is a runtime marker. Legitimately absent optional outputs stay absent; a missing, truncated or same-size-corrupted recorded file forces regeneration. The runtime's own compressed copies and control-specific keys are excluded before hashing, not mistaken for generator output. **The files the game derives at run time are still cleared by every build**, including an unchanged hit: their own keys do not cover changes to `host/prompts.c`. Only the known runtime filenames are removed; if one cannot be cleared, the build regenerates instead of keeping on doubt. An unreadable input cannot authorize reuse. The log gives the reason, even if a forced regeneration subsequently fails. A source edited during generation leaves no stamp, addressing the stale-input certification problem discussed in [#2674's review](#2674 (comment)). ## How it was tested 2026-11-12; Arch Linux, kernel `7.2.15-1-omarchy`, x86-64, Intel Core i9-13900K, 32 logical CPUs, Python 3.12.16. Base `c544828ff`; final head `2ec89de8`. No game or GPU program was launched for these checks. ### Finish-stage measurements These execute the real finish-stage statements through completion, including binary/image copies, the unchanged shader-list check, pipeline-list work and prompt generation, in a disposable copy of an existing built game folder. Compilation, translation and shader preparation are not included. They are **not whole rebuild times**. All optional prompt packages were present: fontTools 4.68.0, Pillow 13.0.0, astc-encoder-py 0.1.12, texture2ddecoder 1.0.6, archspec 0.2.7, NumPy 2.5.4 and zstandard 0.25.0. | Run | Source | Finish time | 1-minute load before → after | Top other work | |---|---|---:|---:|---| | Before 1 | `c544828f` | 67.707 s | 7.13 → 7.40 | Existing game 316 → 315% CPU; file indexing 66 → 79%; relay check also active | | Before 2 | `c544828f` | 66.898 s | 7.06 → 8.24 | Existing game 315 → 318%; terminal process about 41% | | First generation with cache | `63d5c965` | 66.400 s | 8.24 → 8.19 | Existing game 318 → 320%; terminal process about 42% | | Unchanged 1 | `63d5c965` | 0.603 s | 8.19 → 8.19 | Existing game 320%; terminal process about 42% | | Unchanged 2 | `63d5c965` | 0.651 s | 8.19 → 8.19 | Existing game 320%; terminal process about 42% | | Changed table source | `681208dd` | 63.147 s | 6.99 → 6.95 | Existing game 320 → 321%; terminal process about 42% | | Regenerate, final head | `2ec89de8` | 65.007 s | 5.77 → 5.86 | Existing game 317 → 316%; terminal process about 43% | | Unchanged, final head, with run-time files present | `2ec89de8` | 0.616 s | 5.79 → 5.79 | Existing game 316%; terminal process about 43% | Other work was running; none was a quiet measurement. CPU percentages are lifetime averages from snapshots, not interval measurements. The first generation is included to show that this is a skip, not a faster drawing algorithm. The unchanged runs print: ```text prompts: unchanged, kept ``` Adding a harmless comment to the button-table source forces a fresh generation: ```text prompts: regenerated (table changed) ``` The comment was removed afterwards. A timestamp-only touch does not regenerate: the key checks content. An additional run on the final head verifies an unchanged hit with all six runtime-derived files present: all six are removed, while the 16 generated originals and the stamp retain their sizes and SHA-256 values. The focused fixture also compares the retained originals byte for byte. The final timing rows above include that cleanup. **Asset parity:** all 16 regenerated files, 274,704,680 bytes, compare byte for byte with a full generation by main `c544828f`. Only the new stamp is excluded. Both sides used the same `SOURCE_DATE_EPOCH` because fontTools otherwise writes the current time into the OpenType header. This comparison setting is not introduced into normal builds. A development cache hit retains the earlier font timestamp; packaged builds retain their old full-generation behavior. ### Game-free checks - **23 focused tests pass** on the final change. They cover unchanged reuse, every input kind, package versions/presence, switch/parameter changes, missing/truncated/same-size-corrupted output, empty directories, clearing runtime derivatives while retaining byte-identical originals, regeneration when runtime cleanup fails, interruption, mid-generation edits, unreadable input, force-switch parsing, incomplete output, publication failure, and the packaged/no-fontTools paths. A non-zero generator result removes prompts for both source and packaged builds; interruption before that result leaves the old set unstamped. - The same unchanged-build test against main's finish stage fails behaviorally: `2 != 1: unchanged finish must not draw the prompts twice`. - Removing each of **18 guards/key components** in memory makes its corresponding behavior test fail on the final change. Three later corrections also fail against the earlier version of this patch: returned failure retained old prompts in both build paths, runtime files survived a hit, and an uncleared runtime file did not force regeneration. The unreadable-input diagnostic has a separate before-fails/after-passes proof. - The whole suite one commit earlier, at `f20ea58b` (before a failed generation was made to remove the prompts again and the run-time files to be cleared), `python -I -B tests/run_all.py --jobs 8`: ```text 472 passed, 0 failed, 20 skipped in 133 s (linux, 8 at once) ``` Load was 7.37 → 19.48. The first full run, at 32 checks at once, was **471 passed, 1 failed, 20 skipped**: a clang++ child compiling `texture_pass.cpp` for `nx2/test_depth_sample_types.py` died with SIGKILL. That fixture and its input trees are unchanged from main. No kernel OOM line was found, so the cause is not established. The complete run with eight workers passes, including that check. The failed run is retained, not counted as a pass. On the final head `2ec89de8` the focused checks, the baseline failure and all guard mutations were rerun, and the whole suite once more with eight workers: **471 passed, 1 failed, 20 skipped in 136 s** (load 11.14 → 22.62). The one failure is `tests/test_release_scripts.py`, `test_nightly_two_runs_at_once_end_at_the_newer_commit`, the intermittent race reported in #2626; this change touches none of its files. - No core dumps were recorded. Nothing generated from the game is in this change. ## Not tested - Windows filesystem behavior or a Windows build. - The packaged Windows Builder or Linux AppImage build/update end to end. Their always-regenerate selection is covered by a game-free test only. - A full compile/lift or a new gameplay run, other CPUs, or gameplay/frame-rate benefits. No screenshots: the regenerated asset bytes match, and no visual change is intended. ## Risks The input list must grow when the generator gains new file dependencies or outputs outside the prompt directory. A regression flags new local module imports, but it does not replace reviewing new file reads. Generation temporarily needs room for both old and new assets. Publication uses completed directories and rollback on a rename error; interruption cannot create a stamped half-set, though a process killed between the two directory renames may require regeneration on the next build. ## Overlaps The open-PR scan found 47 PRs touching one of these files. The actual conflict hunks were read: - **#2151:** adjacent item-button generation conflicts with the finish-stage prompt call/error branch. Preserve both steps and account for added prompt dependencies. - **#2261:** substantive overlap. It adds `--wheel-icons`, another input folder and output outside `prompts`. Keep that work outside this skip or extend the input/output proof and staging; accepting both hunks mechanically is not enough. - #1872, #1958, #2304, #2317, #2430, #2465, #2474, #2478, #2608, #2673, #2675 and #2686 share code files but merged cleanly in the scan. That is not proof that any newly added generator dependency is covered. Assisted-by: GPT-6 Astra Co-Authored-By: GPT-6 Astra <noreply@openai.com> Co-authored-by: GPT-6 Astra <noreply@openai.com>
The key was taken after lift_key(planning=False) was compared, so a translator file edited between the two would still stamp C from the old file with the new key (SimonKimDev on NotRexed#2686). It is now taken first and kept only when the comparison after it holds: an edit before it is caught by the comparison, and one after it is in no key that is kept. tests/test_lift_cache.py checks that order, and the cleanup test's changed-input case sees the key taken and not kept. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
dazemc
added a commit
to dazemc/Hookshot-rs
that referenced
this pull request
Nov 12, 2026
Upstream NotRexed/Hookshot fixed four builder problems the fork shares, on the path its Flutter builder and Rust engine use. Ported semantically, with the Rust engine's own layout and failure handling: - e7f5581: scripts/oot_saves.py finds Lichii's own data folder (~/.local/share/lichii, %APPDATA%\lichii) before Eden's, where a Lichii before build 150 still keeps its saves. `list` groups programs by the folder import and export use; the fork's `saves` list for the Flutter launcher follows the same rule, so Lichii is not offered from Eden's. - 6c529a2: a Btrfs folder has a device number no disk has, so the hard drive warning could not tell; nx2/disk.py and builder/rust/src/disk.rs now read /proc/self/mountinfo for the deepest mount and every disk of a multi-disk Btrfs (/sys/fs/btrfs/<id>/devices). - c544828 (+ NotRexed#2686, NotRexed#2675): the lift key is taken again after the lift and split_hot, from image/chunks.txt that the first lift writes, so a new work folder's second build no longer translates the game again; the new key is kept only when nothing else the lift reads changed while it ran. A compile that failed only in repository sources keeps the translation (a host typo no longer costs a lift); a lifted or generated file that failed still forgets it. The Rust driver never had a key but had no such distinction: compile failures now follow the same rule, a link failure forgets the lift, and a failed host source keeps it. - a740d67: host_mtune()/host::mtune() detect a Steam Deck APU (0405 or 0932) or RX_STEAMDECK=1 and add -mtune=znver2 beside -march, in nx2/build.py and the Rust driver's game and host compiles; RX_MTUNE names a tune. The fork's lift and host builds both get it. Tests: the upstream ones adapted (test_oot_saves, test_disk_warning, test_cpu_target, test_lift_cache, test_lift_cleanup_error), new tests/test_failed_compile_keeps_lift.py, and Rust tests for the Btrfs mount walk, the lift failure rule and the Zen 2 tune. No game files or generated game data. Assisted-by: opencode (DeepSeek V4.1 Flash 1017)
This was referenced Nov 12, 2026
Joey-Magnus
approved these changes
Nov 12, 2026
Joey-Magnus
left a comment
Joey-Magnus
left a comment
Contributor
There was a problem hiding this comment.
SimonKimDev's ordering point is fixed in 54e2652. after = lift_key(...) is now taken before the planning=False comparison at nx2/make_game.py:2190, and test_lift_cache.py checks that order. mooki000's fixture on the new head confirms both cases: an edit made during the lift is not certified, and a build where only the planning files changed still skips the lift. I merged this into current main. tests/test_lift_cleanup_error.py passes (9 tests, including the new "input changed during the lift" case). tests/test_lift_cache.py has the same single failure here as on main (numpy isn't installed for the lift import check), and all the new checks pass. The fallback is the conservative one: if anything changed during the lift, the pre-lift key stays and the next build lifts again. That costs a few minutes at worst, never a stale build. An edit reverted during the lift (A to B to A) can't be seen by a content hash, but the C would be from A anyway, so that's fine. It merges cleanly with #2693, which also edits make_game.py.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#2674 can certify stale lifted C: if a translator file is edited while a lift runs, the key taken after the lift describes the new file, but the C came from the old one, and the next build skips a lift it needs. mooki000 found and reproduced this on #2674 just before it merged; this closes it.
Change
lift_key(..., planning=False): everything the key hashes except the two files the lift writes for the next one (image/chunks.txt,image/hot_pieces.txt).translate()takes that as the lift starts and again after it, and takes the key again only if it's unchanged. If something else changed while the lift ran, the key from before the lift stays, and the next build translates again, as before fix(build): a new work folder's second build no longer translates the game again #2674.This is mooki000's suggested shape: refresh the self-produced inputs only after confirming the rest are unchanged.
Testing
tests/test_lift_cleanup_error.py: a new case where an input changes during the lift writes the pre-lift key, and the key taken after doesn't validate. Againstmain'smake_game.pyit fails; with this change it passes. The existing cases still see lift → key taken again → key written.tests/test_lift_cache.py:nx2/lift.pyruns, and the check comes before the key is taken again.planning=Falseignores changes tohot_pieces.txtandchunks.txt, but follows the game's code and the hot list.main'smake_game.py, passes with this change.make_game.py(52), on Windows, passes apart fromtest_cpp_preflight,test_linux_updateandtest_prepare_stall, which fail the same way on a cleanmainhere.guardedvariant is the same check this PR adds; a run with--variantpointed at this branch's commit would confirm it.🤖 Generated with Claude Code