-
Notifications
You must be signed in to change notification settings - Fork 353
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
Implemented support for mirrors #3157
Conversation
7cb3b6e
to
7efd632
Compare
1c5aa2c
to
424e1db
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My first few comments to get started since this seems to still be a moving target.
std::ofstream out("trace.txt", std::ios_base::app); | ||
out << "configuring CURL handle:" | ||
<< "\n\turl = " << url << std::endl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this trace.txt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A leftover, I need to remove it (and the other occurences), I just wanted to get all the tests green before removing them.
@@ -109,6 +109,7 @@ namespace mamba::specs | |||
auto clear_platforms() -> platform_list; | |||
void set_platforms(platform_list platforms); | |||
|
|||
[[nodiscard]] auto id() const -> const std::string&; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we a add a doc saying what this is (a cross URL id) and its limitations (dependent on channel_alias
).
libmamba/src/specs/channel.cpp
Outdated
@@ -45,6 +45,7 @@ namespace mamba::specs | |||
Channel::Channel(std::vector<CondaURL> mirror_urls, std::string display_name, platform_list platforms) | |||
: m_mirror_urls(std::move(mirror_urls)) | |||
, m_display_name(std::move(display_name)) | |||
, m_id(m_display_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's still not making it a proper ID, but its something.
, m_id(m_display_name) | |
, m_id(util::rstrip(m_display_name, '/')) |
auto Channel::id() const -> const std::string& | ||
{ | ||
return m_id; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we consider
auto operator==(Channel const& lhs, Channel const& rhs) -> bool {
return lhs.id() == rhs.id();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok let's revert this change for now, the current Channel's API is not really consistent with that comparison operator (especially because we can change attributes of an already created channel, without having its ID changed.).
424e1db
to
9ff330a
Compare
9ff330a
to
2ac58d8
Compare
@AntoinePrv I've addressed the comments but the last one, I think this one is ready to go. |
Bravo ! This is getting close. |
@@ -10,7 +10,7 @@ | |||
|
|||
namespace mamba | |||
{ | |||
TEST_SUITE("mirror") | |||
/*TEST_SUITE("mirror") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason the tests are commented here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I have started to partially move the mirror implementation to the download folder / namespace. Since I want to make incremental steps, I have not port everything yet, but I had to remove core/mirror.cpp
from the CMakeLists.txt to avoid symbol duplication, and I had to comment this test until the missing functions are ported to download
.
// This class is used to create strong alias on | ||
// string_view. This helps to avoid error-prone | ||
// calls to functionsthat accept many arguments | ||
// of the same type | ||
template <int I> | ||
class string_view_alias | ||
{ | ||
public: | ||
|
||
explicit string_view_alias(std::string_view s); | ||
operator std::string_view() const; | ||
|
||
private: | ||
|
||
std::string_view m_wrapped; | ||
}; | ||
|
||
using MirrorName = string_view_alias<0>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not?
// This class is used to create strong alias on | |
// string_view. This helps to avoid error-prone | |
// calls to functionsthat accept many arguments | |
// of the same type | |
template <int I> | |
class string_view_alias | |
{ | |
public: | |
explicit string_view_alias(std::string_view s); | |
operator std::string_view() const; | |
private: | |
std::string_view m_wrapped; | |
}; | |
using MirrorName = string_view_alias<0>; | |
struct MirrorName | |
{ | |
std::string_view name; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because initially I also wanted to have aliases for the url_path
and why not thename
fields of the request, and I found this solution more elegant than repeating the same code for wrapping the same type. I had some problem with wrapping the url that I postponed to a future PR. Ideally, this structure should be moved to some util files, and accept an additional template argument (for the type it should alias), and could be used for aliasing other IDs, such as the Channel ID for instance. But I agree it may seem overkill actually, and I can remove it in a future PR if we decide to alias the MirrorName only.
A minor difference is that the structure forces a {} initialization while the alias lets the choice in the initialization syntax.
std::string MirrorID::to_string() const | ||
{ | ||
return fmt::format("MirrorID <{}>", m_value); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we write a custom fmt::formatter
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would have to be friend to access the m_value
member, and that would make the code heavier. compared to this single line. I am not super familiar with the internals of FMT though, does a custom fmt::formatter
have some advantages over calling fmt::format
in this case?
@@ -0,0 +1,169 @@ | |||
#include <fmt/format.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#include <fmt/format.h> | |
// Copyright (c) 2023, QuantStack and Mamba Contributors | |
// | |
// Distributed under the terms of the BSD 3-Clause License. | |
// | |
// The full license is in the file LICENSE, distributed with this software. | |
#include <fmt/format.h> |
@@ -0,0 +1,51 @@ | |||
#include "mamba/download/mirror_map.hpp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#include "mamba/download/mirror_map.hpp" | |
// Copyright (c) 2023, QuantStack and Mamba Contributors | |
// | |
// Distributed under the terms of the BSD 3-Clause License. | |
// | |
// The full license is in the file LICENSE, distributed with this software. | |
#include "mamba/download/mirror_map.hpp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs more documentation and maybe some tweaks, but LGTM in general.
DownloadOptions options = {}, | ||
DownloadMonitor* monitor = nullptr | ||
Options options = {}, | ||
Monitor* monitor = nullptr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note: this could be a tl::optional<Monitor>
too, as tl::optional
handles references (not the standard one, unfortunately - being discussed again BTW)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but I am a bit reluctant to add another dependency just for this here.
MirrorRequest& operator=(MirrorRequest&&) = default; | ||
}; | ||
|
||
class Mirror |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to have some documentation here about what this specific type is about for this PR .
********************************/ | ||
|
||
/* | ||
* MirrorAttempt FSM: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting way to specify it! 👍🏽
libmamba/src/download/downloader.cpp
Outdated
* LAST_REQUEST_FAILED: | ||
* - m_retries == p_mirror->max_retries ? => SEQUENCE_FAILED | ||
*/ | ||
MirrorAttempt::MirrorAttempt(Mirror* mirror) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is not valid to take nullptr
, then replace this pointer by a reference and store in the member pointer (take dependency by reference, store by pointer). Then add an assertion in the constructor body about this pointer, just to clarify for future maintainers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmh not sure about the assertion here, if the constructor takes the mirror parameter by reference (and I agree that it should), then this parameter has a valid address and it is clear the pointer cannot be null.
d7099f2
to
51eb4d3
Compare
No description provided.