Repository navigation
test(libc): run guest_fopen_errors on Windows - #9583
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.
The Windows symlink handling is scoped to the test setup, requests the unprivileged-creation flag, and converts the specific permission failure into CTest's configured skip code with cleanup. The CMake registration and Windows PATH setup match neighboring tests. The current conventions check passes; the initial failure is superseded. Hosted relinker and related matrix checks pass; product builds are skipped. The Windows test and negative-control results are author-reported; I did not run them locally.
The CMake hunk overlaps #7851: both edit the same if(UNIX) registration area, and #7851 is still open. Your description already gives a safe resolution (keep #7851's guest_atomic_assert registration inside its UNIX guard while moving this test out). Please agree that order with its author and update Depends on if either PR must land first; currently this checklist says none. If both can be merged independently after that explicit agreement, record the agreed order and keep the two registrations under their correct platform conditions.
|
The Conventions check fails on the head of this PR (job 119617768332): Everything else in the matrix is green (relinker on all three OSes, relinker-asan, register-context, write-watch), and |
|
@fogydes thanks. That run was on the first head; the body has since been rewritten to the template and the Conventions job on the current head passes. @MarcZX10, the order with #7851 is agreed: @hypermoist confirmed on #7851 that neither PR depends on the other, and whichever merges second keeps AI-assisted: yes |
There was a problem hiding this comment.
The #7851 overlap is now resolved by agreement: its author accepted the order-independent resolution, keeping guest_atomic_assert inside if(UNIX) and this test outside it. The checklist records that agreement, so Depends on: none is accurate. The Windows symlink setup and permission-only skip path are also clear, and I found no test-logic issue in the diff. This head is 107 commits behind current upstream/main. Please update it and rerun its checks before merge. Checks on exact head da2d201a1de2b495dcd4cc12c8594387eb34821a: - git diff --check upstream/main...HEAD: passed. - python tools/check_conventions.py --base upstream/main --head HEAD: passed. - Hosted eligibility, labels, report, conventions and relinker/matrix checks pass. Hosted Linux/Windows product jobs are skipped. - The changed Windows behavior was not run locally in this Linux workspace. The Windows 11 test and negative control in the description are author-reported.
I’ve noticed that holding this PR because of #5239 isn’t necessary. It’s fine for PRs to keep coming in; they’ll be handled in due course. Please disregard my earlier request to hold or defer it.
|
Independent Windows run of this PR, as data only. Machine: Windows 11 Enterprise 10.0.22621 (
The main thread is stuck inside libstdc++'s directory walk. The gdb stack, attached after 4 s: This is the final
Each killed run also left an One way to fix it: on AI-assisted (Claude Code). |
|
Follow-up: I tried the suggested change in the test itself, on the same local merge (da2d201 + #9584 on std::error_code error;
+#ifdef _WIN32
+ DeleteFileW(L"loop");
+#endif
std::filesystem::current_path(std::filesystem::temp_directory_path(), error);
std::filesystem::remove_all(root, error);With it, AI-assisted (Claude Code). |
MarcZX10
left a comment
There was a problem hiding this comment.
The Windows test has a reproducible cleanup hang on this exact head. main() creates the self-referential loop symlink in EnterFreshRoot(), runs the checks, then changes to the temp parent and calls std::filesystem::remove_all(root) at GuestFopenErrors.cpp:120-121. An independent Windows/MinGW run reports this head timing out 10/10 because remove_all does not recognize the reparse point and loops in the directory walk. The same reviewer added DeleteFileW(L"loop") before changing the current directory and reports 10/10 passes with no leftover temp directories. Please remove the link explicitly on _WIN32 while the test is still in the fresh root, check that removal succeeds, then leave the existing remove_all cleanup in place. The permission-denied skip path is reached only when link creation fails, so it should not contain a created link.
I ran g++ -std=c++20 -Wall -Wextra -Werror -fsyntax-only -Icore/libs core/libs/tests/GuestFopenErrors.cpp and the corresponding x86_64-w64-mingw32-g++ command on exact head da2d201a1de2b495dcd4cc12c8594387eb34821a; both pass. These are syntax checks only; I cannot execute the Windows binary here. Hosted product jobs are skipped. The independent Windows timeout/fix runs are the runtime evidence for this blocker.
Separately, I withdraw my earlier generic intake objection: your contribution-count eligibility means the queue-based hold does not apply. The merge-order agreement with #7851 is already recorded. The branch still needs an update to current upstream main (8b3e39832af85f404c6195418d11b3e1ebd1924d, 150 commits ahead of this head), followed by the focused Windows run. Current-main diff-check and conventions pass.
|
I’ve noticed that my earlier request to hold this PR based on #5239 was unnecessary. It’s fine to keep submitting PRs; maintainers will handle them in due course. Please disregard that request. |
THEMANnt
left a comment
There was a problem hiding this comment.
Reviewed da2d201a1de2b495dcd4cc12c8594387eb34821a against 6ef5a1c4c92272598b18adeb9b5414112f8d15ed. No actionable findings in this focused review.
Source review: Inspected the complete platform/registration change and fixture cleanup. Windows uses CreateSymbolicLinkW for the self-referential file link; privilege failure is explicitly reported as skip 77 after leaving and deleting the fresh fixture directory. CTest now recognizes that skip and adds the Windows runtime path. Existing fopen/reopen/error assertions and non-Windows creation remain unchanged.
Validation: Windows / GCC 15.2: exact head syntax compilation passed; baseline fails because ::symlink is unavailable. The submitted test linked against existing local development libc/libkernel and returned 77 with the expected privilege message. That compatibility execution verifies the skip path, not the fopen assertions or a pinned runtime build. Conventions, whitespace and registration passed. Effective hosted relinker/ASan and metadata checks succeeded; product jobs were skipped.
Scope: Bounded Windows fixture/CTest portability change. Successful symlink-capable fopen execution remains unverified locally; no privileges/settings were changed and no game ran.
The test was registered only under if(UNIX) because it built its symbolic link loop with ::symlink. On Windows it now makes the loop with CreateSymbolicLinkW (unprivileged create) and exits 77, which CTest reports as skipped, when the account cannot create symbolic links.
…xture Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
da2d201 to
2736e7f
Compare
Dismissing a stale review: the requested changes were addressed after it (new head) and the merge queue blocks on it. Please request changes again if anything is still open.
|
The hang MarcZX10 reported is fixed on the current head: our commit deletes the |
What
guest_fopen_errorswas registered only underif(UNIX)(libc/CMakeLists.txt:137). Its only POSIX dependency was::symlink, which it uses to build a self-referencing link. Everything it checks,fopen/freopen/fopen_sreturning ELOOP, ENAMETOOLONG, EISDIR, EINVAL and ENOENT, also exists on Windows.How:
GuestFopenErrors.cpp: on Windows the loop is made withCreateSymbolicLinkW(L"loop", L"loop", SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE). If that fails, the test requiresERROR_PRIVILEGE_NOT_HELD, prints the same "needs Developer Mode or administrator rights" line asGuestDirectory.cppandGuestAmpr.cpp, cleans up and exits 77.<unistd.h>is included only off Windows.libc/CMakeLists.txt: the test leavesif(UNIX). It getsSKIP_RETURN_CODE 77, so a CI account without symlink rights reports a skip, not a failure, plus the usual WIN32PATHprepend of the libc directory.Tested
ctest -R ^guest_fopen_errors$passes.GuestEloopmapping inHostErrno.cpp:26disabled, the test fails at line 69 (fopen("loop", "r")no longer reports ELOOP). Restored, it passes.tools/check_conventions.pypasses.Overlap (
tools/pr_overlap.py --branchplusgit merge-tree):guest_atomic_assertinside the sameif(UNIX)block, directly after the lines this PR moves out, so the two conflict textually. Resolution: keep feat(libc): implement std::_Atomic_assert #7851's five lines insideif(UNIX).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)guest_atomic_assertinsideif(UNIX)andguest_fopen_errorsoutside)