-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Issue 2675 #2699
Issue 2675 #2699
Conversation
c889d84
to
c215f73
Compare
@@ -779,8 +791,8 @@ def _link_package_versions(self, link, search_name): | |||
link, 'Python version is incorrect') | |||
return | |||
logger.debug('Found link %s, version: %s', link, version) | |||
|
|||
return InstallationCandidate(search_name, version, link) | |||
link.canonical_name = search.canonical |
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.
Not a big fan of this extra attribute on Link
.
The canonical name could be passed instead as a new argument to cached_wheel
since it is called from InstallRequirement.link.setter
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'm not a fan either. What you propose won't work since we don't know the actual requested name in InstallRequirement all of the time. Perhaps if we say 'if we don't know it, then we can't cache it', will be sufficient.
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.
Well, if you don't have a name, it means you're not using an index/PyPI (or are providing a direct link to a file on PyPI). Not caching it seems ok.
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.
done. I hope. it passes tests anyhow :P
d40f665
to
b368775
Compare
|
||
def no_binary(): | ||
return Option( | ||
"--no-binary", dest="format_control", action="callback", |
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 don't like "binary". I think it should be "wheel". Are we considering other built package types?
In the Packaging User Guide, we specifically describe wheels generally as "built" packages. "Binary" is reserved for when wheels contain compiled extensions.
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 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, so there is a problem about "Binary" when it means compiled and "Binary" when it means wheels in general. One problem I'd like to address is baking the format names directly into the UI of pip. I was thinking about sdist 2.0 and what we'd end up calling that and an idea that popped into my head was "Source Wheel" (similar to RPM and Source RPM). In that vein the --use-wheel
moniker would be kind of confusing to people.
Maybe that was a bad idea, I don't know.
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.
WHL and sWHL..... catchy....
90c0c77
to
c6ba342
Compare
frozenset(formats)) | ||
canonical_name = pkg_resources.safe_name(project_name).lower() | ||
formats = fmt_ctl_formats(self.format_control, canonical_name) | ||
search = Search(project_name.lower(), canonical_name, formats) |
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 didnt catch it on the previous PR but it seems strange to set project_name.lower()
as the user supplied name (since the user obviously supplied project_name
).
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.
This is what the prior code was doing. I think its fine to want to change it but its unrelated to this goal, and since it might cause test fallout etc, I'd rather someone do that separately.
With wheel autobuilding in place a release blocker is some granular way to opt-out of wheels for known-bad packages. This patch introduces two new options: --no-binary and --only-binary to control what archives we are willing to use on both a global and per-package basis. This also closes pypa#2084
@@ -1,5 +1,6 @@ | |||
from __future__ import absolute_import | |||
|
|||
import pip |
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.
This should import like from pip.index import FormatControl
or import pip.index
.
can we have the changelog mention the new options by name, and also the deprecation of |
See #2722 |
Issue #2675: Granular control over wheels/sdists
With wheel autobuilding in place a release blocker is having some granular way to opt-out of wheels for known-bad packages. This patch introduces two new options: --no-binary and --only-binary to control what archives we are willing to use on both a global and per-package basis.
This also closes #2084