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: Create directories from envs_dirs if they do not exist #3796

Merged
merged 16 commits into from
Feb 28, 2025

Conversation

holzman
Copy link
Contributor

@holzman holzman commented Feb 4, 2025

Fix #3790.
Fix #3836.

@jjerphan jjerphan changed the title Create directory from envs_dirs if it does not exist fix: Create directories from envs_dirs if they do not exist Feb 5, 2025
@jjerphan jjerphan added the release::bug_fixes For PRs fixing bugs label Feb 5, 2025
@holzman
Copy link
Contributor Author

holzman commented Feb 5, 2025

Ah, my bad. I'll revise the failing tests and update this PR.

@holzman
Copy link
Contributor Author

holzman commented Feb 5, 2025

Actually, I think this exposed an issue with #3692:

In 2.0.5, -r took precedence over MAMBA_ROOT_PREFIX:

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

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

but in 2.0.6:

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

@jjerphan
Copy link
Member

Merging main into this branch to get the changes of #3813 in.

@holzman
Copy link
Contributor Author

holzman commented Feb 20, 2025

I assume the Windows test is failing because it succesfully creates Path("/noperm") and can write to it (unlike in Linux) - I can check into it as soon as I get back to a Windows machine.

@jjerphan
Copy link
Member

jjerphan commented Feb 20, 2025

Thank you!

For your information, looking at the test logs:

    def test_create_envs_dirs(tmp_root_prefix: Path, tmp_path: Path):
        """Create an environment when the first env dir is not writable."""
        os.environ["CONDA_ENVS_DIRS"] = f"{Path('/noperm')},{tmp_path}"
        env_name = "myenv"
        helpers.create("-n", env_name, "--offline", "--no-rc", no_dry_run=True)
>       assert (tmp_path / env_name / "conda-meta" / "history").exists()
E       AssertionError: assert False
E        +  where False = exists()
E        +    where exists = (((WindowsPath('C:/Users/runneradmin/AppData/Local/Temp/pytest-of-runneradmin/pytest-0/test_create_envs_dirs_False_0') / 'myenv') / 'conda-meta') / 'history').exists

/noperm is not used but C:/Users/runneradmin/AppData/Local/Temp/pytest-of-runneradmin/pytest-0/test_create_envs_dirs_False_0 (which should be the value of tmp_root_prefix) is.

@holzman
Copy link
Contributor Author

holzman commented Feb 20, 2025

    def test_create_envs_dirs(tmp_root_prefix: Path, tmp_path: Path):
        """Create an environment when the first env dir is not writable."""
        os.environ["CONDA_ENVS_DIRS"] = f"{Path('/noperm')},{tmp_path}"
        env_name = "myenv"
        helpers.create("-n", env_name, "--offline", "--no-rc", no_dry_run=True)
>       assert (tmp_path / env_name / "conda-meta" / "history").exists()
E       AssertionError: assert False
E        +  where False = exists()
E        +    where exists = (((WindowsPath('C:/Users/runneradmin/AppData/Local/Temp/pytest-of-runneradmin/pytest-0/test_create_envs_dirs_False_0') / 'myenv') / 'conda-meta') / 'history').exists

/noperm is not used but C:/Users/runneradmin/AppData/Local/Temp/pytest-of-runneradmin/pytest-0/test_create_envs_dirs_False_0 (which should be the value of tmp_root_prefix) is.

So this test was previously successful on both Windows and Linux because /noperm did not exist, so it then goes to the next env_dir and creates an env in tmp_path. My guess is that this test is now failing because this PR tries to os.mkdir("/noperm") first. That should fail on Linux for permissions reasons, but if it succeeds on Windows, it will create the env there rather than in tmp_path.

@jjerphan
Copy link
Member

Ah yes, my bad, I misread the assertion.

@jjerphan
Copy link
Member

In the meantime, #3836 has been reported.

Could we see if this PR fixes it by adding a non-regression test for it?

@holzman
Copy link
Contributor Author

holzman commented Feb 21, 2025

Ok, I’m a little stumped on this one; I rolled out a Windows dev environment and the test (as revised) passed…

@jjerphan jjerphan force-pushed the create-env-dir branch 4 times, most recently from 7c6a92a to eef136f Compare February 24, 2025 13:32
@jjerphan
Copy link
Member

@holzman: I tried several alternatives, but I think it would be worth having someone with a Windows machine reproduce and understand the error locally.

Feel free to remove / revert my commits.

@holzman
Copy link
Contributor Author

holzman commented Feb 24, 2025

Thanks @jjerphan - I did test in a Windows machine, unfortunately. Something must be subtly different about the environment in the actual Windows runner. I'll see if I can figure that out in a separate project; if it turns out it's impossible to make a read-only directory for the test, we should probably just skip the test for Windows.

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.

LGTM given 🟢 CI.

I am waiting for a review from someone else in the team before merging.

@holzman
Copy link
Contributor Author

holzman commented Feb 25, 2025

This test failing isn't my fault for a change!

I think the latest commit to dateutil has a file with CRLF which gets autoconverted during the pip install (due to dateutil's settings in .gitattributes), showing up as modified and then erroring out.

@jjerphan
Copy link
Member

Indeed.

See dateutil/dateutil#1419.

@jjerphan
Copy link
Member

Merging main in to get #3838.

Copy link
Member

@Klaim Klaim left a comment

Choose a reason for hiding this comment

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

LGTM but I'm not the most knowledgeable about the behavior tested in the python tests, it seems fine to me but I'm not sure it's correct. Maybe wait for a review from @Hind-M before merging.

Copy link
Member

@Hind-M Hind-M left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@jjerphan jjerphan merged commit d9af139 into mamba-org:main Feb 28, 2025
34 checks passed
@holzman holzman deleted the create-env-dir branch February 28, 2025 21:37
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
4 participants