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.

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

Use envs_dirs settings during environment creation #2475

Closed
wants to merge 14 commits into from
Closed

Use envs_dirs settings during environment creation #2475

wants to merge 14 commits into from

Conversation

katringoogoo
Copy link

@katringoogoo katringoogoo commented Apr 19, 2023 •
edited
Loading

Edit: Adresses the issue mentioned in #2465

  • Examine envs_dirs upon environment creation and select the first writeable location.
  • Added envs_dirs and pkg_dirs to the info comman

Open Question: can i somehow distinguish between environment creation by name and by target prefix?

@katringoogoo katringoogoo force-pushed the use_envs_dirs_if_set branch from 3e40cb8 to 3747525 Compare April 19, 2023 21:11
@AntoinePrv
Copy link
Member

Hi @katringoogoo, is this related to #2465 ?

@katringoogoo
Copy link
Author

Hi @AntoinePrv! Yes that is related to this issue ... I'm going to look into the failing test as soon as possible. Is there anything else I need to take care of to get the PR accepted?

AntoinePrv
AntoinePrv reviewed Apr 20, 2023
View reviewed changes
libmamba/src/api/create.cpp Outdated
fs::u8path target_prefix = ctx.target_prefix;
for (auto& env_dir : ctx.envs_dirs)
{
if (fs::exists(env_dir) && fs::is_directory(env_dir) && mamba::path::is_writable(env_dir))
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be reasonable to create env_dir if it does not exist.

AntoinePrv
AntoinePrv reviewed Apr 20, 2023
View reviewed changes
libmamba/src/api/create.cpp Outdated
@@ -29,23 +28,32 @@ namespace mamba
auto& create_specs = config.at("specs").value<std::vector<std::string>>();
auto& use_explicit = config.at("explicit_install").value<bool>();

fs::u8path target_prefix = ctx.target_prefix;
for (auto& env_dir : ctx.envs_dirs)
Copy link
Member

Choose a reason for hiding this comment

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

Nice, maybe this could be a method of the Context (not sure yet how it would play with ctx.target_prefix)

Copy link
Member

Choose a reason for hiding this comment

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

Ideally, this should be part of the deduction of target_prefix

Copy link
Author

Choose a reason for hiding this comment

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

you mean like in a set_post_context_hook for example?

AntoinePrv
AntoinePrv reviewed Apr 20, 2023
View reviewed changes
libmamba/src/core/fsutil.cpp Outdated
Comment on lines 140 to 143
// If it is a directory we created a subitem in that directory and need to delete it
// If the path didn't exist before we created a file that needs to be removed as well
if (is_directory || path_exists == false)
Copy link
Member

Choose a reason for hiding this comment

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

Could you modify libmamba/tests/src/core/test_util.cpp with a test for that behavior?

Copy link
Author

Choose a reason for hiding this comment

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

i added an assertion to test for this as well

@AntoinePrv
Copy link
Member

Hi @katringoogoo,

I had to look more into the Configuration class that I do not know so well.

you mean like in a set_post_context_hook for example?

The target_prefix gets computed (at first through) env_name_hook

mamba/libmamba/src/api/configuration.cpp

Line 440 in 75b49fc

void env_name_hook(std::string& name)

And here we ignore envs_dirs

mamba/libmamba/src/api/configuration.cpp

Line 475 in 75b49fc

prefix = root_prefix / "envs" / name;

My recommendation is

  • So we can add a dependency of taget_prefix to envs_dirs in needs here:

    mamba/libmamba/src/api/configuration.cpp

    Line 1058 in 75b49fc

    insert(Configurable("env_name", std::string(""))
  • In env_name_hook we can go through the envs_dirs,
    • if one matches the name good,
    • otherwise find the first writable directory (with an auxiliary function) and set the target_prefix to that.
  • Then we need to move the error handling to the function such as install/update so that if the target_prefix does not exists the command fails

Does that seem reasonable?

@AntoinePrv AntoinePrv force-pushed the use_envs_dirs_if_set branch from eb23687 to 2417aed Compare May 9, 2023 14:48
@AntoinePrv
Copy link
Member

Waiting for the CI to go 🟢 before sending more changes.

@AntoinePrv AntoinePrv force-pushed the use_envs_dirs_if_set branch from 761bbe3 to 4cef0f2 Compare May 11, 2023 15:38
@pavelzw
Copy link
Member

pavelzw commented May 12, 2023

The CI is red in Windows because of winreg-feedstock#7. This is fixed in #2520.

@AntoinePrv AntoinePrv force-pushed the use_envs_dirs_if_set branch 2 times, most recently from df9e727 to 5734a4f Compare May 15, 2023 13:45
@AntoinePrv AntoinePrv force-pushed the use_envs_dirs_if_set branch from 5734a4f to 4a4d544 Compare May 15, 2023 13:45
@AntoinePrv AntoinePrv mentioned this pull request May 15, 2023
Merged
@AntoinePrv AntoinePrv self-assigned this May 16, 2023
This was referenced May 16, 2023
Closed
Merged
@AntoinePrv
Copy link
Member

Closing in favor of #2538, this is much out of sync with main.

@AntoinePrv AntoinePrv closed this May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AntoinePrv AntoinePrv AntoinePrv left review comments

Assignees

@AntoinePrv AntoinePrv

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@katringoogoo @AntoinePrv @pavelzw @katrinm86