-
Notifications
You must be signed in to change notification settings - Fork 63
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
Move index functionality to rattler and create python bindings #436
Conversation
@wolfv @baszalmstra Hey! This is my first attempt at doing what we discussed of moving the index functionality to rattler. There are some higher level design decisions that happy to talk about further like I created a new crate for the index logic, and the interface I created for the python function may not be what you expected, so let me know what you think. I didn't change any of the indexing logic. I tried running most of the CI tasks manually but still might need to fix some things, could I get approved for running CI? |
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 good! I left some superficial comments.
py-rattler/Cargo.toml
Outdated
@@ -48,7 +49,7 @@ tokio = { version = "1.32" } | |||
thiserror = "1.0.44" | |||
url = "2.4.1" | |||
|
|||
openssl = { version = "0.10", optional = true } | |||
openssl = { version = "0.10", optional = true, features = ["vendored"] } |
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 have features in this crate that control this behavior. See rustls-tls, nativetls and openssl-vendored.
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.
So I added this just as an attempt to fix the issues I'm seeing in the CI related to the openssl-sys crate. Do you have any idea what's going on? I ran this successfully on a linux machine I have access to, and when I uninstalled libssl-dev then I got a similar error message, but not sure how my PR could have caused this. Possibly the latest version of openssl-sys changed? Curious if you've seen this before
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.
There are two libraries that provide TLS for our networking operations (on linux). openssl
(through native-tls
) and rustls
. We default to use native-tls
(so openssl
on linux) but allow using rustls
. We use rustls
for the single executable binaries of pixi
but use openssl
when building for conda-forge.
The problem is that if one rattler crate depends on another rattler crate you need to propagate the proper tls features otherwise the "default" features "leak" through the compilation.
In CI we add the proper features. When we build the wheels for py-rattler we add the vendored-openssl
feature.
So, in essence, you can remove this change and use features to get the desired behavior for different targets.
@BenjaminLowry to get rid of the Thanks for doing this, by the way, I am excited to get rid of this in rattler-build! |
@wolfv Yeah absolutely! I'm excited to start using it too. Fixed the verification |
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 left two small comment but otherwise looks good to me!
@baszalmstra Thanks, addressed! |
Thanks! If you can remove the vendored feature from openssl then Ill merge! |
Ah that is already the case! Thanks! |
Moves the functionality for indexing a local conda channel from rattler-build to a new crate in rattler and create python bindings so that you can index dependencies via py-rattler as faster alternative to using something like conda index. This could happen after solving and downloading dependencies from the python bindings, for example.
This would replace the logic in rattler-build and thus I will create a follow up PR to remove this logic and instead use this new crate in rattler-build.