Skip to content

Conversation

@gridbugs
Copy link
Contributor

@gridbugs gridbugs commented Aug 12, 2025

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

@gridbugs gridbugs force-pushed the fix-dune-path-in-nested-shells branch 7 times, most recently from 0d530f5 to 51f3e01 Compare August 12, 2025 06:25
@gridbugs gridbugs requested review from Sudha247 and shonfeder August 12, 2025 06:32
@gridbugs gridbugs marked this pull request as ready for review August 12, 2025 06:32
@gridbugs gridbugs force-pushed the fix-dune-path-in-nested-shells branch 3 times, most recently from 505f562 to 6804243 Compare August 12, 2025 06:43
@gridbugs
Copy link
Contributor Author

Fixes #10

@gridbugs gridbugs force-pushed the fix-dune-path-in-nested-shells branch 11 times, most recently from c1525e4 to 363aeaa Compare August 13, 2025 04:25
@gridbugs
Copy link
Contributor Author

gridbugs commented Aug 13, 2025

@Sudha247 this is now ready for review. Heads up that I replaced the awk script we wrote with a sed script. The reason is that the pattern the script needs to recognize is a path like "/home/user/.opam/bin", and the slashes in the path interfere with awk's regex syntax (/pattern/ { ... }). The benefit of sed is that the pattern delimiter character does not need to be slash, which makes it much easier to match patterns which are file paths.

The sed script is sed "1,\#^$__DUNE_ROOT/bin\$# { \#^$HOME/.opam#d; }". It uses # as a pattern delimiter. It applies logic on the range of lines from its input 1,\#^$__DUNE_ROOT/bin\$#, which refers to the range from line 1 to the first occurrence of the value of $__DUNE_ROOT/bin ($__DUNE_ROOT is expanded as a shell variable before sed sees it) which is the dune binary distro's bin directory. The range is inclusive at both ends. For lines within the range, sed runs \#^$HOME/.opam#d. This has 2 parts. \#^$HOME/.opam# is another pattern matching paths that look like opam switch bin dirs (again, $HOME is expanded as a shell variable before sed sees it so it just sees the variable's value). The d is the "delete" command which omits matching lines from output. Sed's default behaviour is to print each line, so lines outside the range, and lines inside the range which don't look like opam bin dirs are printed.

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 <[email protected]>
@gridbugs gridbugs force-pushed the fix-dune-path-in-nested-shells branch from 363aeaa to f3669b4 Compare August 15, 2025 06:03
Copy link
Contributor

@Sudha247 Sudha247 left a comment

Choose a reason for hiding this comment

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

Looks good to me! One question below.

This is a very impressive script and I learnt a lot of new stuff about shell scripting by reviewing these PRs! Thanks for the work. However, I do wonder about the maintenance of the env scripts - we have the same logic coded to four different shells. Future changes need to be cognisant of the fact that the changes should occur in all of the env scripts. Would we benefit from fleshing this out in a meta script and importing it to the various configs? (this is not to block the PR)

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"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is __DUNE_SETUP_STATE not being exported here because it's already in success state, and we want to continue it being in the same state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep exactly

@gridbugs
Copy link
Contributor Author

However, I do wonder about the maintenance of the env scripts - we have the same logic coded to four different shells. Future changes need to be cognisant of the fact that the changes should occur in all of the env scripts. Would we benefit from fleshing this out in a meta script and importing it to the various configs? (this is not to block the PR)

It's tricky because these files are sourced in the user's shell config, so they have to be written in the user's shell language. If there was some common language that could be compiled to bash, zsh, fish, and posix shell but which also allowed us to add additional logic so the completion script could be loaded for bash and zsh but not other shells then that would let us centralize the logic. However I'm not aware of such a language. For the time being I think we should rely on the tests and code review process to ensure that changes are applied to each file.

@gridbugs gridbugs merged commit 2f9792e into ocaml-dune:main Aug 18, 2025
6 checks passed
@gridbugs gridbugs deleted the fix-dune-path-in-nested-shells branch August 18, 2025 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants