-
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
Added HTTP Mirrors #3178
Added HTTP Mirrors #3178
Conversation
575fe6c
to
573d687
Compare
a8f37e0
to
d1bbf54
Compare
8ea280c
to
506a875
Compare
a81a9e1
to
33e39d1
Compare
std::string_view url, | ||
const std::string& channel_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.
Shouldn't we rather take a specs::Channel
?
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.
We could but that would require to store the channel in the SubdirData (which is where we get the channel_id
), which means either duplicate it from the ChannelContext
, or ensure the ChannelContext
will live longer than the subdirs.
Besides, I'm not sure we need the entire channel information here, the channel_id
is used to retrieve the mirrors stored in the Context; or we could refactor that to have the mirrors stored in the subdir directly (but again, this refactoring is beyond the scope of this PR).
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.
The channel have value semantics so it does not need to be stored anywhere.
We may not need all the channel information but that make the API more clear as to what to expected and where to get it.
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.
The channel have value semantics so it does not need to be stored anywhere.
I think these are unrelated. Unless you want to resolve channels every time you need them, you have to store the channels somewhere once you have created them. And if f we want to avoid passing the Context
and the ChannelContext
everywhere, we should definitely create the channels once and for good at the "beginning".
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.
Aren't channels already stored in the channel context for caching purposes? Plus they are not that expensive to resolve.
One of the point of the Channel refactor was to make them first class citizen, I feel this is going backwards, putting strings everywhere.
One difficulty though is that this is not even the URL to a channel but a subdir, for which we do not have information...
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.
Aren't channels already stored in the channel context for caching purposes? Plus they are not that expensive to resolve.
The issue is not they are expensive to resolve, but rather that you need the ChannelContext to resolve them (and then you need to pass it everywhere).
One of the point of the Channel refactor was to make them first class citizen, I feel this is going backwards, putting strings everywhere.
The string is only for the mapping channel <=> mirrors. We could actually change that for a "strong" typedef (ChannelId), but we definitely don't need the entire channel object here; and actually, a subdir is a "sub" channel, for a given platform,. If we store a channel in the subdir, we then may have a consistency issue with those in the ChannelContext; avoiding this consistency means either storing a reference (but then we need to ensure the values live longer than the references), or removing all the setters from the channels (which actually would be my preference).
So it's not going backward, but rather an intermediate state before we refactor the SubdirData and the cache in the ChannelContext.
std::string_view(".tar.bz2"), | ||
std::string_view(".conda"), | ||
}; | ||
inline static constexpr auto ARCHIVE_EXTENSIONS = std::array{ std::string_view(".tar.bz2"), |
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 split that in two? Or even just hard code it where we need it?
I fear adding pypi code in Conda abstactions will just make it harder on the long run.
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 split that in two? Or even just hard code it where we need it?
I'm afraid that would lead to more bugs (because we'll have to split the code everywhere, and remember to add this specific use case each time we add a feature for regular arhcives. Also that would duplicate some code (since archives trigger the same behavior where they are identified).
The correct fix on the long run would be to have full abstrcations for pip packages (and later for other packages managers we may want to support), but that's beyond the scope of this PR.
8973fca
to
025bb8b
Compare
No description provided.