opam icon indicating copy to clipboard operation
opam copied to clipboard

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

Open dra27 opened this issue 1 year ago • 1 comments

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 avatar Feb 14 '24 17:02 dra27

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

dra27 avatar Feb 14 '24 17:02 dra27