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

Honor envs_dirs #2538

Merged
merged 9 commits into from
May 25, 2023
Merged

Honor envs_dirs #2538

merged 9 commits into from
May 25, 2023

Conversation

AntoinePrv
Copy link
Member

@AntoinePrv AntoinePrv commented May 22, 2023

@katringoogoo, I had to start anew due to large number of conflicts. This solves the issue of envs_dirs with create and activate (with tests).
Do you want to iterate on this and make sure envs_dirs is honored with other commands as well (install, update, env...)? With the refactor of putting the logic inside Configuration, this could be as simple as making sure these commands use target_prefix.

{
return root_prefix;
}
if (auto dir = find_env_in_dirs(name, envs_dirs); dir.has_value())
Copy link
Member

Choose a reason for hiding this comment

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

Just checking, do we want an env from env_dirs even if it's not writable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, you can activate it for instance (which is what the contributor initially wanted).

micromamba/src/shell.cpp Show resolved Hide resolved

subcmd
->add_option(
"prefix_or_name",
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, do we want to allow this? Is it for specific cases, or to follow e.g conda's behavior, or something else?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, at least for backward compatibility. It is a feature that you can do micromamba activate env without specifying.

@@ -86,16 +169,18 @@ namespace
config.load();
shell_init(
consolidate_shell(config.at("shell_type").compute().value<std::string>()),
config.at("shell_prefix").compute().value<std::string>()
Context::instance().prefix_params.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.

I'm a bit lost here, why do we replace shell_prefix with the root_prefix? Is it because we know that we use it in here deterministically? (same for the other replacements below).

Copy link
Member Author

Choose a reason for hiding this comment

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

It was always the root_prefix 🙃
It was that way because the same CLI option was used as a catch all root/prefix.

+ guessed_shell + R"() shells, run:
$ micromamba shell init --shell=)"
+ guessed_shell + R"( --prefix=~/micromamba
std::string const guessed_shell = guess_shell();
Copy link
Member

Choose a reason for hiding this comment

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

image

@JohanMabille JohanMabille merged commit 17232ca into mamba-org:main May 25, 2023
20 checks passed
@AntoinePrv AntoinePrv deleted the envs_dirs branch May 25, 2023 09:44
@mforbes mforbes mentioned this pull request Jul 17, 2023
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants