Fix case-preserving environment updates on Windows #5356
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rjbou
added
the
PR: QUEUED
|
queued on #5374 |
It's irrelevant - the current directory is _always_ included in PATH on Windows.
dra27
removed
the
PR: QUEUED
rjbou
reviewed
Feb 7, 2023
rjbou
reviewed
Feb 7, 2023
rjbou
reviewed
Feb 7, 2023
rjbou
reviewed
Feb 7, 2023
rjbou
approved these changes
Feb 7, 2023
|
added a test |
OpamEnv.Env.Name.t introduced to defend against comparing environment variable names as strings. Propagate this change and fix comparison of environment variables on Windows in the process.
dra27
added a commit
to dra27/opam
that referenced
this pull request
Feb 14, 2024
Logical statement became accidentally inverted.
rjbou
pushed a commit
to dra27/opam
that referenced
this pull request
Feb 16, 2024
Logical statement became accidentally inverted.
dra27
added a commit
to dra27/opam
that referenced
this pull request
Mar 10, 2024
Logical statement became accidentally inverted.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There's existing code (of mine) which tries to deal with the fact that environment variable names are case insensitive on Windows. In
OpamStd.Env, this is dealt with largely correctly inget, but the handling inOpamEnvis not. When dealing with environment updates, it's necessary to transform the names of any updates - i.e.PATH += "foo"needs to be transformed toPath += "foo"to use the same case as the base environment.The problem is manifesting itself principally with
PATH, which by default is actuallyPathon Windows. When reverting the updates inOpamEnv.get_pure(for example, to evaluateeval_variables), the switch'sbindirectory is correctly ignored when resolving the command, but when the command is executed, it re-emerges, because of a duplicate instruction in the env block for bothPath(with the switch directory, as that comes from the calling environment) andPATH(as determined inOpamEnv.get_pure, which reverts the updates previously made).In particular, with this branch,
opam config liststops reporting the switch's compiler settings insys-ocaml-arch,sys-ocaml-cc, andsys-ocaml-libc, but I still need to mull some more whether this is strictly the correct approach (and the environment update code is already nightmarishly complicated even on Unix...)