Fix paths in env scripts in nested shells (#13)

gridbugs · web-flow · commit 2f9792e357c0 · 2025-08-18T22:08:31.000+10:00
When starting a shell from an existing shell where the shell config has
already been sourced, or when sourcing a shell config for a second time
within the same shell session, any opam setup in the shell config will
re-insert the bin directory from the current opam switch at the
beginning of PATH.

Dune's setup logic doesn't re-insert directories already present in PATH
as dune may share its bin directory with other programs reliant on the
current PATH order. Thus opam's re-initialization will result in any
dune executable in the current opam switch taking precedence over the
binary distro.

To prevent this, the __dune_env function detects when it's called
subsequent times in a shell session, and if it succeeded the first time
in correctly setting up the environment for the dune binary distribution
will remove any opam directories from PATH which precede the first
instance of location of the dune binary distro in PATH. Opam bin
directories following the location of the binary distro in PATH, as well
as all other directories in PATH are left unchanged.

Signed-off-by: Stephen Sherratt &lt;stephen@sherra.tt&gt;
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
@@ -2,8 +2,8 @@ name: test
 on: [push, pull_request]
 jobs:
 
-  test-make-tarball:
-    name: "Test that we can build the binary distro and package it as a tarball"
+  test-env-scripts:
+    name: "Test the env scripts packaged in the dune binary distro"
     runs-on: ${{ matrix.os }}
     strategy:
       matrix:
@@ -33,12 +33,56 @@ jobs:
         run: ./make_tarball.sh $NAME ${{ matrix.output }}
       - name: "Extract the tarball"
         run: tar xf $NAME.tar.gz
-      - name: "Test that we can env scripts and the dune executable is in the expected place"
+      - name: "Test that we can run the env scripts and the dune executable is in the expected place"
         run: |
           bash -c '. $NAME/share/dune/env/env.bash; __dune_env $PWD/$NAME; test $(which dune) = $PWD/$NAME/bin/dune'
           zsh -c  '. $NAME/share/dune/env/env.zsh;  __dune_env $PWD/$NAME; test $(which dune) = $PWD/$NAME/bin/dune'
-          fish -c '. $NAME/share/dune/env/env.fish; __dune_env $PWD/$NAME; test $(which dune) = $PWD/$NAME/bin/dune'
           sh -c   '. $NAME/share/dune/env/env.sh;   __dune_env $PWD/$NAME; test $(which dune) = $PWD/$NAME/bin/dune'
+          fish -c '. $NAME/share/dune/env/env.fish; __dune_env $PWD/$NAME; test $(which dune) = $PWD/$NAME/bin/dune'
+      - name: "Exercise that running __dune_env twice is benign since some logic only runs the second time they are sourced"
+        run: |
+          bash -c '. $NAME/share/dune/env/env.bash; __dune_env $PWD/$NAME; __dune_env $PWD/$NAME; test $(which dune) = $PWD/$NAME/bin/dune'
+          zsh -c  '. $NAME/share/dune/env/env.zsh;  __dune_env $PWD/$NAME; __dune_env $PWD/$NAME; test $(which dune) = $PWD/$NAME/bin/dune'
+          sh -c   '. $NAME/share/dune/env/env.sh;   __dune_env $PWD/$NAME; __dune_env $PWD/$NAME; test $(which dune) = $PWD/$NAME/bin/dune'
+          fish -c '. $NAME/share/dune/env/env.fish; __dune_env $PWD/$NAME; __dune_env $PWD/$NAME; test $(which dune) = $PWD/$NAME/bin/dune'
+      - name: "Make a fake opam switch and add it to PATH in between calls to __dune_env simulating a shell config with both opam and dune installed"
+        run: |
+          # Make a fake opam installation of dune
+          mkdir -p ~/.opam/default/bin
+          printf '%s\necho "%s"' '#!/bin/sh' 'fake dune' > ~/.opam/default/bin/dune
+          chmod a+x ~/.opam/default/bin/dune
+
+          # Test adding the fake opam switch to the path after __dune_env
+          # causes opam's dune to take precedence, and that calling __dune_env
+          # a second time causes the binary distro's dune to take precedence.
+          bash -c '. $NAME/share/dune/env/env.bash; __dune_env $PWD/$NAME; export PATH=~/.opam/default/bin:$PATH; test $(which dune) = ~/.opam/default/bin/dune'
+          bash -c '. $NAME/share/dune/env/env.bash; __dune_env $PWD/$NAME; export PATH=~/.opam/default/bin:$PATH; __dune_env $PWD/$NAME; test $(which dune) = $PWD/$NAME/bin/dune'
+          zsh -c  '. $NAME/share/dune/env/env.zsh;  __dune_env $PWD/$NAME; export PATH=~/.opam/default/bin:$PATH; test $(which dune) = ~/.opam/default/bin/dune'
+          zsh -c  '. $NAME/share/dune/env/env.zsh;  __dune_env $PWD/$NAME; export PATH=~/.opam/default/bin:$PATH; __dune_env $PWD/$NAME; test $(which dune) = $PWD/$NAME/bin/dune'
+          sh -c   '. $NAME/share/dune/env/env.sh;   __dune_env $PWD/$NAME; export PATH=~/.opam/default/bin:$PATH; test $(which dune) = ~/.opam/default/bin/dune'
+          sh -c   '. $NAME/share/dune/env/env.sh;   __dune_env $PWD/$NAME; export PATH=~/.opam/default/bin:$PATH; __dune_env $PWD/$NAME; test $(which dune) = $PWD/$NAME/bin/dune'
+          fish -c '. $NAME/share/dune/env/env.fish; __dune_env $PWD/$NAME; fish_add_path --prepend --move --path ~/.opam/default/bin; test $(which dune) = ~/.opam/default/bin/dune'
+          fish -c '. $NAME/share/dune/env/env.fish; __dune_env $PWD/$NAME; fish_add_path --prepend --move --path ~/.opam/default/bin; __dune_env $PWD/$NAME; test $(which dune) = $PWD/$NAME/bin/dune'
+
+      - name: "Test that opam paths later in the PATH variable are preserved by multiple calls to __dune_env"
+        run: |
+          printf '%s\necho "%s"' '#!/bin/sh' 'only in opam bin' > ~/.opam/default/bin/only-in-opam-bin
+          chmod a+x ~/.opam/default/bin/only-in-opam-bin
+          bash -c '. $NAME/share/dune/env/env.bash; export PATH=~/.opam/default/bin:$PATH; __dune_env $PWD/$NAME; __dune_env $PWD/$NAME; only-in-opam-bin'
+          zsh -c  '. $NAME/share/dune/env/env.zsh;  export PATH=~/.opam/default/bin:$PATH; __dune_env $PWD/$NAME; __dune_env $PWD/$NAME; only-in-opam-bin'
+          sh -c   '. $NAME/share/dune/env/env.sh;   export PATH=~/.opam/default/bin:$PATH; __dune_env $PWD/$NAME; __dune_env $PWD/$NAME; only-in-opam-bin'
+          fish -c '. $NAME/share/dune/env/env.fish; fish_add_path --prepend --move --path ~/.opam/default/bin; __dune_env $PWD/$NAME; __dune_env $PWD/$NAME; only-in-opam-bin'
+
+      - name: "Test that non-opam changes to PATH between two calls to __dune_env have their effect preserved"
+        run: |
+          # Simulate the user setting up a custom version of dune (e.g. what a dune developer might do to try out local changes)
+          mkdir -p ~/bin
+          printf '%s\necho "%s"' '#!/bin/sh' 'fake dune' > ~/bin/dune
+          chmod a+x ~/bin/dune
+          bash -c '. $NAME/share/dune/env/env.bash; __dune_env $PWD/$NAME; export PATH=~/bin:$PATH; __dune_env $PWD/$NAME; test $(which dune) = ~/bin/dune'
+          zsh  -c '. $NAME/share/dune/env/env.zsh ; __dune_env $PWD/$NAME; export PATH=~/bin:$PATH; __dune_env $PWD/$NAME; test $(which dune) = ~/bin/dune'
+          sh   -c '. $NAME/share/dune/env/env.sh;   __dune_env $PWD/$NAME; export PATH=~/bin:$PATH; __dune_env $PWD/$NAME; test $(which dune) = ~/bin/dune'
+          fish -c '. $NAME/share/dune/env/env.fish; __dune_env $PWD/$NAME; fish_add_path --prepend --move --path ~/bin; __dune_env $PWD/$NAME; test $(which dune) = ~/bin/dune'
 
 
   test-make-tarball-shellcheck:
@@ -65,5 +109,4 @@ jobs:
           sudo apt-get install -y shellcheck
       - name: "Run shellcheck on the shell env script"
         run: |
-          # exclude warning for sourcing missing external file
           shellcheck extra/share/dune/env/env.sh extra/share/dune/env/env.bash
diff --git a/extra/share/dune/env/env.bash b/extra/share/dune/env/env.bash
@@ -1,30 +1,105 @@
 #!/usr/bin/env bash
 
+# Equivalent to `which dune`, but `which` might not be installed. Bash's `type`
+# supports -P which acts like `which` but is guaranteed to be installed, but
+# older versions of bash might not support -P so we'll err on the side of
+# caution and implement it here in the interest of portability.
+__dune_which() {
+  echo "$PATH" | \
+    tr ':' '\n' |\
+    while read -r entry; do
+      if test -x "$entry/dune"; then
+        echo "$entry/dune"
+        return 0
+      fi
+    done
+    return 1
+}
+
+export __DUNE_SETUP_STATE="${__DUNE_SETUP_STATE:-incomplete}"
+
 __dune_env() {
   if [ "$#" != "1" ]; then
     echo "__dune_env expected 1 argument, got $#"
     return
   fi
   local ROOT="$1"
 
-  # Add dune to PATH unless it's already present.
-  # Affix colons on either side of $PATH to simplify matching (based on
-  # rustup's env script).
-  case :"$PATH": in
-    *:"$ROOT/bin":*)
-      # Do nothing since the bin directory is already in PATH.
+  case "$__DUNE_SETUP_STATE" in
+    incomplete)
+      # This is the first time __dune_env has been called, so attempt to set up
+      # the environment for dune and record in the global variable
+      # __DUNE_SETUP_STATE whether or not it succeeded.
+
+      # Add dune to PATH unless it's already present.
+      # Affix colons on either side of $PATH to simplify matching (based on
+      # rustup's env script).
+      case :"$PATH": in
+        *:"$ROOT/bin":*)
+          # Do nothing since the bin directory is already in PATH.
+          ;;
+        *)
+          export PATH="$ROOT/bin:$PATH"
+          ;;
+      esac
+
+      if [ "$(__dune_which)" = "$ROOT/bin/dune" ]; then
+        export __DUNE_SETUP_STATE=success
+
+        # Only load completions if the shell is interactive.
+        if [ -t 0 ]; then
+          # Load bash completions for dune.
+          # Suppress warning from shellcheck as it can't see the completion script.
+          # shellcheck disable=SC1091
+          . "$ROOT"/share/bash-completion/completions/dune
+        fi
+      else
+        # Despite modifying the environment, running `dune` would resolve to
+        # the wrong dune instance. This can happen if the bin directory
+        # containing our dune was already present in PATH behind the bin
+        # directory of the default opam switch.
+        # TODO Possibly print a warning/hint in this case to help users fix
+        # their environment.
+        export __DUNE_SETUP_STATE=failure
+      fi
+      ;;
+    success)
+      # This is at least the second time this function was called in the
+      # current environment (possibly by the user sourcing their shell config
+      # or nesting shell sessions), and the previous time it was called it
+      # successfully modified the environment to give precedence to our dune
+      # executable. check that our dune still has precedence, and attempt to
+      # undo any opam-specific path shenanigans that have taken place since the
+      # last time this function was called.
+      if [ "$(__dune_which)" != "$ROOT/bin/dune" ]; then
+        case :"$PATH": in
+          *:"$ROOT/bin":*)
+            # Remove all opam bin directories from the PATH variable
+            # between the start of the PATH variable and the first
+            # occurrence of the dune binary distro's bin directory.
+            PATH_MAYBE_FIXED=$(echo "$PATH" | \
+              tr ':' '\n' |\
+              sed "1,\#^$ROOT/bin\$# { \#^$HOME/.opam#d; }" |\
+              paste -sd ':' -)
+            # Only commit the change if it actually fixed the problem.
+            if [ "$(PATH=$PATH_MAYBE_FIXED __dune_which)" = "$ROOT/bin/dune" ]; then
+              export PATH="$PATH_MAYBE_FIXED"
+            else
+              # The attempt to fix the PATH variable failed, so give up.
+              export __DUNE_SETUP_STATE=failure
+            fi
+            ;;
+          *)
+            # The dune binary distro is no longer in the PATH variable at
+            # all, so give up.
+            export __DUNE_SETUP_STATE=failure
+            ;;
+        esac
+      fi
       ;;
-    *)
-      # Prepending path in case a system-installed dune needs to be overridden
-      export PATH="$ROOT/bin:$PATH"
+    failure)
+      # A previous attempt at modifying the environment failed, so don't
+      # attempt further environment modifications here.
       ;;
   esac
-
-  # Only load completions if the shell is interactive.
-  if [ -t 0 ]; then
-    # Load bash completions for dune.
-    # Suppress warning from shellcheck as it can't see the completion script.
-    # shellcheck disable=SC1091
-    . "$ROOT"/share/bash-completion/completions/dune
-  fi
 }
diff --git a/extra/share/dune/env/env.fish b/extra/share/dune/env/env.fish
@@ -1,8 +1,63 @@
 #!/usr/bin/env fish
 
+if ! set -q __DUNE_SETUP_STATE
+    set --export __DUNE_SETUP_STATE "incomplete"
+end
+
 function __dune_env
-    set dune_bin_path "$argv[1]/bin"
-    if ! contains "$dune_bin_path" $PATH;
-        fish_add_path --prepend --path "$dune_bin_path"
+    if [ "$(count $argv)" != "1" ]
+        echo "__dune_env expected 1 argument, got $(count $argv)"
+    end
+
+    set --local dune_root "$argv[1]"
+    set --local dune_bin "$dune_root/bin"
+
+    switch "$__DUNE_SETUP_STATE"
+        case incomplete
+            if ! contains "$dune_bin" $PATH; and [ -d "$dune_bin" ]
+                fish_add_path --prepend --path "$dune_bin"
+            end
+            if [ "$(type -P dune)" = "$dune_bin/dune" ]
+                set --export __DUNE_SETUP_STATE "success"
+            else
+                # Despite modifying the environment, running `dune` would resolve to
+                # the wrong dune instance. This can happen if the bin directory
+                # containing our dune was already present in PATH behind the bin
+                # directory of the default opam switch.
+                # TODO Possibly print a warning/hint in this case to help users fix
+                # their environment.
+                set --export __DUNE_SETUP_STATE "failure"
+            end
+        case success
+            # This is at least the second time this function was called in the
+            # current environment (possibly by the user sourcing their shell config
+            # or nesting shell sessions), and the previous time it was called it
+            # successfully modified the environment to give precedence to our dune
+            # executable. check that our dune still has precedence, and attempt to
+            # undo any opam-specific path shenanigans that have taken place since the
+            # last time this function was called.
+            if [ "$(type -P dune)" != "$dune_bin/dune" ]
+                if contains "$dune_bin" $PATH
+                    # Remove all opam bin directories from the PATH variable
+                    # between the start of the PATH variable and the first
+                    # occurrence of the dune binary distro's bin directory.
+                    set --local PATH_maybe_fixed $(printf "%s\n" $PATH | \
+                        sed "1,\#^$dune_bin\$# { \#^$HOME/.opam#d; }")
+                    # Only commit the change if it actually fixed the problem.
+                    if [ "$(PATH=$PATH_maybe_fixed type -P dune)" = "$dune_bin/dune" ]
+                        set --export PATH $PATH_maybe_fixed
+                    else
+                        # The attempt to fix the PATH variable failed, so give up.
+                        set --export __DUNE_SETUP_STATE "failure"
+                    end
+                else
+                    # The dune binary distro is no longer in the PATH variable at
+                    # all, so give up.
+                    set --export __DUNE_SETUP_STATE "failure"
+                end
+            end
+        case failure
+            # A previous attempt at modifying the environment failed, so don't
+            # attempt further environment modifications here.
     end
 end
diff --git a/extra/share/dune/env/env.sh b/extra/share/dune/env/env.sh
@@ -1,22 +1,94 @@
 #!/bin/sh
 
+# Equivalent to `which dune`, but `which` might not be installed.
+__dune_which() {
+  echo "$PATH" | \
+    tr ':' '\n' |\
+    while read -r entry; do
+      if test -x "$entry/dune"; then
+        echo "$entry/dune"
+        return 0
+      fi
+    done
+    return 1
+}
+
+export __DUNE_SETUP_STATE="${__DUNE_SETUP_STATE:-incomplete}"
+
 __dune_env() {
   if [ "$#" != "1" ]; then
     echo "__dune_env expected 1 argument, got $#"
     return
   fi
-  __dune_root="$1"
+  __DUNE_ROOT="$1"
 
-  # Add dune to PATH unless it's already present.
-  # Affix colons on either side of $PATH to simplify matching (based on
-  # rustup's env script).
-  case :"$PATH": in
-    *:"$__dune_root/bin":*)
-      # Do nothing since the bin directory is already in PATH.
+  case "$__DUNE_SETUP_STATE" in
+    incomplete)
+      # This is the first time __dune_env has been called, so attempt to set up
+      # the environment for dune and record in the global variable
+      # __DUNE_SETUP_STATE whether or not it succeeded.
+
+      # Add dune to PATH unless it's already present.
+      # Affix colons on either side of $PATH to simplify matching (based on
+      # rustup's env script).
+      case :"$PATH": in
+        *:"$__DUNE_ROOT/bin":*)
+          # Do nothing since the bin directory is already in PATH.
+          ;;
+        *)
+          export PATH="$__DUNE_ROOT/bin:$PATH"
+          ;;
+      esac
+
+      if [ "$(__dune_which)" = "$__DUNE_ROOT/bin/dune" ]; then
+        export __DUNE_SETUP_STATE=success
+      else
+        # Despite modifying the environment, running `dune` would resolve to
+        # the wrong dune instance. This can happen if the bin directory
+        # containing our dune was already present in PATH behind the bin
+        # directory of the default opam switch.
+        # TODO Possibly print a warning/hint in this case to help users fix
+        # their environment.
+        export __DUNE_SETUP_STATE=failure
+      fi
+      ;;
+    success)
+      # This is at least the second time this function was called in the
+      # current environment (possibly by the user sourcing their shell config
+      # or nesting shell sessions), and the previous time it was called it
+      # successfully modified the environment to give precedence to our dune
+      # executable. check that our dune still has precedence, and attempt to
+      # undo any opam-specific path shenanigans that have taken place since the
+      # last time this function was called.
+      if [ "$(__dune_which)" != "$__DUNE_ROOT/bin/dune" ]; then
+        case :"$PATH": in
+          *:"$__DUNE_ROOT/bin":*)
+            # Remove all opam bin directories from the PATH variable
+            # between the start of the PATH variable and the first
+            # occurrence of the dune binary distro's bin directory.
+            PATH_MAYBE_FIXED=$(echo "$PATH" | \
+              tr ':' '\n' |\
+              sed "1,\#^$__DUNE_ROOT/bin\$# { \#^$HOME/.opam#d; }" |\
+              paste -sd ':' -)
+            # Only commit the change if it actually fixed the problem.
+            if [ "$(PATH=$PATH_MAYBE_FIXED __dune_which)" = "$__DUNE_ROOT/bin/dune" ]; then
+              export PATH="$PATH_MAYBE_FIXED"
+            else
+              # The attempt to fix the PATH variable failed, so give up.
+              export __DUNE_SETUP_STATE=failure
+            fi
+            ;;
+          *)
+            # The dune binary distro is no longer in the PATH variable at
+            # all, so give up.
+            export __DUNE_SETUP_STATE=failure
+            ;;
+        esac
+      fi
       ;;
-    *)
-      # Prepending path in case a system-installed dune needs to be overridden
-      export PATH="$__dune_root/bin:$PATH"
+    failure)
+      # A previous attempt at modifying the environment failed, so don't
+      # attempt further environment modifications here.
       ;;
   esac
 }
diff --git a/extra/share/dune/env/env.zsh b/extra/share/dune/env/env.zsh
