-
Notifications
You must be signed in to change notification settings - Fork 263
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
rtx-activate: less aggressive PATH modifications #863
Comments
I actually just ran into this and as far as I could tell there was no way for me to change this in my own config. The use case I had was chef-workstation and the bundled gems as part of this. By default rtx would try and load gems that were loaded into my default ruby version. I would suspect there are a lot of edge cases here because of the various setups people have. For example, I could see a situation for my own setup where based on the project I was in (some projects actually do manage these gems directly) i would want rtx to manage the dependencies however by default its much simpler to just use the gems bundled with chef-workstation. |
It also messes up any Python virtual environment in cases where I’m not using the plugin’s built-in virtual environment handling. I have a separate listener that reacts to $PWD change and activates the environment if there is one. But even without — if I do it manually — RTX will change $PATH eventually, and virtual environment binaries will fall behind. How eventually? It’s a mystery. It can occur right on the following prompt or after a few commands. I can’t stress enough how often I installed something by PIP as a global package instead of an environment one because of this issue. The obvious solution would be to have the My solution was to write yet another listener that reacts to the prompt event and checks if the first element of $PATH resembles a virtual environment path, then re-activates the environment if necessary. But it’s a crutch solution for sure. |
@Aeron you might have better luck not using |
Well, yeah, but Python is not the only plugin I use. It’s not the “Hey, fix my specific case” demand, by no means. But if it treats $PATH that way, it could be a problem not only for Python’s virtual environment. If RTX needs to listen for shell’s Otherwise, it would be a million specific cases to deal with. And it’s not like RTX has (or should have) a monopoly on the last word for $PATH look. At least, that’s what I think. Yet, in my specific case, it would be wonderful if RTX could respect not only |
@Aeron I'm not 100% but it sounds like you think my plan of inserting runtimes into PATH at the location of the rtx bin would be insufficient and you'd prefer more control via events? I feel like inserting instead of just always prepending to the front would work, though it might in some cases require installing rtx to a dedicated directory (one reason I think it should be behind a config setting of some kind). Let me know if I got what you said correct though, I wasn't 100%. In general, virtualenv stuff needs to be improved. (For users using rtx virtualenv activation as well as those that are not) I know it's a well-liked feature but has notable gaps. |
Oh, no-no, I don’t think your plan would not succeed, but as you said, it is a somewhat challenging task. Events are an option, alternative solution, or option B, perhaps easier and faster to implement. If anything, I agree with you and the general idea. |
I like this solution, it may also be worth considering an addition to |
Python people probably want |
Hey jdx thanks a lot for the work on this issue, this has been a recurring issue for me working with python ! Looks like the path is updated the old way in The following bit of code let new_path = join_paths([installs.clone(), env::PATH.clone()].concat())?
.to_string_lossy()
.to_string(); could be replaced with let mut path_env = PathEnv::from_iter(env::PATH.clone());
for p in installs.clone() {
path_env.add(p);
}
let new_path = path_env.join().to_string_lossy().to_string(); Do you want me to open a PR for this ? |
it won't work because there won't be a shim directory to use |
One idea I had is maybe we could keep track of any paths added to the front since activate was first launched. I think we can compare the difference between env::var("PATH") and env::PRISTINE_ENV["PATH"] which would tell us that |
I tried out that idea I had and it seems to work. Would anyone mind trying out by installing from HEAD? |
With this change, if you manually run PATH="/foo:$PATH" in your shell after activating rtx, rtx will keep "/foo" in front of its own PATH entries. Fixes #863
With this change, if you manually run PATH="/foo:$PATH" in your shell after activating rtx, rtx will keep "/foo" in front of its own PATH entries. Fixes #863
With this change, if you manually run PATH="/foo:$PATH" in your shell after activating rtx, rtx will keep "/foo" in front of its own PATH entries. Fixes #863
you didn't install from HEAD, this hasn't been released yet |
I'm confused, isn't 9160c4f the commit that fixed it ? I used the |
oh apologies. You're running in a new shell and everything? does it seems to work for me in fish at least:
|
I get the same results in zsh too |
make sure where you're calling |
oh are you using shims and not |
Using fish with |
ah yeah and I had shims remaining as a test for the first version of the fix, would that affect it ? |
maybe try removing the shims directory, that might mess it up |
ok so I tried removing the shims directory but it did not change the result. Also adjusted the order of initialization in my config.fish file, and I can see the Also, food for thoughts, the way poetry shell does its thing is by creating a subshell, then sourcing th virtualenv file; could that affect the behaviour of rtx ? |
let's try this to make sure it's working at all:
one thing that might be happening: if something is modifying PATH aside from simply adding something to the front then it won't work. I might need to try messing with poetry here. Also, are you using the automatic venv activation for python or poetry? Or just running |
Was able to reproduce consistently in a VM, here's the log
Please let me know if there is any flaw in my approach |
update: this has been released in 2023.12.25 but it's under experimental so set RTX_EXPERIMENTAL=1 to test it |
Added the export at the beginning, should be enough to enable ?
|
no, it needs to be there when you call |
the way it works is when you call So effectively what this should do is make it so if you do this:
then PATH will be the following:
what might be happening (I need to look), is that poetry is modifying the PATH in a way that changes the things that were set in __RTX_ORIG_PATH. If that does not match the suffix of PATH then rtx will just ignore it and prepend things to the beginning. In other words it might be doing something like this:
in that case, because "C" is added to the end of PATH rtx should generate this with nothing prepended:
That's a use-case I might need to think about a bit more. Maybe instead of saving the entire suffix I could just find the first entry in PATH and prepend things before that. |
Here's the log with experimental activated Log
I've also thrown a dummy value in the path and it was moved behind rtx bins. I've checked the poetry repository and the way We then have the following order for activations
Could it be caused by the fact that |
Ah that's the problem. rtx puts itself before everything that was added after it, but if you call You could try this to make sure:
then it should work as expected I think. Trouble is I'm not really sure what rtx should do differently here. I don't want to reuse an existing __RTX_ORIG_PATH since I actually think it should reset it in most use-cases. Maybe I could add this as an argument to activate so you could do this:
|
So maybe instead of trying to fix my PATH, maybe I should prevent the |
Perhaps following the last fix already committed in and the last idea a solution could be that the rtx activate bash should be this :
INSTEAD OF this :
Notice the if added |
@jylenhofdecat can you explain what the SHLVL check is doing here? It's not making sense at first glance |
Looks like this is the level of nesting for the current shell (doc). So in @jylenhofdecat script, it would only apply the |
I don't think that's what we want. You should be able to run activate at any point. I also think in general it should effectively "reset" when called and make that point where it prepends to PATH even though it doesn't work well in this use case. |
One option is to add a flag to configure whether or not it resets PATH and the prepend variable. That's not hard, but coming up with a good name for this behavior and describing it well definitely is. |
No. When you do "export", variable are pushed to subshell, functions are not, so it is already set, just didn't want to reset it.... because I'm doing a python virtualenv with source .venv/bin/activate in my shell manually and does'nt want it to be overriden on the beginning on my PATH |
given I haven't heard much here I suspect this experiment is working well. I'll probably roll this out to all users and make a flag to force mise to be aggressive in a few days/weeks. It might still have some edge cases like we discussed but I think this logic is still preferable. |
Haven't checked again as I moved to using the virtualenv automatic activation with the python plugin |
Currently, when using
rtx activate
, rtx will prepend PATH entries to the very front of PATH, e.g.:However this might be more aggressive than it should be. This means it's basically impossible to override rtx and allow the user to add custom PATH entries ahead of runtimes rtx adds with
rtx activate
.Shims are one solution, but
rtx activate
could be a bit softer here and instead inject the PATH entries at the location of thertx
bin itself. This means if PATH was initially the following:then after running
rtx activate
it could change to this:Note that
~/bin
is before the install directories.Now while I could see use-cases where users want this, it also might be undesirable since the place that rtx is added to PATH matters. Most users will probably want it at the front or near the front regardless. For that reason, we probably want this to be an option (though I'm not sure what default is best).
FYI this is likely a somewhat challenging task. The way rtx sets up env vars is complex and it's also some of the oldest code in the codebase where I was still learning rust. That said, I think this line is what will need to be modified:
https://github.com/jdx/rtx/blob/58b0e0e56f24fb12fae8d36cdf245dcec0808796/src/cli/hook_env.rs#L92
The text was updated successfully, but these errors were encountered: