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.

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

Add the bin directory under the local switch to the system path #130

Closed
wants to merge 3 commits into from

Conversation

smorimoto
Copy link
Member

Closes #70

@smorimoto smorimoto added the enhancement New feature or request label Jun 24, 2021
@smorimoto smorimoto requested a review from dra27 June 24, 2021 19:30
@smorimoto smorimoto force-pushed the bin-path branch 2 times, most recently from 2a1f241 to de189dd Compare June 24, 2021 19:38
Signed-off-by: Sora Morimoto <sora@morimoto.io>
Signed-off-by: Sora Morimoto <sora@morimoto.io>
@smorimoto
Copy link
Member Author

I believe the path to add is enough (<CWD>/_opam/bin), but is there anything I missed?

Signed-off-by: Sora Morimoto <sora@morimoto.io>
@dra27
Copy link
Member

dra27 commented Jun 25, 2021

It's a nice simple idea, but this doesn't work, I'm afraid (it will be brittle):

  • Some time in the next few months, msvc will start adding other directories to PATH (so compiler in PATH won't work)
  • The other environment variables set by opam exec matter - for example, CAML_LD_LIBRARY_PATH in opam-repository's current setup could cause problems

I think it's important if the action exports the environment that it exports the same one as opam exec would give. There are a few solutions which could work:

  • Pick a shell (for example opam env --shell=bash) and parse the output. The annoying thing here is that you have to find the existing value of environment variables to work out the change. For example, on my system:
dra@thor:~$ echo $PATH
/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin:/home/dra/.dotnet/tools
dra@thor:~$ opam env --shell=bash | grep ^PATH
PATH='/home/dra/.opam/dev-4.11/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin:/home/dra/.dotnet/tools'; export PATH;

So you have to post-process the PATH= line to work out that it's really PATH="/home/dra/.opam/dev-4.11/bin:/usr/local/sbin:$PATH" (and remember that switches can append items to variables as well).

  • Parse _opam/.opam-switch/environment. This is probably the best solution right now, though it's tedious to have to parse the file
  • Update opam to support JSON export of the environment updates.

@smorimoto
Copy link
Member Author

Indeed - and I think there is a cleaner solution with opam 2.2, so I'm closing this for now.

@smorimoto smorimoto closed this Jul 12, 2021
@smorimoto smorimoto deleted the bin-path branch July 12, 2021 23:42
@dra27
Copy link
Member

dra27 commented Jul 13, 2021

What's in opam 2.2 to help with this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to add binaries to path
2 participants