Repository navigation
fix(libkernel): classify Windows directory links by app0 scope - #8726
Conversation
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed the current diff and test report. Deriving the getdents entry type from the same no-follow FillLinkStat classification used by lstat correctly distinguishes ordinary files/directories from normal, dangling, and self-loop junctions. Load builds a temporary snapshot and only publishes it after every entry is classified, so an unsupported reparse type does not leave a partially loaded directory. The author reports the Windows junction regression failing on baseline and passing on the PR, plus the full CTest delta with only identical pre-existing failures. Hosted conventions/eligibility/intake/labels pass; platform build jobs are skipped. I did not run a local Windows build. AI-assisted review.
|
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.
|
|
@ArjenS-LA Thanks. Deriving the type from One request about the new failure path. After that, the bar is a full CI run with |
|
I reproduced the stat race in Prospero Explorer 1.0 (PPSA99109) and prepared the narrow fix: failed Using the same shipped Explorer package and workload, a debugger breakpoint removes one added host fixture immediately before stat. Published head Windows x64, MinGW-w64 GCC 15.2.0: the same six directory regression executables pass with both baseline and fixed libraries. Junction cases executed; optional symbolic-link subcases skipped because the host lacks that privilege. The explicit The patch is still local and uncommitted (base above; tree |
|
@ArjenS-LA thanks for this. For links the guest makes itself, We have a concrete case of this on Linux, where Today Windows reports a junction to a directory as 4, so a junction-built app0 works there. After this change it would report 10, so the same title would see the same listing on Windows (we haven't run it there). Your Explorer row shows the switch on Could you keep the target's type inside the app0 tree? For an entry under We're making the matching change to the Linux path, so both platforms will treat app0 the same way. |
|
@oneandonlydean I reproduced the Windows part with a focused native junction fixture on Windows x64 / MinGW-w64 GCC 15.2.0. Against the published There is a path-tracking detail to coordinate: |
|
@oneandonlydean I found the matching Linux change in #9303 at Should the Windows correction mirror that documented limitation, or must it preserve the guest Both PRs also add different implementations of |
|
@ArjenS-LA Mirror the #9303 limitation; don't add directory-origin tracking in this PR. #9303 decides "inside So on Windows the Keep the Windows fixture in its own source file. #9303's |
|
@oneandonlydean The local correction now mirrors #9303's resolved-path limitation. It follows live links when the enumerated directory resolves inside canonical Windows x64 / MinGW-w64 GCC 15.2.0 results:
This is still an uncommitted draft, tree The draft merges textually with main |
ArjenS-LA
changed the title
b898b07 to
4fb4187
Compare
|
@simonbronk I checked the #9732 overlap with Git's merge engine against main This is an adjacent documentation edit: the Windows directory-type entry is inserted immediately before your revised vector-I/O entry. Both entries should survive; the implementation files are separate. I propose #8726 first, then rebasing #9732 while preserving both entries. Does that order work for you? Maintainers retain the final choice; no dependency has been declared before agreement. This check used source inspection and tree-only merges, with no new commits, builds or runtime tests. |
|
Open #9732 changes the vector-I/O debt entry in |
|
Independent Windows run, as data only. Machine: Windows 11 Enterprise 10.0.22621, WinLibs GCC 15.2.0 UCRT r7, Release. Head 4fb4187 was merged onto
AI-assisted (Claude Code). |
|
Correction to point 2 of my comment above: the app0 The test creates
So the fix belongs in the test: write those two targets with backslashes. Point 1, the AI-assisted (Claude Code). |
|
Follow-up: same machine and merge (head 4fb4187 on - for (const auto& name : links) Require(std::filesystem::remove(root / name));
+ for (const auto& name : links) Require(RemoveDirectoryW((root / name).c_str()) != 0 || DeleteFileW((root / name).c_str()) != 0);
- const bool symbolic = CreateSymbolicLinkW((root / "app0/file-link").c_str(), L"../external/regular",
+ const bool symbolic = CreateSymbolicLinkW((root / "app0/file-link").c_str(), L"..\\external\\regular",
- Require(CreateSymbolicLinkW((root / "app0/relative").c_str(), L"../external",
+ Require(CreateSymbolicLinkW((root / "app0/relative").c_str(), L"..\\external",
- for (auto it = links.rbegin(); it != links.rend(); ++it) Require(std::filesystem::remove(root / *it));
+ for (auto it = links.rbegin(); it != links.rend(); ++it) Require(RemoveDirectoryW((root / *it).c_str()) != 0 || DeleteFileW((root / *it).c_str()) != 0);With those changes, AI-assisted (Claude Code). |
|
@JJenkx The correction belongs in Local check: Windows x64, WinLibs GCC 15.2.0 UCRT r7. I freshly compiled both the published fixture and the corrected fixture and ran them against the same preserved libraries from that head. Both pass the junction cases and leave no test directories behind. Both skip native symbolic-link subcases because this session lacks the privilege, so I cannot independently reproduce your symbolic-link before/after result or the cleanup failure on your machine. Your five failing/five passing runs remain separately attributed evidence. No full build, CTest suite, homebrew or hardware run was repeated for these test edits. The correction is preserved locally in |
|
@ArjenS-LA thanks. This machine can create symbolic links without elevation, so the symbolic-link subcases run here. When you push the corrected test, I'll rerun |
|
@ArjenS-LA that order works for me: #8726 first, then I rebase #9732 and keep both debt entries. #9732 doesn't need #8726's code, so I'm leaving Depends on as none. AI-assisted: yes |
|
@qpavyros I confirmed the #8520 overlap with Git's merge engine against main The shared Would #8726 first, once its pending test correction and full CI are ready, then rebasing #8520 with both behaviors preserved work for you? Maintainers retain the final order; no code dependency is declared before agreement. This is source inspection and tree-only merge evidence, with no combined build or runtime result. AI-assisted. |
|
@xXPBSXx I checked the new #10121 overlap against main This is an adjacent documentation edit: #10121 extends the Would #8726 first, once its pending test correction and full CI are ready, then rebasing #10121 while preserving both entries work for you? If the documentation change is ready sooner, please say so and we can coordinate the reverse order. This is a proposal, not an agreed dependency; maintainers retain the final order. This check used source inspection and tree-only Git merges. No code, commits, builds or runtime tests changed in this pass. AI-assisted. |
|
Gruzzberry here: I am opening a small PR that changes the file number (d_fileno) getdirentries reports in DirectoryDescriptor.cpp, next to lines your PR edits. Proposal: yours goes first and mine rebases. Tell me if you would rather go the other way. |
What
Windows directory enumeration classifies links by canonical app0 scope: live links inside that root report their target type, while dangling links and links outside it remain DT_LNK. Failed initial stat returns DT_UNKNOWN, and the directory snapshot is published only after collection succeeds. This follows the reviewed resolved-path scope.
The test correction in
c5229e510a4b463fd4f6d39f017793d753f7c86buses Windows backslashes for relative symbolic-link targets andRemoveDirectoryW/DeleteFileWfor cleanup of dangling and looping reparse points. Production code is unchanged from4fb4187bd327a33cb139959d596c46782c3ac08d.Tested
Windows x64, supported MinGW-w64 GCC 15.2.0: both the previous and corrected fixtures freshly compile and pass the junction cases against the same preserved libraries from 4fb4187. Both leave no test directories behind. Native symbolic-link cases skip because the user token lacks symbolic-link privilege, so the reviewer’s failure is not independently reproduced locally. JJenkx reported five failing runs followed by five passing runs with these exact test edits and symlink cases enabled. His offered rerun on this corrected head remains pending.
The branch and current-main conventions checkers pass, as does whitespace validation. No full build, full CTest, homebrew or GPU run was repeated for this test-only correction. Full Linux/Windows CI still needs a collaborator-triggered run for the new head; the previous successful Build workflow skipped its platform test jobs.
Earlier implementation validation at 4fb4187 remains applicable to the unchanged production code: full build and explicit libraries build passed; CTest reported 1,882 passes, 6 skips and 3 unresolved GPU sample-order failures. Released Prospero Explorer v1.0 (PPSA99109) execution, with disclosed host-added junction and removable-file fixtures, showed app0 junction type 4 and DT_UNKNOWN for the disappearing entry. Original package bytes were unchanged; decrypted inputs were converted separately. This established directory-response behavior under those accommodations, not gameplay or physical-console behavior.
Current overlap check: main
c8d34aa8f4d8058d57169cc1e423ba6db00faaf0, corrected headc5229e510a4b463fd4f6d39f017793d753f7c86b, 1,288 open PRs with complete paginated changed-file lists. This PR merges cleanly with main. Tree-only comparisons of 129 shared-file peers yielded 89 clean pairs, 3 conflicts and 37 inconclusive pairs because the peer already conflicts with main. No other open PR changes the Windows link fixture. Source review was selective; combined runtime compatibility is unverified.0a64d23bdf502e32b1857009a5227634d4a53f16core/libs/prx/libkernel/File/src/DirectoryDescriptor.cppacac3b1c00a97443536b6697bb2f7ad46a0c57b5docs/dev/debt/libkernel.mde9b842569aaf59e5b1282b2637b10e94d589109bdocs/dev/debt/libkernel.md#8520 integration must preserve archive/disk enumeration, inode/cursor handling, canonical-app0 target typing, failed-stat DT_UNKNOWN and complete snapshot publication. Its ordering discussion remains pending. #9732’s author agreed to #8726 first, then rebase while retaining both debt entries. #10121 ordering remains pending; both adjacent debt entries must survive. The test correction does not alter these existing overlaps. The complementary Linux PR #9303 has merged.
Checklist
main; no other open PR implements the same functions — tested merge against the exact main above; overlapping implementation and integration limits are disclosed.