Skip to content

maint: Factorize common parts of the install, update and remove paths - #4215

Merged
jjerphan merged 2 commits into
mamba-org:mainfrom
jjerphan:maint/factorize-install-update-paths
Mar 31, 2026
Merged

maint: Factorize common parts of the install, update and remove paths#4215
jjerphan merged 2 commits into
mamba-org:mainfrom
jjerphan:maint/factorize-install-update-paths

Conversation

@jjerphan

@jjerphan jjerphan commented Mar 27, 2026

Copy link
Copy Markdown
Member

Description

I would like to introduce a proper retry mechanism for #4214 like what exists for install_specs_impl.

I have seen that there's a lot of duplications and inconsistency between this paths, so this first corrects it.

Type of Change

  • Bugfix
  • Feature / enhancement
  • CI / Documentation
  • Maintenance

Checklist

  • My code follows the general style and conventions of the codebase, ensuring consistency
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have run pre-commit run --all locally in the source folder and confirmed that there are no linter errors.
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing tests pass locally with my changes

@github-actions github-actions Bot added the release::maintenance For PRs related to maintenance label Mar 27, 2026
@codecov

codecov Bot commented Mar 27, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0.49505% with 201 lines in your changes missing coverage. Please review.
✅ Project coverage is 53.55%. Comparing base (0d90b6a) to head (eacc3e8).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
libmamba/src/api/utils.cpp 0.00% 105 Missing ⚠️
libmamba/src/api/install.cpp 0.00% 59 Missing ⚠️
libmamba/src/api/update.cpp 0.00% 30 Missing ⚠️
libmamba/src/api/remove.cpp 0.00% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4215      +/-   ##
==========================================
+ Coverage   52.75%   53.55%   +0.79%     
==========================================
  Files         239      239              
  Lines       29270    29264       -6     
  Branches     3121     3106      -15     
==========================================
+ Hits        15441    15671     +230     
+ Misses      13826    13590     -236     
  Partials        3        3              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jjerphan
jjerphan force-pushed the maint/factorize-install-update-paths branch from 980a131 to f9373bb Compare March 27, 2026 14:51
@jjerphan
jjerphan marked this pull request as ready for review March 27, 2026 14:58
@jjerphan
jjerphan force-pushed the maint/factorize-install-update-paths branch from f9373bb to 0b050c7 Compare March 27, 2026 15:11
Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>
@jjerphan
jjerphan force-pushed the maint/factorize-install-update-paths branch from 0b050c7 to 334fde6 Compare March 30, 2026 07:48
@jjerphan jjerphan changed the title maint: Factorize common part of the install and update paths maint: Factorize common part of the install, update and remove paths Mar 30, 2026
@jjerphan
jjerphan force-pushed the maint/factorize-install-update-paths branch 2 times, most recently from 00f2192 to 69562c5 Compare March 30, 2026 08:05
@jjerphan jjerphan changed the title maint: Factorize common part of the install, update and remove paths maint: Factorize common parts of the install, update and remove paths Mar 30, 2026

@jjerphan jjerphan left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Comments for reviewers.

Comment on lines -656 to -657
add_pins_to_request(request, ctx, prefix_data, raw_specs, no_pin, no_py_pin);
request.flags = ctx.solver_flags;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

request.flags = ctx.solver_flags

was moved at the end of add_pins_to_request.

Comment on lines 200 to -209
add_pins_to_request(
request,
ctx,
ctx.pinned_packages,
ctx.solver_flags,
prefix_data,
raw_update_specs,
/* no_pin= */ config.at("no_pin").value<bool>(),
/* no_py_pin = */ config.at("no_py_pin").value<bool>()
no_pin,
no_py_pin
);

request.flags = ctx.solver_flags;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

request.flags = ctx.solver_flags

was moved at the end of add_pins_to_request.

Comment thread libmamba/src/api/utils.cpp Outdated
);
}
}
request.flags = solver_flags;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This was consistently used after add_pins_to_request, so I just made it part of this function.

Comment on lines -55 to -62
void add_pins_to_request(
solver::Request& request,
const Context& ctx,
PrefixData& prefix_data,
std::vector<std::string> specs,
bool no_pin,
bool no_py_pin
);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Moved to libmamba/include/mamba/api/utils.hpp as it is also use in libmamba/src/api/update.cpp

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is theoretically a backward incompatible change since utils.hpp is in the src folder (and we don't want to change that).

One way to avoid code duplication is to keep this declaration and ave the implementation in install.cpp forward the call to the new function in utils.hpp.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, I know; but I wondered whether any other projects use those headers (it does not seem to be the case); we can keep the declaration here; and the definition in the install.cpp translation unit.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's something we cannot really know until someone comes and complains about the breakage, so let's be conservative here.

Comment on lines -552 to -580
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.envs_dirs,
ctx.prefix_params.root_prefix,
ctx.prefix_params.target_prefix
);

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";
}
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Also moved to utils.

Comment on lines -55 to -62
void add_pins_to_request(
solver::Request& request,
const Context& ctx,
PrefixData& prefix_data,
std::vector<std::string> specs,
bool no_pin,
bool no_py_pin
);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is theoretically a backward incompatible change since utils.hpp is in the src folder (and we don't want to change that).

One way to avoid code duplication is to keep this declaration and ave the implementation in install.cpp forward the call to the new function in utils.hpp.

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

Co-authored-by: Johan Mabille <johan.mabille@gmail.com>
@jjerphan
jjerphan merged commit 880e0f0 into mamba-org:main Mar 31, 2026
41 of 43 checks passed
@jjerphan
jjerphan deleted the maint/factorize-install-update-paths branch March 31, 2026 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release::maintenance For PRs related to maintenance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants