-
Notifications
You must be signed in to change notification settings - Fork 31
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
cachi2 wheels: allow dependencies with only wheel distributions #319
Conversation
repo for manual testing |
if best_sdist.is_yanked: | ||
raise PackageRejected( | ||
f"All sdists for package {name}=={version} are yanked", |
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.
Hmm, being yanked is another thing we handle for sdists but not for wheels.
It might be hard to come up with any sane way to handle yanked-ness in an allow_binary context. Maybe we should just log a warning and download yanked things anyway (for consistency: even when allow_binary=False). That's how 'yanked' is supposed to work when the version is pinned, after all. https://peps.python.org/pep-0592/#installers
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.
But that's probably something for a follow-up story
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, in the scenario that a package has wheels but all sdists are yanked, the request would still fail, right?
Should we file the follow-up story right away so we don't forget about 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.
After adding a hash to your requirements.txt:
tensorflow is still reported with missing_hash {
"name": "tensorflow",
"purl": "pkg:pypi/tensorflow@2.13.0",
"version": "2.13.0",
"properties": [
{
"name": "cachi2:found_by",
"value": "cachi2"
},
{
"name": "cachi2:missing_hash:in_file",
"value": "requirements.txt"
}
],
"type": "library"
} |
I refactored almost the whole function
Did not update unit tests yet. |
Nice, for me it's a lot easier to understand now 👍 |
If a user enables `allow_binary option`, reject only those distributions which have no source and no wheel distributions STONEBLD-1641 Signed-off-by: Michal Šoltis <msoltis@redhat.com>
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.
LGTM 👍
I think there's a corner case where we delete all the wheels after downloading because of checksum mismatches so there are 0 archives left for a package, but that should be extremely rare. Probably not worth handling.
marking docs as N/A since there's a separate docs story |
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.
LGTM
If a user enables
allow_binary option
, reject only those distributions which have no source and nowheel distributions
STONEBLD-1641
Maintainers will complete the following section