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

Better isolate the SCT conda env by using -p instead of -n during creation #3989

Merged
merged 3 commits into from Jan 17, 2023

Conversation

joshuacwnewton
Copy link
Member

Description

Previously, when -n was used, there was a risk of the venv_sct conda environment being created in the global $ENVS_DIRS directory, rather than the expected $SCT_DIR/python/envs/venv_sct. This is because -n is supposed to be a relative name within the global conda envs dir. This global envs dir can be set by users using a .condarc file. But, SCT would prefer to ignore this global dir, and always create its env in $SCT_DIR.

So instead, we use -p, which specifies an absolute path, rather than a relative one. This means it is unaffected by the presence of envs_dirs in a .condarc file.

Further rationale is provided in #3982 (comment).

Linked issues

Fixes #3982.

Previously, when `-n` was used, there was a risk of the conda environment
being created in `$ENVS_DIRS/venv_sct`, rather than the expected
`$SCT_DIR/python/envs/venv_sct`.

This is because `-n` is supposed to be a *relative name* within the global conda
envs dir. This global envs dir can be set by users using a `.condarc` file. But,
SCT would prefer to ignore the global dir, and always create its env in $SCT_DIR.

Thankfully, `-p` specifies an absolute path, rather than a relative one, so it is
unaffected by the presence of `envs_dirs` in a `.condarc` file.
@joshuacwnewton joshuacwnewton added the bug category: fixes an error in the code label Jan 5, 2023
@joshuacwnewton joshuacwnewton added this to the 6.0 milestone Jan 5, 2023
@joshuacwnewton joshuacwnewton self-assigned this Jan 5, 2023
Copy link
Contributor

@kousu kousu left a comment

Choose a reason for hiding this comment

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

Yep, looks good. Pre-approving.

I will give this a test run this evening under the reproduction condition you discovered to double-check your work. But it should be okay I think.

@mguaypaq
Copy link
Member

@kousu, gentle ping: did you have a chance to test this fix?

@kousu
Copy link
Contributor

kousu commented Jan 16, 2023 via email

@mguaypaq
Copy link
Member

Ok, using the reproduction steps from this comment, I was able to reproduce the problem inside a Docker container running ubuntu:22.04. Then I was able to check that this branch fixes the problem. I'll go ahead and merge it.

@joshuacwnewton
Copy link
Member Author

Thank you @mguaypaq! ♥️

@mguaypaq mguaypaq merged commit 377bfb8 into master Jan 17, 2023
@mguaypaq mguaypaq deleted the jn/3982-specify-conda-env-by-path branch January 17, 2023 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug category: fixes an error in the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.condarc envs_dirs: breaks install_sct
3 participants