Inject channels+mirrors coming from mambajs env-lockfiles#4126
Inject channels+mirrors coming from mambajs env-lockfiles#4126Klaim wants to merge 64 commits intomamba-org:mainfrom
Conversation
github-actions
bot
added
the
release::enhancements
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4126 +/- ##
==========================================
+ Coverage 52.89% 53.32% +0.43%
==========================================
Files 239 240 +1
Lines 29172 29270 +98
Branches 3092 3095 +3
==========================================
+ Hits 15430 15609 +179
+ Misses 13739 13658 -81
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Klaim
force-pushed
the
env-lockfile-channels
branch
from
8209f96 to
61dd997
Compare
This reverts commit 61dd997.
…yet the url set when we generate the json log
Klaim
requested review from
AntoinePrv and
JohanMabille
and removed request for
JohanMabille
| // Adds mirror urls if not already recorded, by default added at the end of the mirrors | ||
| // list. | ||
| void add_mirror_urls( | ||
| const std::vector<CondaURL>& additional_mirrors, | ||
| UrlPriority priority = UrlPriority::low | ||
| ); | ||
|
|
There was a problem hiding this comment.
I don't have the full context here, but if possible it would be best if these could be part of ChannelParams/resolve workflow.
The general idea of specs::Channel is that with resolve you always obtain what you want, (pure functional no mutable changes). This is because currently with libsolv hacks etc, specs::Channel are rather short-lived, converted back and forth to strings. I believe functions like set_platforms are use for only local reasoning.
| // inject additional mirrors if not already existing in current channels | ||
| for (auto& channel : it->second) | ||
| { | ||
| // TODO C++23: replace all this by std::vector(from_range_t, ...) | ||
| auto urls_view = specs::as_conda_urls(mirrors); | ||
| std::vector<specs::CondaURL> urls(urls_view.begin(), urls_view.end()); | ||
|
|
||
| channel.add_mirror_urls(urls, new_mirrors_priority); | ||
| } |
There was a problem hiding this comment.
If this logic could be made part of ChannelParams/Channel::resolve that would avoid adding extra complexity to the channel creation logic.
|
I keep seeing issues with the tests locally but I suspect it's actually an unrelated problem, I might have to spam the ci a bit, sorry for the noise! |
2 participants
Description
So far the handling of environment lockfile read package specifications from them and used that to get the packages but the channels urls specified in the lockfile were read but then ignored.
This change makes libmamba use the urls specified by lockfiles, only for mambajs for now, and put them at the top of the priority of urls to use for each channel name (could be easily changed later if necessary).
The core of the feature is located in
MTransaction create_explicit_transaction_from_lockfile()(transaction.cpp), reviewers should start reading fromoAs part of that implementation:
ContextandContextChannelafter reading the lockfile -- this unfortunately happens after the channels has been created with other sources of information so it is a bit contrieved by the execution flow;containsstandard algorithm implementation that will be replaced by the standard once available;ChannelContext(see the change intrancation.cppinMTransaction::execute. This change has the potential to be breaking although we didn't find so far an explicit issue.MTransaction::executeso that it relies on the deduced information instead of approximations pre-execution.ChannelContext::make_channelnow allows adding new unique mirror urls to existing channels.Type of Change
Checklist
pre-commit run --alllocally in the source folder and confirmed that there are no linter errors.