-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Windows: Use SHGetKnownFolderPath to compute LocalAppData
#5201
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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 |
4750898 to
e71d604
Compare
|
@swift-ci please test |
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.) |
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. |
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 |

This PR targets the main branch.
On Windows, it's not guaranteed that
USERPROFILEis equivalent to%SYSTEMDRIVE%\Users\%USERNAME%. It is therefore possible thatUserDefaultstries to write to a non-existent location, in which case it will never work. When this happens, all writes silently no-op.This PR uses the Shell API
SHGetKnownFolderPathto programmatically get the location of%LOCALAPPDATA%instead of manually building the path.