Add always-install-via-wheel feature flag #9778
Conversation
sbidoul
force-pushed
the
no-binary-always-install-via-wheel-sbi
branch
from
42d3f75
to
6e66f48
Compare
src/pip/_internal/cache.py
Outdated
| def __init__(self, cache_dir, format_control=None): | ||
| # type: (str, Optional[FormatControl]) -> None | ||
| if format_control is None: | ||
| format_control = FormatControl() |
There was a problem hiding this comment.
Why not explicitly pass a fresh FormatControl() from the caller instead?
There was a problem hiding this comment.
Ultimately the wheel cache will not be concerned about format control at all, so I wanted to already expose a constructor without FormatControl.
There was a problem hiding this comment.
Our cache logic is very much tied to the FormatControl class, and I'd prefer to make this not make this optional. Having this be an explicit dependency was an intentional choice, to make it clearer what the object needs to function.
(we have enough implicit dependencies in the rest of the codebase. :P)
There was a problem hiding this comment.
Further thinking about this I'll see if I can make this PR not touch the caching use logic. Contrarily to what I initially thought it might be possible, and we can revisit the cache logic independently (which is the topic of #9162).
| def check_binary_allowed(req): | ||
| # type: (InstallRequirement) -> bool | ||
| if "always-install-via-wheel" in features_enabled: | ||
| return True |
There was a problem hiding this comment.
Would it be cleaner to have this logic outside of this function, in run() instead?
There was a problem hiding this comment.
I don't think so, because the point is to get a BinaryAllowedPredicate that is passed around down to _should_build.
sbidoul
force-pushed
the
no-binary-always-install-via-wheel-sbi
branch
4 times, most recently
from
d8c4571
to
3a92068
Compare
|
This should now be good for review again. I removed the part that touched wheel caching, as it turns out it's an orthogonal topic. |
|
The RTD failure seems unrelated ? |
|
Yes, because the towncrier-drafts plugin is broken now. |
|
Hello! I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the |
In itself, this commit does not change pip's behaviour, because the presence of --build-option and --global-option imply --no-binary :all: which in turn disable wheel building.
--no-binary does not disable wheel building anymore, so a wheel is built during the install process and setup.py install is not used anymore in this case --build-option and --global-option are passed to setup.py bdist_wheel in the pip install command, like it is done in the pip wheel command.
sbidoul
force-pushed
the
no-binary-always-install-via-wheel-sbi
branch
from
3a92068
to
5588997
Compare
|
@sbidoul Do you want this in 21.2? |
|
I will not be ready in time I'm afraid. If I can scrape some time I'll rather focus on landing PEP 660. |
github-actions
bot
added
the
needs rebase or merge
|
This continues in #11373. |
Towards #9769
Includes #9776
Add the
always-install-via-wheelfeature flag. When enabled, it has the following effects:wheelpackage is installed).--no-binarydoes not implysetup.py installanymore.--build-optionand--global-optionare passed tosetup.py bdist_wheelduring installation, as it was already done when usingpip wheel.--install-optionis ignored (because there is nosetup.py installinvolved)Although the code change is small the implications are many, because so many things are entangled with --no-binary / FormatControl. I believe this disentangling will ultimately be good both for the code base and usability.One thing that I might consider pushing further (in another PR) is the wheel cache control: now that a wheel can be cached as the result of an installation that includes--global-optionor--build-optionwe should probably include those options as part of the cache key.