-
-
Notifications
You must be signed in to change notification settings - Fork 77
fix: filter out RELEASE_ROOT from PATH instead of running a login shell #344
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
fix: filter out RELEASE_ROOT from PATH instead of running a login shell #344
Conversation
|
Would you be able to point me to documentation as to how the RELEASE_ROOT variable works and what it does? |
|
Sure! It looks like it's part of the ERTS release machinery. In the Mix docs:
Note that "can only be read" here means "not written" (as opposed to "can be read only here"). That threw me at first glance. It looks like it's set in SELF=$(readlink_f "$0")
RELEASE_ROOT="$(CDPATH='' cd "$(dirname "$SELF")/.." && pwd -P)"So And the reason we want to use it for filtering is that PATH gets set like this: The BINDIR="$(cd "$(dirname "$SELF")" && pwd -P)"So And then calls So I think what's happening is that when invoking the project Elixir, it's seeing the wrong |
We cannot rely on the presence of SHELL or the behaviour of a login shell because these things are often not available inside containers.
bb0285a to
0ec44ad
Compare
doorgan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this with mise and asdf on macos and it is working. It's essentially the same approach I used to fix it for windows, but I no longer recall if or why it didn't work on unix back when I first was figuring this out months ago.
At this point I think getting rid of the release root in the path is the way to go but I'd like someone using nix/fish/nushell to try this too before merging
|
@doorgan there were a number of release related env vars id filter out in next ls, but I caught those really early on for some reason. I figured the lexical implementation just worked so didn't mention it, but we might want to crib those or at least take a look. |
|
Tested on Fish (MacOS, Mise) and seems to work fine. |
|
@mhanberg these are the ones NextLS gets rid of: |
|
We can just do release root if that's what your research indicates |
|
So can we actually just delete that env var or do we have to actually filter paths that have that root as a prefix? |
|
Sorry I misspoke, I was thinking of a different but related issue.
When finding the elixir executable though, the So there's two related issues:
I haven't seen As a side note: in case we fail to find any elixir executable, the |
|
So this is good to merge as is? |
|
Yes I think so, I just wanted to see if it was working in other environments too since this whole module is very finicky. It looks like it does. |
|
❤️ |
For anyone running the LSP in a dev container or otherwise simplified environment, we cannot rely on the presence of SHELL or the behaviour of a login shell to provide a clean PATH. I propose we instead filter out the RELEASE_ROOT from PATH, which seems to adequately address the issue of using the correct paths for ASDF/mise.