From 57a7b8355aea40daa46493e0e363639c1ec6c4c1 Mon Sep 17 00:00:00 2001 From: David Allsopp Date: Sun, 28 Apr 2024 12:07:54 +0100 Subject: [PATCH] Correct value-splitting w.r.t. x-env-path-rewrite When reverting [FOO += "value"], it is necessary to determine what to do if "value" is of the form "". Because FOO itself will have been split at , we will never match "" so it is necessary to split value at as well (this is the crux of #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 #5838 without regressing #4861. --- master_changes.md | 1 + src/state/opamEnv.ml | 46 +++++++++++++++++++++++++++++------ tests/reftests/env.win32.test | 2 +- 3 files changed, 40 insertions(+), 9 deletions(-) diff --git a/master_changes.md b/master_changes.md index 35003ebd711..0be5b92b3a2 100644 --- a/master_changes.md +++ b/master_changes.md @@ -67,6 +67,7 @@ users) * [BUG] Fix reverting of environment variables, principally on Windows [#5935 @dra27 fix #5838] * [BUG] Fix splitting environment variables [#5935 @dra27] * [BUG] When opam creates an empty variable then appends/prepends a value, ensure no additional separator is added [#5935 @dra27 - fix #5925] + * [BUG] Fix `x-env-path-rewrite` splitting of values when reverting [#5935 @dra27 - fix #5838] ## Opamfile diff --git a/src/state/opamEnv.ml b/src/state/opamEnv.ml index b9293ad7300..846f215437f 100644 --- a/src/state/opamEnv.ml +++ b/src/state/opamEnv.ml @@ -262,7 +262,29 @@ let unzip_to ~sepfmt var elt current = if String.equal elt "" then [{ tr_entry = ""; tr_raw = ""; tr_sep = separator_char_for ~sepfmt var }] - else split_var ~sepfmt var elt + else + match sepfmt with + | `norewrite -> + (* Given FOO += "", then even with + `norewrite it is necessary to split the value as FOO itself + will have been split - i.e. if we don't split elt here then + it cannot be reverted if it contains multiple directories + (which would regression #4861) *) + let sepfmt = `rewrite_default (var :> string) in + split_var ~sepfmt var elt + | `rewrite_default _ -> + (* If no rewrite has been specified at all, then split elt as, + again, #4861 would be regressed otherwise. *) + split_var ~sepfmt var elt + | `rewrite _ -> + (* If a rewrite rule _is_ in effect, then opam 2.2's limited + (but compatible) semantics for setenv and build-env mean that + we're _assuming_ that elt only contains a single path. This + should be addressed in opam 3.0 by having a somewhat richer + syntax for environment changes to make clear the "type" of + the value in FOO += "bar". *) + [{ tr_entry = elt; tr_raw = elt; + tr_sep = separator_char_for ~sepfmt var }] in match elts with | [] -> invalid_arg "OpamEnv.unzip_to" @@ -566,6 +588,14 @@ let env_expansion ?opam st upd = in { upd with envu_value = s } +(* [env_update_resolved_with_default] creates an environment update with a fully + evaluated rewrite rule. It's used internally because the updates in question + are single directories only, which means that the update will then never be + subject to splitting in [unzip_to] *) +let env_update_resolved_with_default ?comment var = + let rewrite = Some (SPF_Resolved (Some (default_sep_fmt_str var))) in + env_update_resolved ?comment ~rewrite var + let compute_updates ?(force_path=false) st = (* Todo: put these back into their packages! let perl5 = OpamPackage.Name.of_string "perl5" in @@ -576,18 +606,18 @@ let compute_updates ?(force_path=false) st = OpamPath.Switch.bin st.switch_global.root st.switch st.switch_config in let path = - env_update_resolved "PATH" + env_update_resolved_with_default "PATH" (if force_path then PlusEq else EqPlusEq) (OpamFilename.Dir.to_string bindir) ~comment:("Binary dir for opam switch "^OpamSwitch.to_string st.switch) - in + in let man_path = let open OpamStd.Sys in match os () with | OpenBSD | NetBSD | FreeBSD | Darwin | DragonFly -> [] (* MANPATH is a global override on those, so disabled for now *) | _ -> - [ env_update_resolved "MANPATH" EqColon + [ env_update_resolved_with_default "MANPATH" EqColon (OpamFilename.Dir.to_string (OpamPath.Switch.man_dir st.switch_global.root st.switch st.switch_config)) @@ -595,7 +625,7 @@ let compute_updates ?(force_path=false) st = ] in let switch_env = - (env_update_resolved "OPAM_SWITCH_PREFIX" Eq + (env_update_resolved_with_default "OPAM_SWITCH_PREFIX" Eq (OpamFilename.Dir.to_string (OpamPath.Switch.root st.switch_global.root st.switch)) ~comment:"Prefix of the current opam switch") @@ -618,14 +648,14 @@ let compute_updates ?(force_path=false) st = let updates_common ~set_opamroot ~set_opamswitch root switch = let root = if set_opamroot then - [ env_update_resolved "OPAMROOT" Eq + [ env_update_resolved_with_default "OPAMROOT" Eq (OpamFilename.Dir.to_string root) ~comment:"Opam root in use" ] else [] in let switch = if set_opamswitch then - [ env_update_resolved "OPAMSWITCH" Eq + [ env_update_resolved_with_default "OPAMSWITCH" Eq (OpamSwitch.to_string switch) ] else [] in root @ switch @@ -745,7 +775,7 @@ let switch_path_update ~force_path root switch = (OpamStateConfig.Switch.safe_load_t ~lock_kind:`Lock_read root switch) in - [ env_update_resolved "PATH" + [ env_update_resolved_with_default "PATH" (if force_path then PlusEq else EqPlusEq) (OpamFilename.Dir.to_string bindir) ~comment:"Current opam switch binary dir" ] diff --git a/tests/reftests/env.win32.test b/tests/reftests/env.win32.test index 76e3571377f..539a10c408d 100644 --- a/tests/reftests/env.win32.test +++ b/tests/reftests/env.win32.test @@ -26,11 +26,11 @@ Done. set "MANPATH=:"${BASEDIR}/OPAM/rewriting/man"" set PATH=${BASEDIR}/OPAM/rewriting/bin;C:\Devel\bin1;C:\Devel\bin2;"C:\Devel\bin3;";C:\Devel\bin4;ZZZ:\; ### opam exec -- opam env --shell=cmd --revert | grep 'ZZZ:' | 'ZZZ:\\.*' -> 'ZZZ:\' | 'set "P' -> 'set P' -set PATH=C:\Devel\bin1;C:\Devel\bin2;"C:\Devel\bin3;";C:\Devel\bin4;ZZZ:\ ### : Test for #5838 ### opam env | grep MANPATH MANPATH=':"${BASEDIR}/OPAM/rewriting/man"'; export MANPATH; ### opam exec -- opam env --revert | grep MANPATH +MANPATH=''; export MANPATH; ### : Tests forward and backslash rewriting on revert ### : This sequence of updates is what presently happens with the compiler ###