Repository navigation
fix(tools): open the unwrap_self temporary file through io.open - #10586
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed the one-line change against the existing temp-file lifecycle. Direct io.open preserves the exclusive "xb" creation semantics while making the injected open failure seam effective on Python 3.10, whose pathlib Path.open implementation calls its filesystem accessor directly. I ran python3 tools/tests/test_unwrap_self.py at head 546bad013f7a7, and all 51 tests passed (1 skipped). Hosted conventions, intake, and labels checks also pass. The PR reports that full runtime/GPU and Windows coverage were not run; those are not needed for this tools-only change.
THEMANnt
left a comment
There was a problem hiding this comment.
Reviewed PR #10586 at head 546bad013f7a145261dbedc49a6169c49002b404 against pinned base fa515def85cda0b9f3b7a70764c2ed7da8167133. The change adds import io and opens the temporary output through io.open(temporary_path, "xb") in tools/unwrap_self.py, allowing the existing failure-injection hooks to apply on Python 3.10.
The same path and exclusive binary mode preserve collision protection. The surrounding write, close, replacement, and cleanup logic remains unchanged. I inspected tests for partial-write and close failures, creation failure, temporary-name collisions, successful replacement, and input aliases. They execute the real conversion using isolated temporary directories and retain assertions for output preservation and cleanup.
Controller-recorded local execution of tools/tests/test_unwrap_self.py on Windows AMD64 reports 51 tests, with 47 passing and four skipped, for baseline, pristine PR, and candidate on Python 3.11.9. Pristine PR and candidate also report those results on Python 3.10.22. Against baseline code on Python 3.10.22, the unchanged candidate suite produces the expected four assertion failures: close failure, creation failure, and two write-failure subtests returning 0 instead of 2. This establishes the interception regression; it does not demonstrate an ordinary unmocked data-loss bug. Local whitespace and repository-convention checks also passed.
GitHub-reported conventions, intake, and labels checks succeeded for the reviewed head. Independent local verification excludes the skipped POSIX permission/group and symlink cases; the author's Ubuntu results remain author-reported. No native build, GPU, or game validation is claimed. No actionable findings in this focused review.
roboterspinne
left a comment
There was a problem hiding this comment.
Reviewed head 546bad0 merged onto upstream/main befe264 (2026-10-24); the merge is clean, also together with #10580 (same file), check_conventions.py passes and pr_overlap.py reports no overlap with open pull requests.
Environment: Windows 11 Home 10.0.26200, Python 3.13.9 (Anaconda); Python only, no build needed.
Results
python -m unittest tools.tests.test_unwrap_selfon the merged tree: 51 tests, 47 pass, 4 skipped, 19 s. There is no test change and on 3.13 no behaviour change:Path.openalready callsio.openhere, soio.open(temporary_path, "xb")opens the same file the same way.- The mechanism matches CPython: on 3.10
Path.opengoes throughpathlib._NormalAccessor.open, a reference toio.opentaken whenpathlibis imported, somock.patch("io.open")never reaches it; 3.9 and 3.11 and newer lookio.openup on each call. 3.10 is the only affected version in the supported range, callingio.opendirectly is the smallest change that makes the seam work everywhere, and"xb"keeps the exclusive creation. - #9547 (shebang scripts executable) also touches
unwrap_self.py, mode only, so no text conflict.
Nit
- Body: "On Python 3.10,
Path.opendoes not go throughio.open" is slightly off; it does, but through a copy bound at import time, which is why patchingio.openhas no effect there. One sentence.
Notes, nothing to change here: CI has not run this change yet: the Build run on 546bad0 is waiting for approval (first contribution), and once approved the eligibility step skips the relinker jobs for authors without a merged PR, so the four tests have run only outside CI so far. No workflow pins a Python version (no setup-python), so CI runs whatever the runner ships, 3.11 or newer, where the tests passed before this change too; a later switch back to Path.open would not show in CI either. Nothing to do here, just the reason this went unnoticed.
Not checked: Python 3.10 itself (not installed here; the four failure-injection failures #10580's description reports are the ones this fixes), Linux and macOS runs.
I found no blocking issue: one line, same file, same mode, and on 3.10 the only call that needed to change. For disclosure: I have open PRs (#10667 on tools/tests/test_suggest_patches.py, and relinker PRs #10679, #10658, #10646, #10607, #10596, #10564, #10403, #10402, #10401, #10391, #10390); none touches the unwrapper.
Written with AI assistance (Claude Code). I ran the tests above on my Windows 11 machine.
| destination.parent.mkdir(parents=True, exist_ok=True) | ||
| temporary_path = destination.parent / (".unwrap-self-" + secrets.token_hex(16)) | ||
| temporary = temporary_path.open("xb") | ||
| temporary = io.open(temporary_path, "xb") |
There was a problem hiding this comment.
Checked: on 3.9 and on 3.11 and newer this is the same call Path.open made; on 3.10 Path.open resolves through pathlib._NormalAccessor.open, a copy of io.open taken at import, which is why the tests' patch("io.open") missed it there. "xb" keeps the exclusive creation, and 51 tests pass here on 3.13.9.
|
Windows check of AI-assisted: yes (Claude Code) |
What
tools/unwrap_self.pyopened its temporary output withPath.open("xb").tools/tests/test_unwrap_self.pyinjects write, close and temporary-file failures by patchingio.open. On Python 3.10,Path.opendoes not go throughio.open, so the injected failure never fires, the write succeeds andmainreturns 0 where the tests expect 2. Callingio.open(temporary_path, "xb")directly opens the same file in the same mode and lets the tests' hook apply on every Python version. BUILD.md lists Python 3.9 or newer for the Python tests, and #7142 kept the tools and tests on 3.9.This only changes how the temporary file is opened. Output, permissions, atomic replace and cleanup are unchanged. It is a test-compatibility fix, not a data-loss fix.
Tested
main99ad84397,ANYPS5_RELINKER_ONLY=ON,BUILD_TESTING=ON.ctest -R '^unwrap_self$'fails, andpython3 tools/tests/test_unwrap_self.pyreportsFAILED (failures=4, skipped=2):test_close_failure_preserves_existing_output,test_temporary_file_creation_failure_preserves_existing_outputand the two subtests oftest_write_failure_preserves_output_and_removes_temporary_files, eachAssertionError: 0 != 2.python3 tools/tests/test_unwrap_self.pyruns 51 tests, OK (skipped=2).python3 tools/check_conventions.py --base HEADpasses andgit diff --checkis clean.tools/pr_overlap.py(needs git 2.38 or newer, local git is 2.34), Windows, full runtime, GPU. Checked by hand: no open or closed PR changes this call. fix(tools): reject unsupported inner ELF versions #10580 also editsunwrap_self.py, in different lines.shader_cache_runtime_sources(no Ninja installed),linux_executable_exportsandpr_overlap_tools/check_main_sync(local toolchain and git version).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)