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

dra27 · 2024-02-14T17:42:43Z

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

The text was updated successfully, but these errors were encountered:

dra27 · 2024-02-14T17:48:58Z

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).

MANPATH should appear in opam env --revert

The cases of this match were the wrong way around. Combined with the previous fixes, this finally addressed ocaml#5838.

MANPATH should appear in opam env --revert

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.

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.

MANPATH should appear in opam env --revert

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.

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.

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.

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.

MANPATH should appear in opam env --revert

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.

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.

MANPATH should appear in opam env --revert

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.

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.

MANPATH should appear in opam env --revert

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.

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.

MANPATH should appear in opam env --revert

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.

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.

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.

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.

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.

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.

MANPATH should appear in opam env --revert

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.

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 the KIND: BUG label Feb 14, 2024

dra27 self-assigned this Feb 14, 2024

dra27 mentioned this issue Apr 24, 2024

Fixes to reversal of environment updates #5935

Merged

2 tasks

dra27 linked a pull request Apr 24, 2024 that will close this issue

Fixes to reversal of environment updates #5935

Merged

2 tasks

dra27 added a commit to dra27/opam that referenced this issue Apr 25, 2024

Add test for ocaml#5838

99db19e

MANPATH should appear in opam env --revert

dra27 added a commit to dra27/opam that referenced this issue Apr 29, 2024

Add test for ocaml#5838

cec8258

MANPATH should appear in opam env --revert

dra27 added a commit to dra27/opam that referenced this issue Apr 29, 2024

Fix wrong split function

2b4b48b

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

Add test for ocaml#5838

b5f2d34

MANPATH should appear in opam env --revert

rjbou pushed a commit to rjbou/opam that referenced this issue May 10, 2024

Add test for ocaml#5838

3cbdf15

MANPATH should appear in opam env --revert

rjbou pushed a commit to rjbou/opam that referenced this issue May 10, 2024

Add test for ocaml#5838

c701728

MANPATH should appear in opam env --revert

rjbou pushed a commit to rjbou/opam that referenced this issue May 10, 2024

Add test for ocaml#5838

b39b2de

MANPATH should appear in opam env --revert

rjbou pushed a commit to rjbou/opam that referenced this issue May 10, 2024

Add test for ocaml#5838

c1402f7

MANPATH should appear in opam env --revert

dra27 added a commit to dra27/opam that referenced this issue May 14, 2024

Add test for ocaml#5838

8a5d3f6

MANPATH should appear in opam env --revert

dra27 added a commit to dra27/opam that referenced this issue May 15, 2024

Add test for ocaml#5838

f16bbc1

MANPATH should appear in opam env --revert

kit-ty-kate closed this as completed in #5935 May 15, 2024

kit-ty-kate added this to the 2.2.0~beta3 milestone May 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dra27 commented Feb 14, 2024

dra27 commented Feb 14, 2024

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

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

Comments

dra27 commented Feb 14, 2024

dra27 commented Feb 14, 2024