-
Notifications
You must be signed in to change notification settings - Fork 180
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
Feature/channel specific dependencies #439
Feature/channel specific dependencies #439
Conversation
…with requirements.
pub host_dependencies: Option<IndexMap<String, NamelessMatchSpec>>, | ||
|
||
/// The build-dependencies of the project. | ||
#[serde(default, rename = "build-dependencies")] | ||
#[serde_as(as = "Option<IndexMap<_, DisplayFromStr>>")] | ||
#[serde_as(as = "Option<IndexMap<_, PickFirst<(_, DisplayFromStr)>>>")] |
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 does PickFirst
do?
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 picks the first serialization that succeeds. This can now be a map or a string.
https://docs.rs/serde_with/latest/serde_with/struct.PickFirst.html
src/environment.rs
Outdated
|
||
// Test if the locked channel is equal to the channel that is requested. | ||
if let Some(channel) = &spec.channel { | ||
if !conda.url.as_ref().contains(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.
I think in the future we should have a proper channel type and compare the real "channels" instead of contains
(I am assuming this would find conda-forge
in the full URL, right?
This is not required for this PR IMO, but would be a good future enhancement.
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.
Yeah this seems very fragile. This will also work for pytorch with the pytorch-nightly channel for instance which might be very confusing. Maybe just compare the base_url since I think thats whats usually stored in the channel field.
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.
Yeah I completely agree. Just don't know how to fix it correctly. I guess we need to do some normalization after the parsing. IIRC, I left a todo in rattler about this.
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 made the check much more precise.
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.
Looks great!
let channel = "conda"; | ||
let url = "https://conda.anaconda.org/conda-forge/some_package"; | ||
assert!(!check_channel_package_url(channel, url)); |
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.
@wolfv This tests that, but with conda instead of pytorch
Works together with conda/rattler#394