Don't source `variables.sh` if `OPAM_SWITCH_PREFIX` is already set by dra27 · Pull Request #6729 · ocaml/opam

dra27 · 2025-10-08T17:23:55Z

I believe the underlying cause of all the various reports we have had over the years of the environment being incorrect (most recently referred to in #6455 (comment)) are because the variables script is not idempotent.

You can see this on Linux easily by starting a subshell:

opam@eaca5b2675c4:~$ echo $MANPATH
:/home/opam/.opam/default/man
opam@eaca5b2675c4:~$ bash -l
opam@eaca5b2675c4:~$ echo $MANPATH
:/home/opam/.opam/default/man:/home/opam/.opam/default/man

or:

Welcome to fish, the friendly interactive shell
Type help for instructions on how to use fish
opam@0ceec1b79104 ~> echo $MANPATH
 /home/opam/.opam/default/man
opam@0ceec1b79104 ~> fish
Welcome to fish, the friendly interactive shell
Type help for instructions on how to use fish
opam@0ceec1b79104 ~> echo $MANPATH
 /home/opam/.opam/default/man /home/opam/.opam/default/man
opam@0ceec1b79104 ~>

What then leads to, for example, seeing multiple switch directories in bin is that any subsequent call to eval $(opam env) and friends will only revert one of those entries, not all of them.

This can be concretely seen on macOS, where the default shell is zsh - if opam has been allowed to update .zshrc, and from a terminal one launches VS Code, a terminal opened in VS Code correctly resources .zshrc and incorrectly ends up with a broken $PATH (you can't see the easy $MANPATH example, because we don't set $MANPATH on macOS).

I think the fix for this trivially simple - if the environment is already set, then the variables should just stop. That means, for example, that if you start a subshell, then you correctly inherit the previous environment, which may even be a different switch. I set it up using this Dockerfile, which runs using bash as the default shell, but for example executes with fish as the shell with docker run --rm -it <sha> fish:

FROM ocaml/opam:ubuntu-24.04-opam
COPY --chmod=755 <<'EOF' /etc/entrypoint.sh
#!/bin/sh
test $# -gt 0 || set -- bash
exec "$@" -l
EOF
ENTRYPOINT ["/etc/entrypoint.sh"]
RUN rm -rf .opam
RUN sudo apt-get update && sudo apt-get install -y fish zsh csh
RUN cat /etc/zsh/newuser.zshrc.recommended > /home/opam/.zshrc
RUN touch /home/opam/.cshrc
#Add opam from this PR to see the fix...
#ADD opam /usr/local/bin/opam
RUN sudo ln -f /usr/bin/opam-2.4 /usr/bin/opam
RUN opam init -a --bare ./opam-repository && opam switch create --empty default
RUN opam init --reinit --shell=zsh -a
RUN opam init --reinit --shell=csh -a
RUN opam init --reinit --shell=fish -a

I've utilised a little trick to allow the init-scripts test on Unix actually to test this. I've also put a test for the behaviour of #6455 at the end, but I haven't fixed that here. I think this fix is worth having in its own right, and is more important than the "I nuked my opam root" fix. However, we should fix that too at some point, because "I nuked my opam root" (which is often a sledgehammer response to a problem) is also "I had multiple terminals open, and I've removed the switch which was activated" (which suffers the same problem - eval $(opam env) won't revert the previous changes.

Fixes #4649
Fixes #6633

dra27 · 2025-10-08T17:26:58Z

While this is in the same area as #6603 (in particular, you can see that a normal fish subshell reads variables.fish where you need to start another login shell for bash - the inconsistency is certainly strange) the fix for this does not lie solely in where we source variables.sh from but also that we should be able to source it multiple times. There's nothing wrong with launching an interactive subshell, after all!

kit-ty-kate

The change seems good overall, however last commit Show the "nuking the root" effect should be included in the second Add a test for variables.sh double-applying commit instead of being its own commit. Otherwise it is a bit confusing. Personally i had a hard time understanding what it was doing and then i understood once i read the second commit again.

Once done and the master_changes updated, lgtm

kit-ty-kate · 2025-10-08T20:44:14Z

tests/reftests/init-scripts.unix.test

+export HOME="$PWD"
+export SHELL="$(command -v sh)"


Suggested change

export HOME="$PWD"

export SHELL="$(command -v sh)"

set -eu

export HOME=$PWD

export SHELL=$(command -v sh)

I get too used to the shellcheck brigade complaining about SC2086 🫣 I don't mind, but this three-line "script" doesn't need set -eu?

the current script no but i'd rather have it to be more future-proof

kit-ty-kate · 2025-10-09T02:40:45Z

src/state/opamEnv.ml


+let abort_if_set var shell =
+  match shell with
+  | SH_zsh | SH_bash | SH_sh -> Printf.sprintf "test -z \"$%s\" || return\n" var


Fish also works with the same syntax

Suggested change

| SH_zsh | SH_bash | SH_sh -> Printf.sprintf "test -z \"$%s\" || return\n" var

| SH_fish | SH_zsh | SH_bash | SH_sh -> Printf.sprintf "test -z \"$%s\" || return\n" var

Aw, I was trying to be all fish-idiomatic, given that we use ; or true in init.fish

both styles seem idiomatic https://fishshell.com/docs/current/language.html#combiners-and-or
but if you prefer using and/or statment, i'd then rather have the same logic as sh/etc. here:

test -z \"$%s\"; or return\n

As a side note, if we were to do the same thing with zsh and bash, the generated code would also look different, probably more like:

[[ $OPAM_SWITCH_PREFIX = "" ]] || return

Oo, the set -eu made me check that again, and it wasn't quite what was meant. The sh version is now technically resistant to set -u as well. fish's variable expansion is different - as far as I can tell, fish doesn't have any equivalent to set -u, so I've not gone with adding the set -q OPAM_SWITCH_PREFIX to test for unset as it doesn't seem to be necessary.

Yeah fish's equivalent to set -u is a long term feature request tracked by fish-shell/fish-shell#4163. Lgtm

dra27 · 2025-10-09T09:15:15Z

Thanks, @kit-ty-kate - I don't understand the request about the last commit. It's not testing the same thing as the second commit, and the test doesn't change by being earlier in the commit series (because it's not fixed by this fix), so I'm not seeing what putting it earlier is helping? I can remove it entirely for now, of course (basically, I wrote the test, started working on the fix, and realised that the fix is more work than I wanted to have to do by next week...!)

dra27 · 2025-10-09T09:32:35Z

#5036 was also an approach for this - however, I think that this technique of simply backing off is actually better, given that it means that that subshells then behave correctly. i.e. variables.sh et al simply ensure that the first environment gets the currently selected switch, subshells then simply inherit the starting environment, as expected.

kit-ty-kate · 2025-10-09T12:00:30Z

tests/reftests/init-scripts.unix.test

@@ -1,12 +1,23 @@
 N0REP0
 ### rm -rf $OPAMROOT
-### opam init -na --bypass-checks --bare REPO --root root


Thinking about it a bit more, i think it might be nice to duplicate this test to another file to make sure we test both -na and -a

Good idea - in fact, in splitting it, it should now run that test on Windows as well.

kit-ty-kate

lgtm ~~modulo some the missing master_changes entries for commits 1, 2 and 4~~

csh requires shell variables to be initialised when used with +=, etc. but this isn't necessary for unconditional assignment.

Environment variables registered in variables.sh remain even on opam env for a different switch.

dra27 added this to the 2.5.0~alpha1 milestone Oct 8, 2025

dra27 added KIND: BUG PRIORITY AREA: ENV labels Oct 8, 2025

kit-ty-kate reviewed Oct 9, 2025

View reviewed changes

dra27 force-pushed the init-variables branch from 0b1df8d to c4e412b Compare October 9, 2025 09:29

kit-ty-kate reviewed Oct 9, 2025

View reviewed changes

dra27 force-pushed the init-variables branch from c4e412b to 89e8dcb Compare October 10, 2025 18:28

kit-ty-kate approved these changes Oct 10, 2025

View reviewed changes

kit-ty-kate requested a review from rjbou October 10, 2025 21:18

kit-ty-kate force-pushed the init-variables branch 2 times, most recently from b6d16f6 to 3cbe335 Compare October 13, 2025 18:16

dra27 force-pushed the init-variables branch from 3cbe335 to 4e8ec3d Compare October 14, 2025 01:29

rjbou mentioned this pull request Oct 14, 2025

init scripts: revert environment before settings new one, on session loading #5036

Closed

kit-ty-kate force-pushed the init-variables branch 2 times, most recently from aca4610 to f298e9f Compare October 16, 2025 00:55

Don't unnecessarily initialise csh variables

e16b84b

csh requires shell variables to be initialised when used with +=, etc. but this isn't necessary for unconditional assignment.

kit-ty-kate force-pushed the init-variables branch 3 times, most recently from ce15291 to 3920c44 Compare October 16, 2025 12:17

dra27 added 3 commits October 16, 2025 13:19

Add a test for variables.sh double-applying

3fa4ac7

Make variables.* back off if already set

97adba1

Add a test showing the "nuking the root" effect

cc8e824

Environment variables registered in variables.sh remain even on opam env for a different switch.

kit-ty-kate force-pushed the init-variables branch from 3920c44 to cc8e824 Compare October 16, 2025 12:20

rjbou approved these changes Oct 16, 2025

View reviewed changes

rjbou merged commit 8510bce into ocaml:master Oct 16, 2025
46 checks passed

	\| SH_zsh \| SH_bash \| SH_sh -> Printf.sprintf "test -z \"$%s\" \|\| return\n" var
	\| SH_fish \| SH_zsh \| SH_bash \| SH_sh -> Printf.sprintf "test -z \"$%s\" \|\| return\n" var

Conversation

dra27 commented Oct 8, 2025 • edited by kit-ty-kate Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dra27 commented Oct 8, 2025

Uh oh!

kit-ty-kate left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kit-ty-kate Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dra27 commented Oct 9, 2025

Uh oh!

dra27 commented Oct 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kit-ty-kate left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dra27 commented Oct 8, 2025 •

edited by kit-ty-kate

Loading

kit-ty-kate left a comment •

edited

Loading

kit-ty-kate Oct 9, 2025 •

edited

Loading

kit-ty-kate left a comment •

edited

Loading