fix(pkg): add stub for shGetFolderPath #9486
Conversation
Signed-off-by: Ali Caglayan <alizter@gmail.com>
| @@ -78,6 +79,26 @@ CAMLprim value OPAMW_GetArchitecture(value unit) | |||
| return get_PE_cpu_architecture(NativeMachine); | |||
| } | |||
|
|
|||
| /* | |||
| * Somewhat against my better judgement, wrap SHGetFolderPath rather than | |||
| * SHGetKnownFolderPath to maintain XP compatibility. OPAM already requires | |||
There was a problem hiding this comment.
But then, wouldn't we be perfectly fine to use SHGetKnownFolderPath in our stubs? We don't really have to stay compatible with an OS from 2001.
I don't even think OCaml 5+ has a native compiler that supports 32-bit systems like XP machines would predominantly be.
There was a problem hiding this comment.
I didn't write this comment, but took it from the opam stubs. I think I'll try and do a more modern equivalent. We aren't really looking to support XP in Dune.
There was a problem hiding this comment.
Yes, I know, but the point of the comment is a valid one and while for OPAM it might've been the right call (I assume), it might not be for us.
|
Before there's any discussion of the code, can you update the description to include:
|
|
@rgrinberg I've updated the PR description with the issue that this is meant to fix: #9474 |
|
Either I'm missing some context, or the description is still incomplete. How is this stub supposed to help with reading a config file? Why would we even need this platform specific code to read a config file? |
|
@rgrinberg I'm not entirely sure why opam wants to read the home directory in this case. This PR simply stops that from being an error which it currently is. You can check the backtrace in that issue to see that this function eventually gets called and because it doesn't have an implementation it errors. We could probably do this in a better way, but it was my understanding that we didn't want to modify the opam code for now and get something working. |
|
Yes, we don't want to modify the opam code. Unfortunately, this is exactly what your PR is doing. Looking at the stacktrace, here's an alternative that does not modify upstream:
|
|
I somewhat disagree that this is what my PR is doing, but I agree that your alternative is perhaps a better option. |
Building packages on windows results in error described here:
This is due to a missing stub. This PR adds that missing stub.
@dra27 Could you take a look at this? I've plucked your code from the opam stubs, but there were some warnings about
WCHARbeing incompatible with the expected type forSHGetFolderPath. Turns out there is a variant calledSHGetFolderPathWspecifically for "W"ide characters which seems to work.When this patch is used, the build of a lock file gets further however invocation of
ocamlcseems to get confused about Windows paths inBUILD_PATH_PREFIX_MAP. Would you know if this is a known problem in OCaml?