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

Environment revert not working correctly for Unix-like variables on Windows #5838

Closed
dra27 opened this issue Feb 14, 2024 · 1 comment · Fixed by #5935
Closed

Environment revert not working correctly for Unix-like variables on Windows #5838

dra27 opened this issue Feb 14, 2024 · 1 comment · Fixed by #5935
Assignees
Projects
Milestone

Comments

@dra27
Copy link
Member

dra27 commented Feb 14, 2024

C:\Users\DRA>opam switch create manpath --empty
# Run for /f "tokens=*" %i in ('opam env --switch=manpath') do @%i to update the current shell environment

C:\Users\DRA>opam env
set OPAM_LAST_ENV=C:\Users\DRA\AppData\Local\opam\manpath\.opam-switch\last-env\env-3fd90d6beff7ec39310fda00a45ad218-0
set OPAM_SWITCH_PREFIX=C:\Users\DRA\AppData\Local\opam\manpath
set "MANPATH=:"C:\Users\DRA\AppData\Local\opam\manpath\man""
set "Path=C:\Users\DRA\AppData\Local\opam\manpath\bin;..."

C:\Users\DRA>for /f "tokens=*" %i in ('opam env') do @%i

C:\Users\DRA>echo %MANPATH%
:"C:\Users\DRA\AppData\Local\opam\manpath\man"

C:\Users\DRA>opam env
set OPAM_LAST_ENV=C:\Users\DRA\AppData\Local\opam\manpath\.opam-switch\last-env\env-3fd90d6beff7ec39310fda00a45ad218-0
set OPAM_SWITCH_PREFIX=C:\Users\DRA\AppData\Local\opam\manpath
set "MANPATH="C:\Users\DRA\AppData\Local\opam\manpath\man":"C:\Users\DRA\AppData\Local\opam\manpath\man""
set "Path=C:\Users\DRA\AppData\Local\opam\manpath\bin;..."

C:\Users\DRA>opam switch
#  switch   compiler  description
→  manpath            manpath

With #5837, we now get the correct warning:

C:\Users\DRA>opam switch --debug
00:00.041  CLI                    Parsing CLI version 2.2
00:00.042  GSTATE                 LOAD-GLOBAL-STATE @ C:\Users\DRA\AppData\Local\opam
00:00.043  SWITCH                 list
#  switch   compiler  description
→  manpath            manpath
00:00.044  ENV                    Not up-to-date env variables: [MANPATH]

[WARNING] The environment is not in sync with the current switch.
          You should run: for /f "tokens=*" %i in ('opam env') do @%i
@dra27 dra27 added this to For RC in Opam 2.2.0 Feb 14, 2024
@dra27 dra27 self-assigned this Feb 14, 2024
@dra27
Copy link
Member Author

dra27 commented Feb 14, 2024

I'm not sure how this slipped through with the x-env-path-rewrite changes - possibly it just wasn't noticeable because of #5837. The WIP I have so far observes a coding fault with OpamEnv.split_var (the cases seem to be the wrong way around, and the quoting function doesn't appear to be working properly) but there's also a logical error in that the manpath C:\Users\DRA\AppData\Local\opam\manpath\man is being split during the revert and as it's not quoted, it's being treated as C and \Users\DRA\AppData\Local\opam\manpath\man which is what causes the environment blow-up.

I think that what's needed (and which I thought we had done, but I haven't debugged enough to see what's going on) was that the variable gets split when the environment update is resolved into constituent parts - so given a resolved update FOO += "bar", we're supposed to know that "bar" is an unquoted single "directory" (or string), i.e. setenv: FOO += "\"bar:baz\":foo" is supposed to resolve to two updates FOO += "foo" and FOO += bar:baz`` with no further splitting on either update (subject to x-env-path-rewrite).

@kit-ty-kate kit-ty-kate moved this from For RC to For beta3 in Opam 2.2.0 Apr 8, 2024
@dra27 dra27 linked a pull request Apr 24, 2024 that will close this issue
2 tasks
dra27 added a commit to dra27/opam that referenced this issue Apr 25, 2024
MANPATH should appear in opam env --revert
dra27 added a commit to dra27/opam that referenced this issue Apr 29, 2024
MANPATH should appear in opam env --revert
dra27 added a commit to dra27/opam that referenced this issue Apr 29, 2024
The cases of this match were the wrong way around.

Combined with the previous fixes, this finally addressed ocaml#5838.
dra27 added a commit to dra27/opam that referenced this issue May 2, 2024
MANPATH should appear in opam env --revert
dra27 added a commit to dra27/opam that referenced this issue May 2, 2024
Existing implementation was not correct w.r.t. separators which were
themselves quoted. Use OpamStd.String.split_quoted instead. However,
this causes superfluous quotes in variables to be removed.

Note that this fixes the MANPATH issue of ocaml#5838 but the quoting-handling
is still broken.
dra27 added a commit to dra27/opam that referenced this issue May 2, 2024
When reverting [FOO += "value"], it is necessary to determine what to do
if "value" is of the form "<value1><sep><value2>". Because FOO itself
will have been split at <sep>, we will never match
"<value1><sep><value2>" so it is necessary to split value at <sep> as
well (this is the crux of ocaml#4861).

However, the interaction of this and x-env-path-rewrite is incorrect. If
x-env-path-rewrite has [FOO false] (the `norewrite case), then the value
must be split according to the default interpretation of FOO (which is
how it was added).

If x-env-path-rewrite has a rewrite rule for FOO, then opam has already
assumed that value refers to a single directory (the host/target and
quoting transformation options all being this way).

If x-env-path-rewrite does not have any rule (and does have have
[FOO false]) then we are in essence in the backwards-compatible case,
and must split value.

In order to stop poor interaction between this and opam itself
injecting Windows paths, opam's updates in OpamEnv (to MANPATH, etc.)
are explicitly expanded so that in unzip_to they are treated as though
they had an x-env-path-rewrite rule given (and are therefore not split).

Combined with the previous fixes, this addresses ocaml#5838 without
regressing ocaml#4861.
rjbou pushed a commit to rjbou/opam that referenced this issue May 10, 2024
MANPATH should appear in opam env --revert
rjbou pushed a commit to rjbou/opam that referenced this issue May 10, 2024
Existing implementation was not correct w.r.t. separators which were
themselves quoted. Use OpamStd.String.split_quoted instead. However,
this causes superfluous quotes in variables to be removed.

Note that this fixes the MANPATH issue of ocaml#5838 but the quoting-handling
is still broken.
rjbou pushed a commit to rjbou/opam that referenced this issue May 10, 2024
When reverting [FOO += "value"], it is necessary to determine what to do
if "value" is of the form "<value1><sep><value2>". Because FOO itself
will have been split at <sep>, we will never match
"<value1><sep><value2>" so it is necessary to split value at <sep> as
well (this is the crux of ocaml#4861).

However, the interaction of this and x-env-path-rewrite is incorrect. If
x-env-path-rewrite has [FOO false] (the `norewrite case), then the value
must be split according to the default interpretation of FOO (which is
how it was added).

If x-env-path-rewrite has a rewrite rule for FOO, then opam has already
assumed that value refers to a single directory (the host/target and
quoting transformation options all being this way).

If x-env-path-rewrite does not have any rule (and does have have
[FOO false]) then we are in essence in the backwards-compatible case,
and must split value.

In order to stop poor interaction between this and opam itself
injecting Windows paths, opam's updates in OpamEnv (to MANPATH, etc.)
are explicitly expanded so that in unzip_to they are treated as though
they had an x-env-path-rewrite rule given (and are therefore not split).

Combined with the previous fixes, this addresses ocaml#5838 without
regressing ocaml#4861.
rjbou pushed a commit to rjbou/opam that referenced this issue May 10, 2024
Existing implementation was not correct w.r.t. separators which were
themselves quoted. Use OpamStd.String.split_quoted instead. However,
this causes superfluous quotes in variables to be removed.

Note that this fixes the MANPATH issue of ocaml#5838 but the quoting-handling
is still broken.
rjbou pushed a commit to rjbou/opam that referenced this issue May 10, 2024
When reverting [FOO += "value"], it is necessary to determine what to do
if "value" is of the form "<value1><sep><value2>". Because FOO itself
will have been split at <sep>, we will never match
"<value1><sep><value2>" so it is necessary to split value at <sep> as
well (this is the crux of ocaml#4861).

However, the interaction of this and x-env-path-rewrite is incorrect. If
x-env-path-rewrite has [FOO false] (the `norewrite case), then the value
must be split according to the default interpretation of FOO (which is
how it was added).

If x-env-path-rewrite has a rewrite rule for FOO, then opam has already
assumed that value refers to a single directory (the host/target and
quoting transformation options all being this way).

If x-env-path-rewrite does not have any rule (and does have have
[FOO false]) then we are in essence in the backwards-compatible case,
and must split value.

In order to stop poor interaction between this and opam itself
injecting Windows paths, opam's updates in OpamEnv (to MANPATH, etc.)
are explicitly expanded so that in unzip_to they are treated as though
they had an x-env-path-rewrite rule given (and are therefore not split).

Combined with the previous fixes, this addresses ocaml#5838 without
regressing ocaml#4861.
rjbou pushed a commit to rjbou/opam that referenced this issue May 10, 2024
MANPATH should appear in opam env --revert
rjbou pushed a commit to rjbou/opam that referenced this issue May 10, 2024
Existing implementation was not correct w.r.t. separators which were
themselves quoted. Use OpamStd.String.split_quoted instead. However,
this causes superfluous quotes in variables to be removed.

Note that this fixes the MANPATH issue of ocaml#5838 but the quoting-handling
is still broken.
rjbou pushed a commit to rjbou/opam that referenced this issue May 10, 2024
When reverting [FOO += "value"], it is necessary to determine what to do
if "value" is of the form "<value1><sep><value2>". Because FOO itself
will have been split at <sep>, we will never match
"<value1><sep><value2>" so it is necessary to split value at <sep> as
well (this is the crux of ocaml#4861).

However, the interaction of this and x-env-path-rewrite is incorrect. If
x-env-path-rewrite has [FOO false] (the `norewrite case), then the value
must be split according to the default interpretation of FOO (which is
how it was added).

If x-env-path-rewrite has a rewrite rule for FOO, then opam has already
assumed that value refers to a single directory (the host/target and
quoting transformation options all being this way).

If x-env-path-rewrite does not have any rule (and does have have
[FOO false]) then we are in essence in the backwards-compatible case,
and must split value.

In order to stop poor interaction between this and opam itself
injecting Windows paths, opam's updates in OpamEnv (to MANPATH, etc.)
are explicitly expanded so that in unzip_to they are treated as though
they had an x-env-path-rewrite rule given (and are therefore not split).

Combined with the previous fixes, this addresses ocaml#5838 without
regressing ocaml#4861.
rjbou pushed a commit to rjbou/opam that referenced this issue May 10, 2024
MANPATH should appear in opam env --revert
rjbou pushed a commit to rjbou/opam that referenced this issue May 10, 2024
Existing implementation was not correct w.r.t. separators which were
themselves quoted. Use OpamStd.String.split_quoted instead. However,
this causes superfluous quotes in variables to be removed.

Note that this fixes the MANPATH issue of ocaml#5838 but the quoting-handling
is still broken.
rjbou pushed a commit to rjbou/opam that referenced this issue May 10, 2024
When reverting [FOO += "value"], it is necessary to determine what to do
if "value" is of the form "<value1><sep><value2>". Because FOO itself
will have been split at <sep>, we will never match
"<value1><sep><value2>" so it is necessary to split value at <sep> as
well (this is the crux of ocaml#4861).

However, the interaction of this and x-env-path-rewrite is incorrect. If
x-env-path-rewrite has [FOO false] (the `norewrite case), then the value
must be split according to the default interpretation of FOO (which is
how it was added).

If x-env-path-rewrite has a rewrite rule for FOO, then opam has already
assumed that value refers to a single directory (the host/target and
quoting transformation options all being this way).

If x-env-path-rewrite does not have any rule (and does have have
[FOO false]) then we are in essence in the backwards-compatible case,
and must split value.

In order to stop poor interaction between this and opam itself
injecting Windows paths, opam's updates in OpamEnv (to MANPATH, etc.)
are explicitly expanded so that in unzip_to they are treated as though
they had an x-env-path-rewrite rule given (and are therefore not split).

Combined with the previous fixes, this addresses ocaml#5838 without
regressing ocaml#4861.
rjbou pushed a commit to rjbou/opam that referenced this issue May 10, 2024
MANPATH should appear in opam env --revert
rjbou pushed a commit to rjbou/opam that referenced this issue May 10, 2024
Existing implementation was not correct w.r.t. separators which were
themselves quoted. Use OpamStd.String.split_quoted instead. However,
this causes superfluous quotes in variables to be removed.

Note that this fixes the MANPATH issue of ocaml#5838 but the quoting-handling
is still broken.
rjbou pushed a commit to rjbou/opam that referenced this issue May 10, 2024
When reverting [FOO += "value"], it is necessary to determine what to do
if "value" is of the form "<value1><sep><value2>". Because FOO itself
will have been split at <sep>, we will never match
"<value1><sep><value2>" so it is necessary to split value at <sep> as
well (this is the crux of ocaml#4861).

However, the interaction of this and x-env-path-rewrite is incorrect. If
x-env-path-rewrite has [FOO false] (the `norewrite case), then the value
must be split according to the default interpretation of FOO (which is
how it was added).

If x-env-path-rewrite has a rewrite rule for FOO, then opam has already
assumed that value refers to a single directory (the host/target and
quoting transformation options all being this way).

If x-env-path-rewrite does not have any rule (and does have have
[FOO false]) then we are in essence in the backwards-compatible case,
and must split value.

In order to stop poor interaction between this and opam itself
injecting Windows paths, opam's updates in OpamEnv (to MANPATH, etc.)
are explicitly expanded so that in unzip_to they are treated as though
they had an x-env-path-rewrite rule given (and are therefore not split).

Combined with the previous fixes, this addresses ocaml#5838 without
regressing ocaml#4861.
dra27 added a commit to dra27/opam that referenced this issue May 14, 2024
MANPATH should appear in opam env --revert
dra27 added a commit to dra27/opam that referenced this issue May 14, 2024
Existing implementation was not correct w.r.t. separators which were
themselves quoted. Use OpamStd.String.split_quoted instead. However,
this causes superfluous quotes in variables to be removed.

Note that this fixes the MANPATH issue of ocaml#5838 but the quoting-handling
is still broken.
dra27 added a commit to dra27/opam that referenced this issue May 14, 2024
When reverting [FOO += "value"], it is necessary to determine what to do
if "value" is of the form "<value1><sep><value2>". Because FOO itself
will have been split at <sep>, we will never match
"<value1><sep><value2>" so it is necessary to split value at <sep> as
well (this is the crux of ocaml#4861).

However, the interaction of this and x-env-path-rewrite is incorrect. If
x-env-path-rewrite has [FOO false] (the `norewrite case), then the value
must be split according to the default interpretation of FOO (which is
how it was added).

If x-env-path-rewrite has a rewrite rule for FOO, then opam has already
assumed that value refers to a single directory (the host/target and
quoting transformation options all being this way).

If x-env-path-rewrite does not have any rule (and does have have
[FOO false]) then we are in essence in the backwards-compatible case,
and must split value.

In order to stop poor interaction between this and opam itself
injecting Windows paths, opam's updates in OpamEnv (to MANPATH, etc.)
are explicitly expanded so that in unzip_to they are treated as though
they had an x-env-path-rewrite rule given (and are therefore not split).

Combined with the previous fixes, this addresses ocaml#5838 without
regressing ocaml#4861.
dra27 added a commit to dra27/opam that referenced this issue May 14, 2024
Existing implementation was not correct w.r.t. separators which were
themselves quoted. Use OpamStd.String.split_quoted instead. However,
this causes superfluous quotes in variables to be removed.

Note that this fixes the MANPATH issue of ocaml#5838 but the quoting-handling
is still broken.
dra27 added a commit to dra27/opam that referenced this issue May 14, 2024
When reverting [FOO += "value"], it is necessary to determine what to do
if "value" is of the form "<value1><sep><value2>". Because FOO itself
will have been split at <sep>, we will never match
"<value1><sep><value2>" so it is necessary to split value at <sep> as
well (this is the crux of ocaml#4861).

However, the interaction of this and x-env-path-rewrite is incorrect. If
x-env-path-rewrite has [FOO false] (the `norewrite case), then the value
must be split according to the default interpretation of FOO (which is
how it was added).

If x-env-path-rewrite has a rewrite rule for FOO, then opam has already
assumed that value refers to a single directory (the host/target and
quoting transformation options all being this way).

If x-env-path-rewrite does not have any rule (and does have have
[FOO false]) then we are in essence in the backwards-compatible case,
and must split value.

In order to stop poor interaction between this and opam itself
injecting Windows paths, opam's updates in OpamEnv (to MANPATH, etc.)
are explicitly expanded so that in unzip_to they are treated as though
they had an x-env-path-rewrite rule given (and are therefore not split).

Combined with the previous fixes, this addresses ocaml#5838 without
regressing ocaml#4861.
dra27 added a commit to dra27/opam that referenced this issue May 14, 2024
Existing implementation was not correct w.r.t. separators which were
themselves quoted. Use OpamStd.String.split_quoted instead. However,
this causes superfluous quotes in variables to be removed.

Note that this fixes the MANPATH issue of ocaml#5838 but the quoting-handling
is still broken.
dra27 added a commit to dra27/opam that referenced this issue May 14, 2024
When reverting [FOO += "value"], it is necessary to determine what to do
if "value" is of the form "<value1><sep><value2>". Because FOO itself
will have been split at <sep>, we will never match
"<value1><sep><value2>" so it is necessary to split value at <sep> as
well (this is the crux of ocaml#4861).

However, the interaction of this and x-env-path-rewrite is incorrect. If
x-env-path-rewrite has [FOO false] (the `norewrite case), then the value
must be split according to the default interpretation of FOO (which is
how it was added).

If x-env-path-rewrite has a rewrite rule for FOO, then opam has already
assumed that value refers to a single directory (the host/target and
quoting transformation options all being this way).

If x-env-path-rewrite does not have any rule (and does have have
[FOO false]) then we are in essence in the backwards-compatible case,
and must split value.

In order to stop poor interaction between this and opam itself
injecting Windows paths, opam's updates in OpamEnv (to MANPATH, etc.)
are explicitly expanded so that in unzip_to they are treated as though
they had an x-env-path-rewrite rule given (and are therefore not split).

Combined with the previous fixes, this addresses ocaml#5838 without
regressing ocaml#4861.
dra27 added a commit to dra27/opam that referenced this issue May 15, 2024
MANPATH should appear in opam env --revert
dra27 added a commit to dra27/opam that referenced this issue May 15, 2024
Existing implementation was not correct w.r.t. separators which were
themselves quoted. Use OpamStd.String.split_quoted instead. However,
this causes superfluous quotes in variables to be removed.

Note that this fixes the MANPATH issue of ocaml#5838 but the quoting-handling
is still broken.
dra27 added a commit to dra27/opam that referenced this issue May 15, 2024
When reverting [FOO += "value"], it is necessary to determine what to do
if "value" is of the form "<value1><sep><value2>". Because FOO itself
will have been split at <sep>, we will never match
"<value1><sep><value2>" so it is necessary to split value at <sep> as
well (this is the crux of ocaml#4861).

However, the interaction of this and x-env-path-rewrite is incorrect. If
x-env-path-rewrite has [FOO false] (the `norewrite case), then the value
must be split according to the default interpretation of FOO (which is
how it was added).

If x-env-path-rewrite has a rewrite rule for FOO, then opam has already
assumed that value refers to a single directory (the host/target and
quoting transformation options all being this way).

If x-env-path-rewrite does not have any rule (and does have have
[FOO false]) then we are in essence in the backwards-compatible case,
and must split value.

In order to stop poor interaction between this and opam itself
injecting Windows paths, opam's updates in OpamEnv (to MANPATH, etc.)
are explicitly expanded so that in unzip_to they are treated as though
they had an x-env-path-rewrite rule given (and are therefore not split).

Combined with the previous fixes, this addresses ocaml#5838 without
regressing ocaml#4861.
Opam 2.2.0 automation moved this from For beta3 to Done May 15, 2024
@kit-ty-kate kit-ty-kate added this to the 2.2.0~beta3 milestone May 15, 2024
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 a pull request may close this issue.

2 participants