-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Install build dependencies in-process #13450
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
base: main
Are you sure you want to change the base?
Conversation
I should probably be explicit with what I want from people right now. I simply want feedback on the general idea. Is this something we want to pursue given the inherent additional complexity? If so, I'll add tests, write proper commit messages, and break this up into smaller PRs so this is easier to review. For the rational, please see #9081 (TL;DR, it will be faster, fix a bunch of bugs with passing the right arguments down, allow for better error handling, and clear the way to remove some ugly hacks in the codebase) If not, I'll abandon this. |
To help put the extra code into context, I've prototyped what the removal of the legacy installer would look like. In the 10 minutes I spent on this, this is the result: $ git sdiff
src/pip/__pip-runner__.py | 50 -------------
src/pip/_internal/build_env.py | 82 +---------------------
src/pip/_internal/commands/install.py | 4 +-
src/pip/_internal/commands/wheel.py | 4 +-
src/pip/_internal/index/package_finder.py | 31 --------
.../_internal/operations/build/build_tracker.py | 75 ++------------------
6 files changed, 10 insertions(+), 236 deletions(-) |
+1 from me on the general idea. |
FYI, I was planning to get back to working on #13300 soonish, but looking over this PR I see no issue with working on both at the same time, there will only be a small overlap, just at the point of passing the flags to the build dependency installation command. |
Talking with @ichard26, out of band, my above comment is completely wrong, and I think we need to make a choice on this. The current behavior: This PRs Now, while I understand that the current behavior was probably not intentional I do think it's advantageous to be able to have build constraints and regular constraints as separate. And I am concerned with this as a breaking change. But if this is going to be a breaking change I have a proposal to clean everything up: Match uv's behavior, add a build constraint flag, and have regular constraints NEVER apply to the build environments, and have build constraints apply to ALL build environments:
From a user's perspective this will be much more clean and obvious as to what each flag is doing, the new I am willing to figure out how to wrangle the constraints vs build constraints and have it as a separate PR to land after the inprocess-build-deps lands, if @ichard26 is not up for that work. Whether this proposal is agreed to or not, I think it's important that a constraint design is agreed upon before merging this. |
This is also not exactly correct. The current behaviour is that neither I agree that this would be a good opportunity to clean up the constraint handling though. |
Alrighty. I've broken this up into a handful of smaller PRs. Reviews on the following would be appreciated :) |
Do those 3 PRs cover everything in this PR or are intended to be the building steps before actually adding the feature that users can enable? |
Building blocks. The actual feature will be added in a separate PR (or more likely, I'll force push to this PR) once all of the prerequisite building blocks have been landed. |
Towards #7294, #9081, and its associated issues.
This PR adds an alternative build environment dependency installer that installs in-process, using a stripped down reimplementation of the install command. It's disabled by default. To use the in-process installer, pass
--use-feature inprocess-build-deps
.Building pip from source with setuptools cached,
inprocess-build-deps
shaves ~300 ms (1850ms -> 1530ms).This is a draft as there are no new tests. I'm opening this now to see whether there is even any interest in pursing this feature.
I'll note that the plan would be to: