Skip to content
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

Windows: Remove use of deprecated function SHGetFolderPath and use SHGetKnownFolderPath instead #5862

Merged
merged 2 commits into from Mar 4, 2024

Conversation

kit-ty-kate
Copy link
Member

@kit-ty-kate kit-ty-kate commented Feb 28, 2024

Fixes #5861

Based on the 6 years old comment at the top of OPAMW_SHGetFolderPath we might probably want to use the new API and don't want to spend the time supporting such an old system

Copy link
Member

@dra27 dra27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this - apart from getting rid of youthfully exuberent (= daft) comments of mine from the source code, the fact SHGetKnownFolderPath handles allocation itself will automatically deal with long paths (which wasn't a thing when I wrote the original code; I just still harboured an unhealthy desire to keep old OSes working...!). I have a hunch that long paths might be what's gone wrong in the original issue (although it's surprising).

My instinct would be to have a single C stub taking a variant (more closely wrapping one API function), but actually I think your instinct is better in having separate functions! 🙂

We need to link opam with ole32 (for CoTaskMemFree) and uuid (for the FOLDERID_ "constants") - I'm pushing a commit which hopefully does that bit!

src/stubs/win32/opamWindows.c Outdated Show resolved Hide resolved
src/stubs/win32/opamWindows.c Outdated Show resolved Hide resolved
src/core/opamStd.ml Outdated Show resolved Hide resolved
@kit-ty-kate kit-ty-kate force-pushed the windows-SHGetKnownFolderPath branch 5 times, most recently from c3d8903 to 8d630a6 Compare February 29, 2024 10:32
@kit-ty-kate kit-ty-kate added this to the 2.2.0~beta2 milestone Feb 29, 2024
@kit-ty-kate kit-ty-kate added this to For beta2 in Opam 2.2.0 Feb 29, 2024
kit-ty-kate and others added 2 commits March 4, 2024 13:27
…GetKnownFolderPath instead

Co-authored-by: David Allsopp <david.allsopp@metastack.com>
Co-authored-by: David Allsopp <david.allsopp@metastack.com>
Copy link
Member

@dra27 dra27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@kit-ty-kate kit-ty-kate merged commit 1eeaa74 into ocaml:master Mar 4, 2024
29 checks passed
Opam 2.2.0 automation moved this from For beta2 to Done Mar 4, 2024
@kit-ty-kate kit-ty-kate deleted the windows-SHGetKnownFolderPath branch March 4, 2024 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Opam 2.2.0
  
Done
Development

Successfully merging this pull request may close these issues.

Opam for windows : make cold fails with OPAMW_SHGetFolderPath
3 participants