Repository navigation
fix(builder): stop when damaged resources have no replacement - #2096
Conversation
c6cfa4a to
b250206
Compare
|
Rebased onto main |
e40ea63 to
ab08c76
Compare
|
The earlier AppImage run stopped before compilation on a Launchpad HTTP504 signing-key request. Update: superseded by the rebase onto main |
ab08c76 to
085b166
Compare
ghost
left a comment
There was a problem hiding this comment.
Looks right to me. Stopping on a damaged file with no stand-in is the right call for #2005 (77 all-zero files, the game can't reach the first frame), and writing the marker only after repair succeeds fixes the "checked once, skipped forever" hole. The tests cover fresh, cached, update, mixed and legacy-marker paths through the real entry point.
On false positives, I checked the case main lets through on purpose: the common 23.3 GB dump. Its three damaged textures all get a stand-in (#1308 lists the picks), so unresolved is 0 there and those builds go through as before. What gets blocked now is only a dump where some damaged file has no whole sibling of the same kind. Before this, main left those files as zeros and built anyway. There's no override for someone whose unresolved file is never loaded, but given #2005 I think stopping is the better default.
Costs and nits, none blocking:
- Every existing install with a legacy
.hookshot_romfs_checkedrescans the whole romfs once on its next build or update (about 100k files opened, minutes with a virus scanner). That's unavoidable with this design, but it's worth one line in the description so nobody reads it as a regression. - The worklist row scores reach 1.0 as an "upper bound". The 23.3 GB dumps aren't affected (all resolved), so the real reach is the rare heavily damaged dump, and 5.0 puts it at the top of the list. A lower estimate would sort it more honestly.
- Overlap with #2254: both rewrite the same print block in
stand_in_damaged_files, so whichever lands second will conflict (bronze-ff has said they'll rebase). Semantically, once this is in, #2254 should writehookshot_damaged_files.txtbefore thefail(). Otherwise the unresolved case, the one most worth leaving a note for, never gets the file. Its premise that "a later build skips this check" also only holds for resolved dumps after this PR.
CI: everything passes except the AppImage job, which was cancelled at "System packages" after the 1 h timeout. That's infra, not this change, and main's AppImage runs at 49f75dc and 32693f0 fail too. Good to merge from my side.
| if f.read() == stamp: | ||
| if f.read() == checked_stamp: | ||
| return | ||
| os.remove(checked) # Earlier checks also marked files with no replacement as checked. |
There was a problem hiding this comment.
This makes every existing install do the full scan once on its next build or update, since nothing in the old marker tells resolved from unresolved. That's fine, but please note it in the PR description (the progress line does show it at least).
| else: # (found while the .nup was extracted: no file opened again) | ||
| done = nup.stand_in_damaged(romfs, damaged=[os.path.normpath(os.path.join(romfs, rel)) for rel in known]) | ||
| if done: | ||
| print("Your game dump is incomplete: %d game file%s in it %s damaged. Similar files are used where available, " |
There was a problem hiding this comment.
Nit: the old text gave the sizes (complete about 23.8 GB, incomplete 23.3 GB). That's the quickest way for a player to tell which dump they have, and triage leans on it. Worth keeping.
| (len(done), "" if len(done) == 1 else "s", "is" if len(done) == 1 else "are"), flush=True) | ||
| for path, other in done: | ||
| print(" %s: %s" % (os.path.relpath(path, romfs), | ||
| "in its place " + os.path.basename(other) if other else "nothing similar found, left as it is"), |
There was a problem hiding this comment.
Nit: when the build then stops, "nothing similar found, left as it is" reads as if the build carried on with it. Something like "no replacement found" would match the error below.
|
|
||
| | Score | Problem | S | Reach | Conf. | Owner | Links | | ||
| |---:|---|---|---:|---:|---|---| | ||
| | 5.0 (upper bound) | Builder continues with damaged resources that have no stand-in, then records a successful check and silently skips it on retry; stop before lift and recheck old markers. Reach is the all-platform upper bound, with the damaged-dump subset unknown | S1 | 1.0 | 1.0 | Arkex-ai | #2005; [claim](https://discord.com/channels/1568810484818378804/1569019786766188665/1569897037862805726); authored entry-point reproduction on Windows and Ubuntu | |
There was a problem hiding this comment.
Nit: 23.3 GB dumps resolve all three files, so reach 1.0 overstates this a lot. I'd score it on the heavily damaged dump subset (#2005-like) so it doesn't sit at the top of the list.
085b166 to
35415b0
Compare
|
Thanks for the review. Rebased at For #2254, agreed: the damaged-file report should be written before this failure, including unresolved files. Fresh CI is starting; earlier results remain labeled in the description. |
ghost
left a comment
There was a problem hiding this comment.
Re-checked at 35415b0 (rebased; the code change since my approval is the #2253 merge plus the Unicode-stamp test, and the resolved-only marker still reads right: only a fully resolved check writes resolved-v1, and a legacy marker is dropped and rescanned once).
Fixed: the one-time full rescan is now in the description; the worklist reach is lowered to a provisional 1%.
Still open, all nits: the dump-size hint is still gone from the message, and "left as it is" is still there (inline). The new worklist row sits after a blank line, so it falls outside the table and won't render as a row; "provisional1percent" and "common23.3GB" are also missing spaces.
The #2254 ordering (report written before fail()) is agreed in the thread, and this print block already comes before it. CI: AppImage failed on apt mirror Ign errors during system packages (not the change); the rest was still running when I looked.
| done = nup.stand_in_damaged(romfs, damaged=[os.path.normpath(os.path.join(romfs, rel)) for rel in known]) | ||
| write_stamp(checked, stamp) | ||
| if done: | ||
| print("Your game dump is incomplete: %d game file%s in it %s damaged. Similar files are used where available, " |
There was a problem hiding this comment.
Nit (from before): the sizes (complete about 23.8 GB, incomplete 23.3 GB) are still gone. They are the quickest way for a player to tell which dump they have.
| (len(done), "" if len(done) == 1 else "s", "is" if len(done) == 1 else "are"), flush=True) | ||
| for path, other in done: | ||
| print(" %s: %s" % (os.path.relpath(path, romfs), | ||
| "in its place " + os.path.basename(other) if other else "nothing similar found, left as it is"), |
There was a problem hiding this comment.
Nit (from before): "left as it is" still reads as if the build carries on with it; "no replacement found" would match the error below.
| | 0.11 | Intel GPU: black screen after loading a save (or on the Deku Tree), sound only, very low fps; three reports, no logs yet; tracked in the GitHub issue linked in #bug-reporting replies (#999) | S0 | 0.035 | 0.4 | | #bug-reporting "Intel G3E. Title and intro loads...", "intel arc b580 problems", "game gets stuck on a black screen when loading a save (intel gpu... gulp...)" | | ||
| | 0.11 | Crash at the door after the owl intro (Navi wakes Link, 2.0.13): access violation in `igvk64.dll` at start-up; GPU: Intel UHD Graphics (i3-1115G4, 128 MB shared, no mesh shaders: the best-effort fallback, #1001); reach is Intel's 3.5% | S0 | 0.035 | 0.4 | | #bug-reporting "2.0.13 Crash after you see the owl during intro cutscene" (5 logs) | | ||
|
|
||
| | 0.05 (provisional) | Builder continues with damaged resources that have no stand-in, then records a successful check and silently skips it on retry; stop before lift and recheck old markers. Reach is a provisional1percent planning estimate for rare severely damaged dumps, not measured prevalence; the common23.3GB dump with all replacements is unaffected | S1 | 0.01 | 1.0 | Arkex-ai | #2005; [claim](https://discord.com/channels/1568810484818378804/1569019786766188665/1569897037862805726); authored entry-point reproduction on Windows and Ubuntu | |
There was a problem hiding this comment.
Nit: the blank line above this row ends the table, so the row won't render as part of it. Move it up next to the other rows (and keep the score column numeric, plus the missing spaces in "provisional1percent" / "common23.3GB").
Recheck legacy repair markers, preserve unresolved damage lists for retries, and publish a successful marker only after all detected damage has a counterpart. Assisted-by: GPT-6 (OpenAI)
Lower provisional reach to reflect the rare unresolved-dump subset. Assisted-by: OpenAI Codex
Exercise the integration with merged2254 in both cached rebuild and update paths. Assisted-by: OpenAI Codex
Address review wording and render the provisional reach inside the worklist table. Assisted-by: OpenAI Codex
Keep the persistent-report assertion aligned with the reviewer-requested wording. Assisted-by: OpenAI Codex
35415b0 to
c215f01
Compare
|
Updated at c215f01 on main 6df80a8. The merged #2254 report is now written before the unresolved failure, with a regression covering cached rebuild and update paths. The review nits are addressed too: dump-size guidance, clear missing-replacement wording and the worklist table/spacing. Focused tests pass on Windows and Linux; earlier full-suite results remain labeled by SHA in the description. Fresh CI is starting. #2391 is coordinating the subsequent resolved-v2 stamp for wrapped-file detection. |
Summary
The Builder could compile damaged files with no replacement, then mark the check successful and skip the damage on retry. It now stops before lift, leaves a persistent list of the affected files, and retries the check until the dump is repaired. Addresses the unresolved-resource part of #2005.
Technical changes
resolved-v1only after every detected damaged file has a replacement. Preserve unresolved extraction heads for retries; recheck older success markers once.Validation
Tested head
c215f012, rebased onto main6df80a8d. Focused cache-entry suite passes on Windows and Linux (17 methods; Windows skips one link-privilege control), plus eight NUP head/stamp tests and the stand-in checks on both systems. The cached rebuild/update regression verifies the unresolved report exists before the failure, while the successful marker remains absent and retry heads remain intact. Hosted checks will run on this head.Existing installations rescan their romfs once when replacing a legacy checked marker: about 100,000 file opens, potentially minutes. The common 23.3 GB dump remains accepted when all damaged files have replacements. The worklist's 1% reach is a provisional planning estimate, not measured prevalence.
Earlier full-suite evidence and limits
At
1f96189bon exact base7759a6f3, full Linux candidate and baseline each passed 373 / 0 failed / 23 skipped, using the same filesystem and toolchain. Full Windows candidate passed 363 / 2 failed / 31 skipped, with the same two failures as its exact base: the recorder destructor negative control and NUP link privileges. This is earlier-head evidence; the subsequent rebase and diagnostic wording changes have focused validation above, with fresh hosted checks pending.These are authored, game-free fixtures. No complete real-dump build or gameplay validation is claimed for this pre-build gate. Donor integrity is handled separately by #2019, already merged. #2391's author is coordinating a later combined
resolved-v2marker so wrapped-file detection still invalidates older checks.Dev-work claim.