Repository navigation
fix(tests): open scratch streams under a Unicode TEMP on Windows - #13104
Conversation
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — looks good
The Unicode-safe temporary-file helper is scoped to tests, and the exact-head diff and available Windows evidence show no blocking issue.
What I ran
Reviewed head 9ffb865109811c3c161159ebccce68373eed0d03 against PR base 886e6d752cfb41d8f634ca072f7ca1d9bee98f0d. The merge preview against current upstream/main 08a10d225306ad102b57408e2ef7566bcfb829b2 was clean (tree fe1282aef442de1c69531788583cc7a1ea9ef0b6). Diff and convention checks passed. I also checked the 11 shared-file comparisons flagged by the overlap workflow: each pairwise merge preview with the corresponding current PR head was clean. I did not build or run tests locally.
git merge-tree --write-tree upstream/main refs/remotes/upstream/pr13104
git diff --check upstream/main...refs/remotes/upstream/pr13104
python3 tools/check_conventions.py --base upstream/main --head refs/remotes/upstream/pr13104 --title 'fix(tests): open scratch streams under a Unicode TEMP on Windows' --body <(gh pr view 13104 -R boykopovar/AnyPS5 --json body --jq .body)Results
| Check | Result |
|---|---|
| Current-main merge preview | Clean; tree fe1282aef442de1c69531788583cc7a1ea9ef0b6 |
| Diff check | Passed; no output |
| Repository conventions | Passed |
| Flagged overlap comparisons | All 11 pairwise merge previews clean |
| Hosted CI | Windows, conventions, intake, eligibility, labels and report passed; Linux and specialized jobs skipped |
| Windows Unicode-path tests | Author-reported: all 15 changed tests passed three runs under an out-of-code-page TEMP; the unchanged guest_syslog test still fails as reported |
Unedited local check output
fe1282aef442de1c69531788583cc7a1ea9ef0b6
All conventions checks passed.
7386 e74db94b92ee9a1cabf15c9844e3e26becec2a28
8150 74942c47a4c5d4110013d00c16d8f19277584ef8
9371 cd32cee8d5105eca2986f14d759618216ec4b330
10405 255587cc8fd0180be8aed1787b3f4f2f82adf15e
10620 0cc80c6e6112f059662d4ce5e3ce933b91b4e6d7
11724 f97435f42e9d14b8667bf85648a5bf0de65cecd9
11726 3f845220c35773cda49734692c57e7ec4cb4915d
12057 4720b8de8391887560fbe70f2a2052907fa39b9d
12292 1e18a0e59e6a8fedb1b10d4de2d166d9c585bc26
12468 fd4439edb3c98e3e9cd808ef00f6d9e3487f4bea
12560 21ecce3834c01f0c693d8c1367adf302e3c53f0a
Findings
Blocking
None found.
Non-blocking
GuestSyslog.cpp remains on std::tmpfile() and is explicitly deferred until #9929 is resolved; the PR body accurately identifies that limitation.
Sources
- TemporaryFile helper at the reviewed head
- Microsoft CRT
_wfopenmode documentation documentsxas exclusive creation andDas automatic deletion when the file is closed.
Not verified
I did not compile or run the test suite locally. The Unicode-path tests and Windows build/test results above are reported by the PR author/hosted CI; Linux and macOS results are not provided.
|
Windows run of this head (
No file overlap with #12993, which fixes AI-assisted: yes (Claude Code). |
9ffb865 to
0f880fe
Compare
malekaifa
left a comment
There was a problem hiding this comment.
Reviewed exact current head 0f880fe with an independent Windows Unicode-TEMP reproduction. This host's Windows ANSI code page is 1252. With TEMP=TMP=C:\tmp\zh_Ж, a baseline std::tmpfile() probe returns NULL with errno=22; the exact PR TemporaryFile.hpp compiled with MSVC 14.44 (/std:c++17) opens successfully, writes/reads 'hello', closes successfully, and leaves zero anyps5-test-* files behind. Current-main merge-tree and git diff --check are clean. Compared with MarcZX10's previously approved 9ffb865 head, 14 of the 16 PR files are byte-identical; the two changed files (FileStreams.cpp and GuestStandardStreams.cpp) only absorbed upstream test additions, and their current PR-only hunks remain mechanical std::tmpfile() -> TemporaryFile::Open() replacements. Hosted CI is restarting on this rebase. This adds direct ACP1252 evidence for the issue's out-of-code-page path case; no blocking issue found.
On Windows, std::tmpfile() returns NULL with errno 22 when %TEMP% contains a character outside the ANSI code page, where GetTempPathA returns '?' in its place. Fifteen tests that capture output through it aborted. TemporaryFile::Open() opens a unique delete-on-close file under temp_directory_path() with _wfopen; other hosts keep std::tmpfile(). Part of boykopovar#13083 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… TEMP Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…de TEMP Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
0f880fe to
1ba27b1
Compare
|
Checked the diff, the helper (exclusive create, delete on close, EEXIST retry, non-Windows path unchanged) and the Windows runs from @MarcZX10, @JJenkx and @malekaifa.
|
… TEMP on Windows With %TEMP% holding a character outside the ANSI code page, two Windows tests still abort: - guest_syslog captures stderr through std::tmpfile(), which returns NULL there (boykopovar#13083). It now opens the capture with TemporaryFile::Open(), as the other libc tests do since boykopovar#13104. - guest_kernel_sys_console_identity starts its PS4 child with std::system and a command built from directory.string() and argv[0]. Both are narrow, so `cd /d` gets '?' in place of the character and fails. On Windows the command is now built from directory.wstring() and GetModuleFileNameW and run with _wsystem, as AsyncLog's child does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…on Windows On Windows, std::tmpfile() returns NULL with errno 22 when %TEMP% contains a character outside the ANSI code page. Thirteen more test files still called it: the AGC async log, frame dump, PM4 replay and recorder tests, eight AGC execution tests and the SceNet test. They now use TemporaryFile::Open(), as #13104 did for the libc tests. Part of #13083 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What
On Windows,
std::tmpfile()returnsNULLwitherrno22 when%TEMP%contains a character outside the ANSI code page, as it does for a profile likeC:\Users\Жаннаon a code page 1252 system. ThereGetTempPathAreturns?in place of the character (#13083). The tests that capture output through a host scratch stream then abort withFileStream: null handleorcannot open a temporary file for stderr.core/libs/tests/TemporaryFile.hpp:TemporaryFile::Open()creates a uniquely named file understd::filesystem::temp_directory_path()with_wfopen(..., L"w+bxD")(exclusive create, deleted on close, astmpfiledoes). Other hosts still returnstd::tmpfile(). It does not includewindows.h.std::tmpfile()withTemporaryFile::Open()(60 calls) and include the header. No other line changes.core/libs/tests/GuestSyslog.cpp, the 16th test in #13083, is left out: #9929 rewrites the lines around itsstd::tmpfile()call. It can switch to the helper after #9929 merges.No production code changes.
Tested
Windows 11 Pro 10.0.22631 (code page 1252), i9-9900K, MSYS2 UCRT64 GCC 16.1.0, Ninja, Release,
BUILD_TESTING=ON. The 16 test targets were built onmainf6fddd6 and on this branch, and the 16 tests ran withctest -j 2 -R '^(guest_binary_word|...|guest_filesystem)$'.TEMPandTMPmainf6fddd6...\zh_Ж(outside code page 1252)guest_syslog(unchanged) still fails...\e_é(inside the code page)TEMPA standalone probe of the header with the same toolchain opened two distinct files under each of those three
TEMPvalues, read back what it wrote, and left noanyps5-test-*file afterfclose. After the 3 runs underzh_Ж, that directory held noanyps5-test-*file.python tools/check_conventions.py --base origin/main: all checks passed.python tools/pr_overlap.py --branch fix/tests-wide-tmpfile: one line conflict,GuestSyslog.cppwith fix(libc): restore syslog program-name fallback #9929, removed by leaving that file out. It also listed 11 PRs that conflict withmainand share a file without a line comparison: feat(libc): scan 16-bit wide conversions on Linux #7386, fix(libc): support character reads from random device streams #8150, fix(libc): follow a Windows symbolic link before .. in a guest path #9371, fix(libc): share multibyte state in fgetwc #10405, fix(libc): reopen a stream onto /dev/random or /dev/urandom with freopen #10620, feat(libc): read 16-bit wide fields in fscanf on Linux #11724, feat(libc): accept positional arguments with wide scanf fields on Linux #11726, fix(libc): give an unbuffered stream FreeBSD's one-byte buffer on Windows #12057, fix(libc): validate null stream in ungetwc #12292, fix(libc): read wide scanf fields byte by byte, as titles' libc.prx does #12468, fix(libkernel): preserve DOS device names and trailing dots or spaces on Windows #12560.std::tmpfile(), which is what the tests called before.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)GuestSyslog.cppis left out for fix(libc): restore syslog program-name fallback #9929)guest_syslogfollows after fix(libc): restore syslog program-name fallback #9929)🤖 Generated with Claude Code