Repository navigation
fix(tools): report release BadZipFile errors with asset names - #10576
Conversation
|
Could a maintainer please start the full Linux/Windows CI build for this PR via Actions → Build → Run workflow on Commit Thanks! AI-assisted: yes (Codex). |
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed exact head 7212aea7f83595153b92e5a85163fcf720cf773d. Adding zipfile.BadZipFile to the CLI error handler converts malformed converter/library ZIP inputs into the documented argparse exit path; regression coverage verifies exit code 2, no traceback, preservation of the prior output, and staging cleanup. python3 -m unittest tools.tests.test_web_release passed (29 tests), and git diff --check upstream/main...HEAD is clean. Hosted conventions, intake, and labels pass.
roboterspinne
left a comment
There was a problem hiding this comment.
Reviewed head 7212aea merged onto upstream/main dca72a8 (2026-10-24); the merge is clean, also together with #10624 (same two files, disjoint hunks), check_conventions.py passes and pr_overlap.py reports no overlap.
Environment: Windows 11 Home 10.0.26200, Python 3.13.9 (Anaconda); Python only, no build needed. The hosted Build run for this head is action_required with no jobs (fork PR from a first-time contributor), and the eligibility job (build.yml:238-266) enables the relinker jobs only for authors with a commit or merged pull request in main, so even an approved run would skip them; the web_release and web_library_assets ctests have not run anywhere but on the author's Mac and here.
Results
python -m unittest tools.tests.test_web_release tools.tests.test_build_webon the merged tree: 39 of 39 pass in 1.6 s (29 intest_web_release, 10 intest_build_web, as the body counts). Red/green: the new test againstmain'sbuild_web.pyerrors in all three subcases with an uncaughtzipfile.BadZipFile: File is not a zip file; with the fix it passes.- The truncation by 22 bytes removes exactly the end-of-central-directory record of the fixture archives,
set_assetkeeps size and digest consistent, so the earlier size and checksum checks pass and the failure really comes fromzipfile. The previous site and the absence of.web-release-*staging are asserted. - I ran
build_web.mainwith the same fixtures on archives whose structure is valid but whose content is not (requestmocked as in the test). The previous site stayed unchanged and no staging directory was left in every case, so only the exit path differs:
| release asset | zipfile raises |
with this PR |
|---|---|---|
| EOCD record removed (the PR's test) | BadZipFile |
error: File is not a zip file, exit 2 |
web-v1.zip, deflate stream of index.html corrupted |
zlib.error from archive.read (build_web.py:143) |
traceback, exit 1 |
prx-linux-v1.zip, member compression method 99 |
NotImplementedError from testzip (:87) |
traceback, exit 1 |
web-v1.zip or prx-linux-v1.zip, member flagged encrypted |
RuntimeError: File ... is encrypted, password required from archive.read / testzip |
traceback, exit 1 |
| library member that inflates but fails its CRC | testzip returns the name |
error: Corrupt release entry: libs/libkernel.prx, exit 2 (unchanged) |
Nits
build_web.py:188: the title promises CLI errors for invalid release archives, but the tuple gains onlyzipfile.BadZipFile, which covers a missing or broken end-of-central-directory record. Archives that are invalid in other ways still leavemain()with a traceback and exit 1, as the table shows: a corrupt deflate stream raiseszlib.errorfromarchive.readortestzip, an unsupported compression methodNotImplementedError, an encrypted memberRuntimeError. Nothing regresses (same onmain) and the site stays intact, so this is incomplete coverage rather than a bug: either catchzlib.errortoo and reject the other member errors asValueErrorlayout checks (compress_typenot stored or deflated,flag_bits & 0x41), with a subcase each, or narrow the title and What to truncated or non-ZIP archives.- Same line:
str(error)printserror: File is not a zip filefor all three assets alike, so the Pages log does not say whetherweb-v1.zip,prx-windows-v1.ziporprx-linux-v1.zipis broken. Catching at the twozipfile.ZipFile(...)calls (:68,:128) and re-raising asValueError(f"Invalid release archive {name}: {error}") from errorwould name the asset and let the subcases tell the three apart. Several existing library messages omit the platform too, so optional polish.
Notes, nothing to change here: in the Pages job the archive is checked for size and SHA-256 against GitHub's asset metadata before zipfile sees it, and a fresh checkout has no previous site, so the change is a diagnostic and exit-code improvement, not a correctness one. The template's Tested HTML comment sits below the author's bullets instead of under the heading; every line is kept, the order is not. The assertNotIn("Traceback") line only guards against a future print_exc before parser.error, since an uncaught exception already errors the subtest earlier; consistent with the file's other tests.
Not checked: macOS and Linux runs, Python 3.9 (the body's interpreter; BadZipFile and the message text are unchanged there), a live release download.
I found no blocking issue: the added catch is correct and its test exercises exactly the EOCD case with consistent size and checksum metadata, nothing else changes behaviour, and the counter-check holds. The two nits are about how much of the title the change covers and about the message. For disclosure: I have open relinker PRs (#10658, #10646, #10607, #10596, #10564, #10403, #10402, #10401, #10391, #10390); none touches these files.
Written with AI assistance (Claude Code). I ran the tests and the archive probes above on my Windows 11 machine.
| print(output) | ||
| return 0 | ||
| except (OSError, ValueError) as error: | ||
| except (OSError, ValueError, zipfile.BadZipFile) as error: |
There was a problem hiding this comment.
Nit: this covers the missing or broken end-of-central-directory record, but other invalid release archives still leave main() with a traceback and exit 1 (reproduced with the test fixtures through build_web.main): a corrupt deflate stream raises zlib.error from archive.read (:143) or testzip (:87), an unsupported compression method NotImplementedError, an encrypted member RuntimeError. Same on main, and the previous site and staging cleanup hold in every case, so not a regression. Either catch zlib.error here and reject the other member errors as ValueError layout checks (compress_type not stored or deflated, flag_bits & 0x41), with a subcase each, or narrow the title and What to truncated or non-ZIP archives. Also, str(error) is File is not a zip file for all three assets alike; re-raising at the two zipfile.ZipFile(...) calls (:68, :128) with the asset name would tell the publisher which one is broken.
7212aea to
c329424
Compare
|
Re-checked c329424 merged onto Written with AI assistance (Claude Code). I ran the tests and probes above on my Windows 11 machine. |
MarcZX10
left a comment
There was a problem hiding this comment.
Re-reviewed exact head c32942496d945a9558b3f3fce10be6193dcba8fe. The new handling converts BadZipFile into an asset-named CLI error in both archive paths, and the regression verifies exit code 2, no traceback, and preservation of the prior site and staging files. Local test_web_release and test_build_web pass (38 tests); diff-check and conventions pass. The author also reports the same 39-test suite passing on Windows.
What
tools/build_web.py --releasecurrently letszipfile.BadZipFileescape when a converter or library release asset is truncated or is not a ZIP file. Wrap that exception at each release-asset boundary with the archive name, then route it through the existing CLI error handler, producing a diagnostic and exit code 2 instead of a traceback. For example:Invalid release archive prx-linux-v1.zip: File is not a zip file.This change handles
zipfile.BadZipFile; it does not add handling forzlib.error, unsupported compression methods, or encrypted members.The regression test supplies truncated converter, Windows-library and Linux-library archives with matching size/checksum metadata. Each case asserts its exact archive name in the CLI diagnostic. It also verifies that the previous site remains unchanged and staging is removed. No dependencies or runtime changes.
Tested
macOS 27.0.1, Apple Silicon arm64, Python 3.9.6, CMake 4.4.4. Updated head
c32942496d945a9558b3f3fce10be6193dcba8fe, rebased ontomainat137a8fab8984506ec9359dbfc2651bc367e0d381.BadZipFile. The added archive-name assertions also fail against the previous PR head in all three cases.python3 -m unittest tools.tests.test_web_release tools.tests.test_build_webpasses, 39/39 tests, after the rebase.ctest --test-dir build-relinker -R '^(web_release|web_library_assets)$' --output-on-failure: both suites pass, 39 Python tests total (29 + 10).python3 tools/check_conventions.py --base origin/main: passed, including the PR title/body check.git diff --check: passed.python3 tools/pr_overlap.py --branch fix/web-release-invalid-zipfound no overlap; the archive-name update stays within the same two files and adds no exports.HTTP responses are mocked; no live release download or publication was performed. No author-run Windows/Linux validation, full C++ build, GPU or game testing.
Independent contributor evidence for the previous head (before the archive-name update): roboterspinne reports 39/39 Python tests passing on Windows 11 / Python 3.13.9 with head
7212aea7f83595153b92e5a85163fcf720cf773dmerged onto upstreamdca72a82da. This is contributor-run verification, not hosted CI, and does not validate the updated head.Independent contributor verification of the updated head: roboterspinne rechecked c32942496d merged onto main
137a8fab89on Windows 11 / Python 3.13.9. All 39 tests passed, including the correct asset name, exit 2, previous-site preservation and staging cleanup for all three truncated assets. This is contributor-run evidence, not hosted CI.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)