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.

Sign up for GitHub

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

Revert environment variables instead of setting them to an empty string #5039

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Revert environment variables instead of setting them to an empty string #5039

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9ddf813
update env reftest with shell syntax for `opam env'
rjbou Feb 3, 2022
bdda7fb
revert env: unset environment variables instead of setting them to an…
rjbou Feb 3, 2022
acd307e
update env reftest
rjbou Feb 3, 2022
ee5a9ed
update changes
rjbou Feb 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions master_changes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,8 @@ users)
* [BUG] Fix directory display in dev mode [#5102 @rjbou]
* Download source even if no switch is set [#4850 @rjbou @zapashcanon - fix #4809]
* [NEW] Add `--no-switch` option [#4850 @rjbou - fix #4858]
## Env
* Revert environment variables instead of setting them to an empty string [#5039 @rjbou]

## Lint
* W68: add warning for missing license field [#4766 @kit-ty-kate - partial fix #4598]
Expand Down
24 changes: 16 additions & 8 deletions src/client/opamConfigCommand.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,13 @@ let rec print_env = function
if OpamConsole.verbose () then
OpamStd.Option.iter (OpamConsole.msg ": %s;\n") comment;
if not (List.exists (fun (k1, _, _) -> k = k1) r) || OpamConsole.verbose ()
then (
then
let v' = possibly_unix_path_env_value k v in
OpamConsole.msg "%s='%s'; export %s;\n"
k (OpamStd.Env.escape_single_quotes v') k);
if String.equal v' "" then
OpamConsole.msg "unset %s;\n" k
else
OpamConsole.msg "%s='%s'; export %s;\n"
k (OpamStd.Env.escape_single_quotes v') k;
print_env r

let rec print_csh_env = function
Expand All @@ -84,10 +87,13 @@ let rec print_csh_env = function
if OpamConsole.verbose () then
OpamStd.Option.iter (OpamConsole.msg ": %s;\n") comment;
if not (List.exists (fun (k1, _, _) -> k = k1) r) || OpamConsole.verbose ()
then (
then
let v' = possibly_unix_path_env_value k v in
OpamConsole.msg "setenv %s '%s';\n"
k (OpamStd.Env.escape_single_quotes v'));
if String.equal v' "" then
OpamConsole.msg "unsetenv %s;\n" k
else
OpamConsole.msg "setenv %s '%s';\n"
k (OpamStd.Env.escape_single_quotes v');
print_csh_env r

let rec print_pwsh_env = function
Expand Down Expand Up @@ -137,6 +143,9 @@ let print_sexp_env env =
let rec print_fish_env env =
let set_arr_cmd ?(modf=fun x -> x) k v =
let v = modf @@ OpamStd.String.split v ':' in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Academic, but this should hopefully fix the tests:

Suggested change
let v = modf @@ OpamStd.String.split v ':' in
let v = modf @@ OpamStd.String.split v OpamStd.Sys.path_sep in

if v = [] then
OpamConsole.msg "set -ge %s;\n" k
else
Comment on lines +146 to +148
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to check for fish if it's ok to unset manpath

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in fact, it seems that it fixes it

OpamConsole.msg "set -gx %s %s;\n" k
(OpamStd.List.concat_map " "
(fun v ->
Expand Down Expand Up @@ -170,8 +179,7 @@ let rec print_fish_env env =
| "MANPATH" ->
manpath_cmd v
| _ ->
OpamConsole.msg "set -gx %s '%s';\n"
k (OpamStd.Env.escape_single_quotes ~using_backslashes:true v));
set_arr_cmd k v);
print_fish_env r

let print_eval_env ~csh ~sexp ~fish ~pwsh ~cmd env =
Expand Down
33 changes: 29 additions & 4 deletions tests/reftests/env.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ Done.
### opam env | grep "NV_VARS" | ';' -> ':'
NV_VARS='${BASEDIR}/OPAM/setenv/doc/nv:${BASEDIR}/OPAM/setenv/share/nv': export NV_VARS:
### opam exec -- opam env --revert | grep "NV_VARS" | ';' -> ':'
NV_VARS='': export NV_VARS:
unset NV_VARS:
### NV_VARS=/another/path
### opam env | grep "NV_VARS" | ';' -> ':'
NV_VARS='${BASEDIR}/OPAM/setenv/doc/nv:${BASEDIR}/OPAM/setenv/share/nv:/another/path': export NV_VARS:
Expand Down Expand Up @@ -43,9 +43,9 @@ NV_VARS='hej!!'; export NV_VARS;
### opam exec -- opam env | grep "^NV_VARS|^OPAM_SWITCH_PREFIX|${OPAM}"
OPAM_SWITCH_PREFIX='${BASEDIR}/OPAM/conffile'; export OPAM_SWITCH_PREFIX;
NV_VARS='hej!!'; export NV_VARS;
### opam exec -- opam env --revert | grep "^NV_VARS|^OPAM_SWITCH_PREFIX|${OPAM}"
OPAM_SWITCH_PREFIX=''; export OPAM_SWITCH_PREFIX;
NV_VARS=''; export NV_VARS;
### opam exec -- opam env --revert | grep "NV_VARS|OPAM_SWITCH_PREFIX|${OPAM}"
unset OPAM_SWITCH_PREFIX;
unset NV_VARS;
### opam exec -- env | grep '^NV_VARS|^OPAM_SWITCH_PREFIX|${OPAM}'
NV_VARS=hej!!
OPAM=${OPAM}
Expand All @@ -71,3 +71,28 @@ Switch invariant: ["nv"]
-> installed nv.1
Done.
# Run eval $(opam env '--root=${BASEDIR}/root 2' '--switch=${BASEDIR}/switch w spaces') to update the current shell environment
### : test all shell env & reverts syntax
### opam env --shell=bash | grep -v PATH
OPAM_SWITCH_PREFIX='${BASEDIR}/OPAM/conffile'; export OPAM_SWITCH_PREFIX;
NV_VARS='hej!!'; export NV_VARS;
### opam env --shell=zsh | grep -v PATH
OPAM_SWITCH_PREFIX='${BASEDIR}/OPAM/conffile'; export OPAM_SWITCH_PREFIX;
NV_VARS='hej!!'; export NV_VARS;
### opam env --shell=fish | grep -v PATH
set -gx OPAM_SWITCH_PREFIX '${BASEDIR}/OPAM/conffile';
set -gx NV_VARS 'hej!!';
### opam env --shell=csh | grep -v PATH
setenv OPAM_SWITCH_PREFIX '${BASEDIR}/OPAM/conffile';
setenv NV_VARS 'hej!!';
### opam exec -- opam env --revert --shell=bash | grep -v PATH
unset OPAM_SWITCH_PREFIX;
unset NV_VARS;
### opam exec -- opam env --revert --shell=zsh | grep -v PATH
unset OPAM_SWITCH_PREFIX;
unset NV_VARS;
### opam exec -- opam env --revert --shell=fish | grep -v PATH
set -ge OPAM_SWITCH_PREFIX;
set -ge NV_VARS;
### opam exec -- opam env --revert --shell=csh | grep -v PATH
unsetenv OPAM_SWITCH_PREFIX;
unsetenv NV_VARS;