Closed dra27 closed 4 months ago
OK, the fix I was proposing I didn't like because it didn't preserve unnecessarily escaped parts of existing variables. I then further found it regressed #4861 and consequently broke the msvs-detect package.
I have a plan (which I'm currently implementing) - a failing test for the main issue (#5838) added and for the first bit of the fix, which I am certain is right. The change I think is required for #5838 without breaking #4861 I think will also address #5925 and #5926, so I've added tests showing the failure behaviour for them as well.
The change in c4167f6a7ee8248b7aae0fe65f3695703e80c73b requires environment
files to be reset - @rjbou... does that happen naturally with a format upgrade?
That said, it's making me wonder if we should ever be "putting" rewrite_default
in the environment
cache files anyway?
Thanks!
Fixes for #5838, #5925 and #5926. This PR has become quite an epic rabbithole, but the fixes and changes are very, very intertwined. It is possible to separate the final fix for #5926 to a subsequent PR, but the final fix itself is actually quite small.
The first commit adds a new
sort
operation to the reftest language. While working on this, the stability of the output ofopam env
and the stability of the output when usingunordered
made making the true effect of each commit tricky. I'm still not sure thatunordered
is behaving as well as it should when there's actually a change to the output (i.e. when one line changes, it's allowing some unchanged lines to be re-ordered noisily in the diff).I then add a whole range of tests:
MANPATH
, I noticed thatCAML_LD_LIBRARY_PATH
wasn't reverting correctly on Windows either, and it turns out that's because opam wasn't correctly handling updates with forward slashes (i.e.CAML_LD_LIBRARY_PATH += "%{lib}%/stublibs
expands to%OPAMROOT%\switch\lib/stublibs
butopam env
normalises the/stublibs
to\stublibs
. When reverting, this normalisation is missing.+=
,=+
,:=
and=:
applied to a variable set to""
in the samesetenv
block and shows+=
and=+
misbehaving.a:
and:a
, both set in the environment, and synthesised by using:=
or=:
on an unset variablea1::a2
(a "middle" blank) entry:
There have been a lot of issues and PRs in the past to do with the treatment of
:=
and=:
principally whereMANPATH
is concerned. I'm relatively confident that the approach for this before has not been strictly correctly (and is the cause of the issue being seen in #5926). For bothPATH
(as defined by Posix) andMANPATH
(as defined by the tool itself), an unset value, an empty value and the value:
are all equivalent. ForPATH
, that means the current directory and all of these entries are literally equivalent toPATH=.
. ForMANPATH
, those three entries mean "use the default search path".Now, as far as I can see the previous discussions have been about ensuring that
MANPATH
has a leading or trailing:
, to ensure that opam never hides the default path. Thus, ifMANPATH
is not set, then we haveMANPATH =: "%{man}%"
giving us:/home/dra/.opam/default/man
. However, the focus should not be on having a leading or trailing:
, the focus should be on ensuring that a "blank" entry is created if needed (this is the rule forFOO := "a"
andFOO =: "a"
area:
or:a
respectively ifFOO
is unset or empty) and but then for all updates separately ensuring that the blank entry is preserved.i.e. if
FOO
is:a
then bothFOO += "b"
andFOO := "b"
should setFOO
tob::a
The actual fixes then proceed as:
CAML_LD_LIBRARY_PATH
issue arises from what appears to be an incorrect comment inOpamEnv.apply_op_zip
indicating that the argument is already transformed, which may suggest whyOpamEnv.reverse_env_update
was missing the transformation. Essentially, the transformation ensures thatopam env --revert
reverts the same value as it applied inopam env
OpamEnv.split_var
, the cases for splitting using the quoted rules were the wrong way around. Fixing that regresses #4861, though[]
. However, ifsetenv
orbuild-env
hasFOO = ""
then we end up with[""]
as opam must ensure that this variable is overwritten. The easiest solution for #5925 is simply to allow for that -[""]
can only arise fromFOO = ""
as an update, so+=
and=+
handle that case, and remove the""
entry from the list.:=
and=:
at this point of course work correctly, because the""
should be there.OpamEnv.split_var
is not quite right - it doesn't cope correctly with a separator quoted inside the double-quote marks. My initial approach therefore was to replace it with the PATH-splitter inOpamStd.Env.split_path
. This, however, reveals a more fundamental problem - the PATH-splitter correctly splits the PATH, but it removes all the quote marks. The user is at liberty to add quotes in a PATH even if they're not needed. For example, it's a common mistake on Windows to quote directories with spaces in them (having, for example,Path
set toC:\Windows\system32;"C:\Program Files\Something\bin"
). With the corrected split function,PATH += "foo"
causesPath
to becomefoo;C:\Windows\system32;C:\Program Files\Something\bin
with the superfluous quotes removed. This isn't correct, given that the effect ofPATH += "foo"
should morally be much closer toexport PATH="foo;$PATH"
.x-env-path-rewrite
, if two packages disagree over the separator, then we have the slightly odd situation where the variable is split according to the first package's separator and re-joined according to the second package's. i.e. ifPath
isfoo;bar
and the first package to be evaluated hasPATH += "baz"
thenPath
internally is["baz"; "foo"; "bar"]
and will be exported asbaz;foo;bar
. However, if another package is then added which incorrectly hasx-env-path-rewrite: PATH ":" "target"
and executesPATH += "broken"
then internally we have["broken"; "baz"; "foo"; "bar"]
but the variable will be reconstituted asbroken:baz:foo:bar
, so from an initial value offoo;bar
,opam env
changed the existing separator.Opam.unzip_to
to split the value inFOO += value
. I've commented (hopefully clearly) the three cases in the commit. At this point, the #4861 test passes again and #5838 is fixed.::
as required (see the changes in the reftests in that commit)OpamEnv.reverse_env_update
for=+
and=:
where one list was reversed which shouldn't be. Again, the effect is visible in the reftests! The point is that a reversed environment variable was unzipped, and needs to reversed, but there was one reversal too many, given how environment variable updates are stored.TODO