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

Fix display of env activation message and co #3715

Merged
merged 3 commits into from
Jan 6, 2025
Merged

Fix display of env activation message and co #3715

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1506023
Fix env activation message display
Hind-M Dec 30, 2024
b9e875c
Add tests
Hind-M Dec 30, 2024
19ce847
Code review: add tests with `--json` and `--quiet`
Hind-M Jan 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions libmamba/include/mamba/core/transaction.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,9 @@ namespace mamba
MultiPackageCache& package_caches,
std::vector<detail::other_pkg_mgr_spec>& other_specs
);

// NOTE: This can be moved to somewhere else if more appropriate
Hind-M marked this conversation as resolved.
Show resolved Hide resolved
void print_activation_message(const Context& ctx);
} // namespace mamba

#endif // MAMBA_TRANSACTION_HPP
12 changes: 12 additions & 0 deletions libmamba/src/api/install.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,6 +537,12 @@ namespace mamba

trans.execute(ctx, channel_context, prefix_data);

// Print activation message only if the environment is freshly created
if (create_env)
{
print_activation_message(ctx);
}

Comment on lines +540 to +545
Copy link
Member

Choose a reason for hiding this comment

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

Currently the message is shown for installations and updates if the target prefix is different than the currently activated one. Should we preserve this behavior?

Also, should the message rather be printer after the call to install_for_other_pkgmgr (namely just before the 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.

Currently the message is shown for installations and updates if the target prefix is different than the currently activated one. Should we preserve this behavior?

Indeed, and I was going to keep it that way but I realized conda was only showing activation message with environment creation, and that actually makes more sense and is less verbose (recommending env activation sounds relevant when the user creates a new environment but seems redundant when installing/updating).
For now, I suggest to do it this way, and if we want to restore the previous behavior, that should be straightforward by adding print_activation_message where needed.

Also, should the message rather be printer after the call to install_for_other_pkgmgr (namely just before the else)?

Good question! I put it right after the transaction execution to preserve the order of printing messages (since it was inside Transaction::execute()). But that could be discussed: either consider that the transaction is finished and print the activation message, and then handle pip packages (or anything else?) considering we do not recommend it (cf. warning that we added recently) or print the activation message after everything is done including things we don't really approve.
Note however that install_for_other_pkgmgr does not involve Transaction so it's supposed to be done with before handling other pkg managers.
In my opinion, let's keep it as it is.

if (!ctx.dry_run)
{
for (auto other_spec : config.at("others_pkg_mgrs_specs")
Expand Down Expand Up @@ -641,6 +647,12 @@ namespace mamba

transaction.execute(ctx, channel_context, prefix_data);

// Print activation message only if the environment is freshly created
if (create_env)
{
print_activation_message(ctx);
}

Comment on lines +650 to +655
Copy link
Member

Choose a reason for hiding this comment

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

Same suggestions as above.

Side-note: are there a way we could factor (parts of) install_explicit_with_transaction and install_specs_impl in another PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Side-note: are there a way we could factor (parts of) install_explicit_with_transaction and install_specs_impl in another PR?

Depends on what we want to refactor and if we need some parts in the codebase reworked first. But we can look into it.

for (auto other_spec : others)
{
install_for_other_pkgmgr(ctx, other_spec, pip::Update::No);
Expand Down
54 changes: 27 additions & 27 deletions libmamba/src/core/transaction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,32 @@ namespace mamba
{
namespace nl = nlohmann;

void print_activation_message(const Context& ctx)
{
// Check that the target prefix is not active before printing the activation message
if (util::get_env("CONDA_PREFIX") != ctx.prefix_params.target_prefix)
{
// Get the name of the executable used directly from the command.
const auto executable = get_self_exe_path().stem().string();

// Get the name of the environment
const auto environment = env_name(ctx);

Console::stream() << "\nTo activate this environment, use:\n\n"
" "
<< executable << " activate " << environment
<< "\n\n"
"Or to execute a single command in this environment, use:\n\n"
" "
<< executable
<< " run "
// Use -n or -p depending on if the env_name is a full prefix or just
// a name.
<< (environment == ctx.prefix_params.target_prefix ? "-p " : "-n ")
<< environment << " mycommand\n";
}
}

namespace
{
bool need_pkg_download(const specs::PackageInfo& pkg_info, MultiPackageCache& caches)
Expand Down Expand Up @@ -438,33 +464,7 @@ namespace mamba
LOG_INFO << "Waiting for pyc compilation to finish";
m_transaction_context.wait_for_pyc_compilation();

// Get the name of the executable used directly from the command.
const auto executable = get_self_exe_path().stem().string();

// Get the name of the environment
const auto environment = env_name(ctx);

// Check if the target prefix is active
if (util::get_env("CONDA_PREFIX") == ctx.prefix_params.target_prefix)
{
Console::stream() << "\nTransaction finished\n";
}
else
{
Console::stream() << "\nTransaction finished\n\n"
"To activate this environment, use:\n\n"
" "
<< executable << " activate " << environment
<< "\n\n"
"Or to execute a single command in this environment, use:\n\n"
" "
<< executable
<< " run "
// Use -n or -p depending on if the env_name is a full prefix or just
// a name.
<< (environment == ctx.prefix_params.target_prefix ? "-p " : "-n ")
<< environment << " mycommand\n";
}
Console::stream() << "\nTransaction finished\n";

prefix.history().add_entry(m_history_entry);
return true;
Expand Down
6 changes: 6 additions & 0 deletions micromamba/tests/test_create.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -545,6 +545,12 @@ def test_classic_specs(tmp_home, tmp_root_prefix, tmp_path, outside_root_prefix)
assert cached_file.exists()


def test_create_check_logs(tmp_home, tmp_root_prefix):
env_name = "env-create-check-logs"
res = helpers.create("-n", env_name, "xtensor")
assert "To activate this environment, use:" in res
Hind-M marked this conversation as resolved.
Show resolved Hide resolved


@pytest.mark.skipif(
helpers.dry_run_tests is helpers.DryRun.ULTRA_DRY,
reason="Running only ultra-dry tests",
Expand Down
4 changes: 3 additions & 1 deletion micromamba/tests/test_env.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,9 @@ def test_env_remove(tmp_home, tmp_root_prefix):
assert str(env_fp) in lines

# Unregister / remove env_name
helpers.run_env("remove", "-n", env_name, "-y")
res = helpers.run_env("remove", "-n", env_name, "-y")
assert "To activate this environment, use:" not in res

env_json = helpers.run_env("list", "--json")
assert str(env_fp) not in env_json["envs"]
assert not env_fp.exists()
Expand Down
7 changes: 7 additions & 0 deletions micromamba/tests/test_install.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -665,6 +665,13 @@ def test_install_check_dirs(tmp_home, tmp_root_prefix):
assert os.path.isdir(env_prefix / "lib" / "python3.8" / "site-packages")


def test_install_check_logs(tmp_home, tmp_root_prefix):
env_name = "env-install-check-logs"
helpers.create("-n", env_name)
res = helpers.install("-n", env_name, "xtensor")
assert "To activate this environment, use:" not in res


def test_install_local_package(tmp_home, tmp_root_prefix):
env_name = "myenv"
tmp_root_prefix / "envs" / env_name
Expand Down
7 changes: 7 additions & 0 deletions micromamba/tests/test_remove.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,13 @@ def test_remove(tmp_home, tmp_root_prefix, env_selector, tmp_xtensor_env, tmp_en
assert res["actions"]["PREFIX"] == str(tmp_xtensor_env)


@pytest.mark.parametrize("shared_pkgs_dirs", [True], indirect=True)
def test_remove_check_logs(tmp_home, tmp_root_prefix, tmp_xtensor_env, tmp_env_name):
helpers.install("xtensor-python", "-n", tmp_env_name, no_dry_run=True)
res = helpers.remove("xtensor", "-n", tmp_env_name)
assert "To activate this environment, use:" not in res


@pytest.mark.parametrize("shared_pkgs_dirs", [True], indirect=True)
@pytest.mark.skipif(sys.platform == "win32", reason="This test is currently failing on Windows")
def test_remove_orphaned(tmp_home, tmp_root_prefix, tmp_xtensor_env, tmp_env_name):
Expand Down
4 changes: 4 additions & 0 deletions micromamba/tests/test_update.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,10 @@ def test_channel_alias(self, alias, env_created):
for to_link in res["actions"]["LINK"]:
assert to_link["channel"] == "conda-forge"

def test_update_check_logs(self, env_created):
res = helpers.update("-n", TestUpdate.env_name, "xtensor=0.24.5")
assert "To activate this environment, use:" not in res


class TestUpdateConfig:
current_root_prefix = os.environ["MAMBA_ROOT_PREFIX"]
Expand Down
Loading