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

Remove base environment PATH components from conda run environment #11257

Closed
wants to merge 10 commits into from

Conversation

jezdez
Copy link
Member

@jezdez jezdez commented Feb 23, 2022

Fix #11174.

This is to have more consistency between dev and prod envs.
@jezdez jezdez requested a review from a team as a code owner February 23, 2022 23:02
@anaconda-issue-bot anaconda-issue-bot added the cla-signed [bot] added once the contributor has signed the CLA label Feb 23, 2022
@@ -78,7 +78,7 @@ jobs:
--rm -v ${PWD}:/opt/conda-src
-e TEST_SPLITS
-e TEST_GROUP
ghcr.io/conda/conda-ci:master-linux-python${{ matrix.python-version }}
ghcr.io/conda/conda-ci:pr-11257-linux-python${{ matrix.python-version }}
Copy link
Member Author

Choose a reason for hiding this comment

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

This'll need to be changed back to master, but is needed since the addition of /opt/conda/bin to PATH in the Dockerfile below made testing this impossible.

conda/utils.py Outdated Show resolved Hide resolved
@kenodegard
Copy link
Contributor

I don't think this is the fix we want. This change will blindly remove the base env in all scenarios which would be problematic when stacking environments (either via auto_stack >= 1 or conda activate --stack).

In non-stacked cases (see Case 1 & 2) this fix will correctly handle the problem but in stacked cases (see Case 3) this fix will incorrectly strip out the stacked base env.

Case 1: No environments activated (I think we all agree with this one)
$ conda run -n haspy which -a python           # expect haspy python
$CONDA_ROOT/envs/haspy/bin/python
/usr/bin/python

$ conda run -n notpy which -a python           # expect system python
/usr/bin/python
Case 2: auto_stack: 0 (default behavior, here too I think we are in agreement)
# activating base
$ conda activate base
(base) $

(base) $ conda run -n haspy which -a python
$CONDA_ROOT/envs/haspy/bin/python
/usr/bin/python

(base) $ conda run -n notpy which -a python
/usr/bin/python

# activating haspy, no stacking
(base) $ conda activate haspy
(haspy) $

(haspy) $ conda run -n haspy which -a python
$CONDA_ROOT/envs/haspy/bin/python
/usr/bin/python

(haspy) $ conda run -n notpy which -a python
/usr/bin/python
Case 3: auto_stack: 1+ or conda activate --stack ENV
# activating base
$ conda activate base
(base) $

(base) $ conda run -n haspy which -a python    # expect haspy python
$CONDA_ROOT/envs/haspy/bin/python
$CONDA_ROOT/bin/python
/usr/bin/python

(base) $ conda run -n notpy which -a python    # expect base python
$CONDA_ROOT/bin/python
/usr/bin/python

# activating haspy, with stacking
(base) $ conda activate haspy
(haspy) $

(haspy) $ conda run -n haspy which -a python   # expect haspy python
$CONDA_ROOT/envs/haspy/bin/python
$CONDA_ROOT/bin/python
/usr/bin/python

(haspy) $ conda run -n notpy which -a python   # expect haspy python
$CONDA_ROOT/envs/haspy/bin/python
$CONDA_ROOT/bin/python
/usr/bin/python

I noticed two interesting things:

  1. when conda activate/deactivate is called it is only replacing the first prefix match, see conda.activate._Activator._replace_prefix_in_path
  2. our PATH is already mangled at the very start of the conda import (i.e. add print(os.environ["PATH"]) at the top of conda/__init__.py and it lists the current $CONDA_PREFIX twice but echo $PATH only shows it once), this duplication is happening in our shell interface:
    • __conda_exe() (
      __add_sys_prefix_to_path
      "$CONDA_EXE" $_CE_M $_CE_CONDA "$@"
      )
      __add_sys_prefix_to_path() {
      # In dev-mode CONDA_EXE is python.exe and on Windows
      # it is in a different relative location to condabin.
      if [ -n "${_CE_CONDA}" ] && [ -n "${WINDIR+x}" ]; then
      SYSP=$(\dirname "${CONDA_EXE}")
      else
      SYSP=$(\dirname "${CONDA_EXE}")
      SYSP=$(\dirname "${SYSP}")
      fi
      if [ -n "${WINDIR+x}" ]; then
      PATH="${SYSP}/bin:${PATH}"
      PATH="${SYSP}/Scripts:${PATH}"
      PATH="${SYSP}/Library/bin:${PATH}"
      PATH="${SYSP}/Library/usr/bin:${PATH}"
      PATH="${SYSP}/Library/mingw-w64/bin:${PATH}"
      PATH="${SYSP}:${PATH}"
      else
      PATH="${SYSP}/bin:${PATH}"
      fi
      \export PATH
      }
    • @SET PATH=!_sysp!;!_sysp!\Library\mingw-w64\bin;!_sysp!\Library\usr\bin;!_sysp!\Library\bin;!_sysp!\Scripts;!_sysp!\bin;%PATH%

So I'm thinking that we need to be looking into understanding why the prefix is being re-injected into the PATH prior to calling conda.

@jezdez
Copy link
Member Author

jezdez commented Feb 25, 2022

Well, #8528 added __add_sys_prefix_to_path and doesn't say a word why except for pointing to a number of tickets, which seem to be regression tickets for stuff that was broken in 910a7ee. Super messy changes and so little explanation in public tickets :(

@jezdez
Copy link
Member Author

jezdez commented Feb 25, 2022

We've talked a bit more and think this is best solved differently by looking at the changes made in #8528. Given that the ticket #11174 isn't the highest priority we shouldn't block the 4.12.0 release, either.

@kenodegard in case you find anything new, feel free to reopen of course!

@jezdez jezdez closed this Feb 25, 2022
@jezdez jezdez deleted the conda-run-path branch March 10, 2022 12:07
@kenodegard kenodegard mentioned this pull request Jul 28, 2022
3 tasks
@github-actions github-actions bot added the locked [bot] locked due to inactivity label Mar 19, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-signed [bot] added once the contributor has signed the CLA locked [bot] locked due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

conda shell function injects $CONDA_PREFIX into $PATH causing incorrect behavior in conda run
3 participants