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.

Sign up for GitHub

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

Jump to bottom

fix: Adapt root prefix' precedence for `envs_dirs` #3813

Open

holzman wants to merge 4 commits into mamba-org:main from holzman:fix-root-prefix-precedence

+40 −2

holzman commented Feb 10, 2025

Adds a new test and fixes #3806. With this PR it now has the desired behavior:

$ mamba create -r /tmp/cliroot --debug -n foo --print-config-only | yq '. | .envs_dirs'
[
  "/tmp/cliroot/envs"
]

$ MAMBA_ROOT_PREFIX=/tmp/envroot mamba create --debug -n foo --print-config-only | yq '. | .envs_dirs'
[
  "/tmp/envroot/envs"
]

$ MAMBA_ROOT_PREFIX=/tmp/envroot mamba create -r /tmp/cliroot --debug -n foo --print-config-only | yq '. | .envs_dirs'
[
  "/tmp/cliroot/envs"
]

$ CONDA_ENVS_DIRS=/tmp/userdirs/envs MAMBA_ROOT_PREFIX=/tmp/envroot mamba create --debug -n foo --print-config-only | yq '. | .envs_dirs'
[
  "/tmp/userdirs/envs",
  "/tmp/envroot/envs"
]

One note - in order for this approach to work, I needed to ensure that the minimum vector size for configuration items was 1 so that --print-config didn't crash. (Alternatively I could change the print-config code to ignore a zero-size vector, but this seemed like a more robust approach).

holzman added 2 commits

February 5, 2025 16:46


          Add test case for test_create::test_target_prefix

e7d1398

This adds the case to simulate what happens when
${MAMBA_ROOT_PREFIX}/envs already exists.


          Source for config item is a list of at least size 1

f6e2e27

jjerphan changed the title ~~Fix root prefix precedence~~ fix: Adapt root prefix' precedence for envs_dirs

jjerphan added the release::bug_fixes label

jjerphan reviewed

View reviewed changes

Member

jjerphan left a comment

Thank you for your contribution, @holzman!

libmamba/include/mamba/api/configuration_impl.hpp Show resolved Hide resolved

micromamba/tests/test_create.py

Member

jjerphan Feb 11, 2025

Can you add at least a test based on the outputs that you have given in the description of the PR?

Author

holzman Feb 11, 2025

Done.

holzman added 2 commits

February 11, 2025 11:25


          Linter fixes

93008a4


          Add test for root prefix precedence

dfe768a

jjerphan reviewed

View reviewed changes

micromamba/tests/test_create.py

    
                  cliroot = tmp_path / "cliroot"

                  userenv = tmp_path / "userenv" / "envs"

                  map(lambda p: os.makedirs(p), (envroot, cliroot, userenv))

Member

jjerphan Feb 12, 2025

Is this required because of #3790?

Suggested change

      
                map(lambda p: os.makedirs(p), (envroot, cliroot, userenv))
          
                map(lambda p: os.makedirs(p, exist_ok=True), (envroot, cliroot, userenv))

Author

holzman Feb 12, 2025

Yes

micromamba/tests/test_create.py

    
                  map(lambda p: os.makedirs(p), (envroot, cliroot, userenv))

                  monkeypatch.setenv("MAMBA_ROOT_PREFIX", str(envroot))

Member

jjerphan Feb 12, 2025

Can you set it at the really top of this test?

micromamba/tests/test_create.py

Comment on lines +686 to +687

    
                  if user_envs_dirs:

                      monkeypatch.setenv("CONDA_ENVS_DIRS", str(userenv))

Member

jjerphan Feb 12, 2025

Identically, can you set it at the top of the test?

micromamba/tests/test_create.py

Comment on lines +674 to +676

    
                  envroot = tmp_path / "envroot"

                  cliroot = tmp_path / "cliroot"

                  userenv = tmp_path / "userenv" / "envs"

Member

jjerphan Feb 12, 2025 •

edited

Loading

Can you distinguish the prefixes from their envs folders?

Author

holzman Feb 12, 2025

Sorry, I don't quite understand what you're asking.

Member

jjerphan Feb 12, 2025

I meant to have something like:

Suggested change

      
                envroot = tmp_path / "envroot"
          
                cliroot = tmp_path / "cliroot"
          
                userenv = tmp_path / "userenv" / "envs"
          
                envroot = tmp_path / "envroot"
          
                cliroot = tmp_path / "cliroot"
          
                userenv = tmp_path / "userenv"
          
                env_root_envs = envroot / "envs"
          
                cliroot_envs  = cliroot / "envs"
          
                userenv_envs  = userenv / "envs

micromamba/tests/test_create.py

    
                  if user_envs_dirs:

                      monkeypatch.setenv("CONDA_ENVS_DIRS", str(userenv))

                  res = helpers.create(*cmd)

Member

jjerphan Feb 12, 2025

Can you check that foo is successfully created and that, depending the test case, foo is created under envroot/envs, cliroot/envs or (after distinguishing prefixes from their envs folder) userenv/envs?

Author

holzman Feb 12, 2025

In addition to checking the output from --print-config or in addition to it?

micromamba/tests/test_create.py

Comment on lines +282 to +284

    
                  if root_prefix_env_exists:

                      os.mkdir(Path(os.environ["MAMBA_ROOT_PREFIX"]) / "envs")

Member

jjerphan Feb 12, 2025

Why is this needed?

Author

holzman Feb 12, 2025

Because of #3790.

Member

jjerphan Feb 12, 2025

Suggested change

      
                if root_prefix_env_exists:
          
                    os.mkdir(Path(os.environ["MAMBA_ROOT_PREFIX"]) / "envs")
          
                # TODO: Remove once https://github.com/mamba-org/mamba/issues/3790 is fixed.
          
                if root_prefix_env_exists:
          
                    os.mkdir(Path(os.environ["MAMBA_ROOT_PREFIX"]) / "envs", exist_ok=True)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release::bug_fixes