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

Always restore PATH after deactivate even if modified in activate scripts #8070

Closed
beenje opened this Issue Jan 9, 2019 · 10 comments

Comments

Projects
None yet
5 participants
@beenje
Copy link

beenje commented Jan 9, 2019

It's currently not possible to modify the PATH in the deactivate scripts. See #3915
There is one pending PR to fix that using a two phase deactivate (#7026).

Request to update the PATH in the deactivate scripts is to reverse changes performed in the activate scripts. I don't see another use case:
After deactivate, the PATH should be the same as what it was before activate.

An easier way (than the 2 phase deactivate) to achieve this would be to save the current PATH in an environment variable (like CONDA_RESTORE_PATH) during the activation and use it to restore the PATH during deactivate.

If this sounds like a good idea, I can make a PR.

@mingwandroid

This comment has been minimized.

Copy link
Contributor

mingwandroid commented Jan 14, 2019

Thanks @beenje

After deactivate, the PATH should be the same as what it was before activate.

I disagree with this assertion. It is more correct IMHO to say:

After deactivate, the specific entries added to PATH during activate should be removed from PATH.

Otherwise the following would not do what the user expected:

conda activate env
PATH=$HOME/bin:$PATH
conda deactivate
@beenje

This comment has been minimized.

Copy link
Author

beenje commented Jan 15, 2019

I didn't think about this use case.

I personally think it makes more sense for the PATH to be identical to what it was before activate after deactivate.
conda modifies the PATH. I could argue that if you modify the PATH when being in an environment, it's something that is needed in that environment. It would make sense to revert to the original PATH after deactivate. I agree this changes the current behaviour (but probably not in the most common case).

I want to add extra things to the PATH in the activate.d scripts and I want those changes to be gone after deactivate.

As I see it, we have a few options:

  1. we agree that the PATH is always reverted to its original value after deactivate. Any modification to the PATH inside a conda environment is lost. I think it's actually cleaner, but I might miss some valid use cases.
  2. we use an extra environment variable and/or flag to specify the behaviour (a bit like --stack). We keep the current behaviour by default, but allow restoring to the original PATH if this flag was set.
  3. We give up on the CONDA_RESTORE_PATH idea and look again at the two phase deactivate PR #7026

What do you think?

@msarahan

This comment has been minimized.

Copy link
Contributor

msarahan commented Jan 31, 2019

I think explicit behavior is better than implicit. If packages change env vars in activate.d scripts, they should undo that change in deactivate.d scripts. That mechanism already exists, but it's too tedious. I think the right answer is not to change deactivation, but to improve mechanisms for packages to specify env vars, and let the cleanup be part of that mechanism. Defining that behavior from the start for the new mechanism is better than changing behavior now for the old mechanism.

@kalefranz

This comment has been minimized.

Copy link
Member

kalefranz commented Jan 31, 2019

PATH is not a special case. Any environment variable that gets modified by activate should be put back to it previous state with deactivate. There's some work to do to actually get a viable and clean solution here, including #6820.

@beenje

This comment has been minimized.

Copy link
Author

beenje commented Jan 31, 2019

Thanks for the feedback. I get your point and agree. The new mechanism you describe would be much better.
Anyone currently working on #6820?

Anyway I'll close this issue.

@beenje beenje closed this Jan 31, 2019

@mingwandroid

This comment has been minimized.

Copy link
Contributor

mingwandroid commented Jan 31, 2019

PATH is not a special case. Any environment variable that gets modified by activate should be put back to it previous state with deactivate. There's some work to do to actually get a viable and clean solution here, including #6820.

I disagree with this. We went to some lengths to make it so that conda's modifications are undone without discarding other modifications. In my opinion that should be be the case with all environment variables we modify.

This can be expressed succinctly as "you should not undo things you did not do."

I am of the opinion that if conda simply dismissed the user modifications in such a manner it could not be used in a range of workflows and would cause a lot of surprise.

@mingwandroid

This comment has been minimized.

Copy link
Contributor

mingwandroid commented Jan 31, 2019

To be clear, we modify env vars to serve a purpose (in the case of PATH, so that conda software gets found). Users modify env vars to serve their own purpose (in the case of PATH, so that other software gets found, either in preference to ours or otherwise). Once our purpose is done, we cannot pretend or assume that the same is true for the user's purpose. I do not agree that "environments" make this special, to be clear, to me, an environment is not a subshell.

I would also encourage @kalefranz and @beenje to consider carefully what would happen if you have two packages that modify the same env. vars upon activation and what happens when you remove either of them and the ordering problems this creates.

@msarahan

This comment has been minimized.

Copy link
Contributor

msarahan commented Jan 31, 2019

This can be expressed succinctly as "you should not undo things you did not do."

Yes, I think this captures the idea well. Conda can provide ways to set things and clean up after itself, but it should not clean up after things the user does in that space.

@kalefranz

This comment has been minimized.

Copy link
Member

kalefranz commented Jan 31, 2019

I agree with both of you @mingwandroid and @msarahan. Maybe better though is "you should not undo things you did not do; you should undo things you're sure you did"?

@nickelsey

This comment has been minimized.

Copy link

nickelsey commented Mar 1, 2019

I would like add that there is an active need for this, at least in my workflow: its needed to extend conda environments to work seamlessly with non-conda packages located outside of the environment directory - in my case, c/c++ libraries and third-party binaries. I know this is a workflow that conda probably can't (and shouldn't) be optimized for, but it would be nice if this technical roadblock was removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.