Windows: Use SHGetKnownFolderPath to compute LocalAppData
#5201
Conversation
|
cc @compnerd Could you please take a look at this PR to see whether there are anything else to fix? |
|
Could anyone please review this? |
compnerd
left a comment
compnerd
left a comment
There was a problem hiding this comment.
Sorry about the delay. This looks good to me!
|
@swift-ci please test |
This seems unlikely to be related ... @slavapestov any idea what this might be about? |
|
@swift-ci please test |
|
Could anyone please take a look at this PR to see what is going on? |
|
Swift 6.2 is released for months and this PR is till gets ignored. |
|
@swift-ci please test |
|
@ShikiSuen I think that we just need to get this through the CI systems, but this should be good to go otherwise. Please create a new PR with the same changes for the release/6.3 branch |
compnerd
changed the title
SHGetKnownFolderPath to compute LocalAppData
|
Oh @ShikiSuen we'd love to use this! Thank you for the fix, can't wait to see it in 6.3! |
|
@swift-ci please test Windows platform |
I've modified this PR targeting
I've modified this PR targeting |
|
@ShikiSuen no, that won't work - this must go to main first. We want a cherry-pick for 6.3 |
ShikiSuen
force-pushed
the
SHGetKnownFolderPath-Continued
branch
from
4750898 to
e71d604
Compare
|
@swift-ci please test |
ShikiSuen
force-pushed
the
SHGetKnownFolderPath-Continued
branch
from
e71d604 to
56a4e3f
Compare
|
@compnerd The current PR (5201) is now rebased using and targeting the main branch. (PR 5295 I pushed just now is targeting the 6.0.3 branch.) |
ShikiSuen
changed the title
SHGetKnownFolderPath to compute LocalAppDataSHGetKnownFolderPath to compute LocalAppData. (targeting main).
|
@swift-ci please test |
|
@swift-ci please test Windows platform |
|
@ShikiSuen this might require more work - it seems that the tests are failing due to an abnormal termination with this change. |
compnerd
changed the title
SHGetKnownFolderPath to compute LocalAppData. (targeting main).SHGetKnownFolderPath to compute LocalAppData
|
@compnerd I saw the test on WinNT is still running: I searched keyword 
|
I re-triggered the tests, so the link is gone. The search result is not important - the fact that the test process terminated abnormally is. That means that there is an invalid memory access or assertion being triggered now. |
|
@compnerd I need to know which assertion in which file fails the unit tests. P.S.: I'm not familiar with Win32 development. However, I personally suggest that CI for PRs should do A/B tests consecutively to identify whether a CI failure really is caused by the contents of the PR. Also, if it fail, it'd better to be a repeatable failure. Still, to my experience, network dependencies, if have, can fail CI unit tests reandomly. |
|
Unfortunately, SPM doesn't always report that. You would need to try to see if you can reproduce the issue locally. |
|
@compnerd Are unit test cases in CI run sequencially or in parallel? Let me note some intelligence collected from the failed checks: |
|
@compnerd By the way: This PR at this moment added |
|
@swift-ci Please test Windows platform. |
|
@swift-ci please test windows platform |
|
@swift-ci please test Windows platform |
|
Regarding the following CI log: CI Failure Analysis — Windows (WinNT) BuildTL;DR ✅This PR (branch:
I recommend re-running the CI (to rule out flake) and investigating LLDB's What I checked 🛠️
Evidence from CI log (excerpts)
Why I believe this is unrelated to the PR 🧭
Given these points, this failure is not introduced by changes to Suggested next steps / reproduction steps 🔁If you want to confirm or gather more debugging data, here’s what you can do:
# Example using dotest.py from llvm's test runner
python $LLVM_PROJECT_ROOT/lldb/test/API/dotest.py -v -p TestPlatformLaunchGDBServer.py -k test_launch_with_unusual_process_name --build-dir <LOCAL_BUILD_DIR> --arch x86_64 --executable <lldb_exe> --compiler <clang_exe>
cd <build-dir>
cmake --build . --target check-lldb
# or invoke the dotest.py as used by CI, with the same environment variables shown in CI log
Suggested reporting for LLDB (if issue persists) 🐛If the LLDB test fails consistently, please file an issue to LLVM/LLDB (https://github.com/llvm/llvm-project/issues/) and include:
Quick summary to maintainers 📨
If you want, I can also prepare a draft of the issue to open against LLVM/LLDB with the logs and reproduction steps included. Thanks — let me know if you want this file updated to include additional log excerpts or a screenshot of the CI failure details. |
|
Triggered the build, but it will pass. foundation is being built and tested for each swift PR. |
|
@compnerd Not always. The timestamp matters. That's why I said A/B tests for a PR is necessary. The current CI testing system only tests post-PR results. This hinders us from knowing whether the test pipeline works well at the time when the PR was committed. Let's see how the results of PR#5297 will be. |
No, that is a flake - that was LLDB tests |
|
https://ci-external.swift.org/job/swift-corelibs-foundation-PR-windows/810/ indicates that the tests are in fact passing as I had previously stated. |
|
@swift-ci Please test Windows platform. |
|
This patch is incorrect. I do not feel comfortable with this change due to the use of the AI tools as there isn't clear policy around that yet. |
|
@compnerd Is this repo compilable on ARM64 Windows? The CI takes way too many hours (probably days) to get a result. Unfortunately, I only have a M4 mac mini to run all these code stuffs. My only Windows machine is a Gigabyte Brix Game UHD which CPU is i7-6700HQ... might be too weak for compiling this kind of project. |
ShikiSuen
force-pushed
the
SHGetKnownFolderPath-Continued
branch
from
eedcd7a to
14cd1c3
Compare
|
I removed those commits generated by CfCCAgent and rebased to let this PR inherit recent changes from the merging target. Lemme draft this PR for now. Gonna see whether we can align the behavior to what dotNET 10 does. |
|
@compnerd I pushed a refactor commit to let the concerned block aligning to dotNET 10. Here are the references: |
ShikiSuen
force-pushed
the
SHGetKnownFolderPath-Continued
branch
from
60dc9fd to
2684bfe
Compare
|
(I intentionally squashed all commits and removed |
3 participants
Summary
Target Branch: swiftlang:main
This PR replaces manual
LocalAppDatapath construction with the Windows Shell APISHGetKnownFolderPathto reliably obtain the correct per-user local application data directory.Problem
On Windows, the environment variable
USERPROFILEis not guaranteed to equal%SYSTEMDRIVE%\Users\%USERNAME%. When the manually constructedLocalAppDatapath does not exist,UserDefaultswrites silently no-op, causing preferences to never persist.Solution
Use
SHGetKnownFolderPath(&FOLDERID_LocalAppData, ...)to programmatically retrieve the correctLocalAppDatapath instead of building it from environment variables or string concatenation.Changes
LocalAppDatapath construction withSHGetKnownFolderPathAPI call#include <shlobj_core.h>for the Shell APICoTaskMemFreeto properly release the pointer returned bySHGetKnownFolderPath_kCFKnownLocationUserCurrentto prevent unintended fallthrough after successful username lookupScope
TARGET_OS_WIN32)Sources/CoreFoundation/CFKnownLocations.cRisk Assessment
Severity: Low to Moderate (Windows-only)
Low Risk
LocalAppDataModerate Risk
LocalAppDatapath for users other than the current user (viaGetProfilesDirectoryW+ username). The new approach usesSHGetKnownFolderPath, which returns the path for the current user context only. If support for other-user preference paths (CFKnownLocationUserByName) on Windows is required, an alternative approach (e.g., SID impersonation orGetProfilesDirectoryW) may be needed.Build/Link Considerations
shlobj_core.hheader and linking toole32orshell32librariesRuntime Considerations
SHGetKnownFolderPathis available on Windows Vista and later. Swift targets higher WinNT builds, hence no worries.CoTaskMemFreeReferences
This approach is consistent with how .NET runtime handles
LocalAppDatapath retrieval on Windows:Testing
TO BE FILLED IN
Reviewers
TO BE FILLED IN