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

fix: Adapt root prefix' precedence for envs_dirs #3813

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

holzman
Copy link

@holzman 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).

This adds the case to simulate what happens when
${MAMBA_ROOT_PREFIX}/envs already exists.
@jjerphan jjerphan changed the title Fix root prefix precedence fix: Adapt root prefix' precedence for envs_dirs Feb 11, 2025
@jjerphan jjerphan added the release::bug_fixes For PRs fixing bugs label Feb 11, 2025
Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution, @holzman!

libmamba/include/mamba/api/configuration_impl.hpp Outdated Show resolved Hide resolved
micromamba/tests/test_create.py Show resolved Hide resolved
micromamba/tests/test_create.py Outdated Show resolved Hide resolved
micromamba/tests/test_create.py Outdated Show resolved Hide resolved
micromamba/tests/test_create.py Outdated Show resolved Hide resolved
micromamba/tests/test_create.py Outdated Show resolved Hide resolved
micromamba/tests/test_create.py Outdated Show resolved Hide resolved
micromamba/tests/test_create.py Outdated Show resolved Hide resolved
@holzman
Copy link
Author

holzman commented Feb 13, 2025

Ok, I think I've addressed all of the issues, thanks for being so patient!

Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

Thank you for your patience.

micromamba/tests/test_create.py Outdated Show resolved Hide resolved
micromamba/tests/test_create.py Outdated Show resolved Hide resolved
micromamba/tests/test_create.py Outdated Show resolved Hide resolved
Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

Thank you, @holzman!

I just have a couple of nitpicks but apart from them, the test covers all the cases.

I would wait for another approval from someone in the team before merging this contribution.

micromamba/tests/test_create.py Show resolved Hide resolved
if set_in_condarc:
f.write(f"envs_dirs: [{str(condarc_envs_dirs)}]")

for envs_folder in (condarc_envs_dirs, conda_envs_dirs, cli_provided_root, mamba_root_prefix):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for envs_folder in (condarc_envs_dirs, conda_envs_dirs, cli_provided_root, mamba_root_prefix):
# TODO: Remove those calls to `os.makedirs` once
# https://github.com/mamba-org/mamba/issues/3790 is fixed
for envs_folder in (condarc_envs_dirs, conda_envs_dirs, cli_provided_root, mamba_root_prefix):

micromamba/tests/test_create.py Outdated Show resolved Hide resolved
micromamba/tests/test_create.py Show resolved Hide resolved
micromamba/tests/test_create.py Outdated Show resolved Hide resolved
micromamba/tests/test_create.py Outdated Show resolved Hide resolved
@holzman
Copy link
Author

holzman commented Feb 14, 2025

Ok, I've added one more commit to address the nits. (Now I'm off to write a better test for #3796).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release::bug_fixes For PRs fixing bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MAMBA_ROOT_PREFIX has precedence over -r option
2 participants