fix: Recognize incompatible python wheels#5273
Conversation
|
This is for initial feedback only. I think this approach works, but I am sure somebody with more Conda-magic can shoot holes into it. |
ff59661 to
91f1e59
Compare
|
The latest version should still be slower than the current code as it does one extra iteration over all packages to find the python packages for each environment. This is only done when pypi packages are actually used now though:-) |
|
This looks correct to me, excellent! Did you check it against the original issue? |
baszalmstra
left a comment
There was a problem hiding this comment.
I left one comment where I think the code is more complex than it needs to be, but otherwise looks great!
| let checks = { | ||
| let mut tmp = Vec::new(); | ||
| if let Some(no_build) = group_pypi_options.no_build.as_ref() | ||
| && let Some(func) = verify_pypi_no_build(no_build) | ||
| { | ||
| // Check that if `no-build` is set, we only have binary packages | ||
| // or that the package that we disallow are not built from source | ||
| tmp.push(func); | ||
| } | ||
|
|
||
| // Check that all wheels still match with the system requirements | ||
| tmp.push(verify_pypi_wheel_tags(environment, locked_environment)); | ||
| tmp | ||
| }; | ||
|
|
||
| // Actually check all pypi packages in one iteration | ||
| for (platform, package_it) in locked_environment.pypi_packages_by_platform() { | ||
| for (package_data, package_env_data) in package_it { | ||
| for c in &checks { | ||
| (c)(platform, package_data, package_env_data)?; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Instead of creating a vec with boxed functions, couldnt we just build booleans, that indicate whether a certain thing needs to be checked and then use those in the inner for loop? I think that will improve readability.
There was a problem hiding this comment.
I went for boxed functions as that nicely captures all the necessary context.
It was pretty ugly to set up and pass around the state that the functions need.
Figure out that the lock file needs to be re-checked when system requirements change. Calculate the lost of possible tags a wheel can have and check that those match up to the wheels found in the wheel.
... for the wheel change
ed87513 to
5459ef6
Compare
|
The latest iteration re-visits the latest change only: I redid it without Boxed lambda functions :-) The rest is unchanged but rebased on top of current |
5459ef6 to
e206399
Compare
|
Satisfiability seems to be failing :) |
Do not over-complicate this: Use simple Check structs with a member function over Boxed function objects.
e206399 to
f819ba8
Compare
|
... and forgot to move the cargo-insta data, now that the PR fixing the test names is merged :-/ |
Description
Figure out that the lock file needs to be re-checked when system requirements change.
Calculate the lost of possible tags a wheel can have and check that those match up to the wheels found in the wheel.
Fixes #4607
How Has This Been Tested?
manually
AI Disclosure
Tools: None
Checklist: