-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Add build constraints #13534
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?
Add build constraints #13534
Conversation
60df4a0
to
d9d5f5d
Compare
…needing to use the `build-constraint` feature
6c44d43
to
8d170b5
Compare
Very, very much needed. Thanks @notatallshaw -> today |
Yanked now luckily. Looking forward to this one - I really like how this one will help us in the future to make both - our PRs and installation mechanisms we tell our users, to make them resilient to similar accidents broken packages released on PyPI. I really look forward to this one to make it part of our regular process. The existing workarounds are complicated and non-end-user friendly at all and would complicate our CI process quite a lot as we woudl have to combine regular and build constraints dynamically on our CI. |
Thanks @potiuk, a few things worth nothing though: This only allows users to protect themselves from breakages, not libraries. I feel like there is an open question about whether libraries (or popular applications) should provide upperbounds on their build dependencies. In fact it was recently discussed in pypa/packaging.python.org#1880 (comment) This doesn't technically introduce any new functionality if you have full control of pip you can reproduce all of this:
But this is poor UX, it means you don't have the usual CLI and ENV flexibility, and the fact this works the way it does depends on multiple internal details of how pip is implemented. In fact what finally inspired me to write this PR is that one of those internal details (installing build dependencies via calling pip in a subprocess) is likely to change soon. |
Yep. I agree 100% with everythi g you wrote. I already started lookng at implementing a co bo of co ateaints solution in our CI this morning - bit I was so relieved I did not have to (yet) as today's aetuptools-scm 9.1.0 was yanked - precisely because of the poor UX and the undocumented, internal behaviour and potential conflicts between build and regular constraints which has kinda unspecified behaviour. And - as for Airflow - we simply pin our build dependencies - because in our case reproducibility trumps potential security issues with build dependencies - and also it makes it easier for us because we are pure Python only so very few upstream users would actually use .sdist to build airflow - and if they do, they usually do it in controlled environments. |
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 looks quite good already! Thanks a lot for thinking through the scenarios and interactions thoroughly. I'm impressed with your unit and functional test.
This is my first review pass which mostly consists of minor comments. I'll want to do another pass before I approve.
A general note is that the codebase seems to have standardized around constraints
for --constraint
while this PR uses build_constraint
for --build-constraints
. I'd prefer that we use build_constraints
with a S internally for consistency, but I do acknowledge that constraints
is not consistent with the CLI.
src/pip/_internal/build_env.py
Outdated
if not self._constraints: | ||
return | ||
|
||
if not os.environ.get("PIP_CONSTRAINT"): | ||
return | ||
|
||
pip_constraint_files = [ | ||
f for f in os.environ["PIP_CONSTRAINT"].split() if f.strip() | ||
] | ||
if pip_constraint_files and pip_constraint_files == self._constraints: |
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 assume the scenario where the stored constraints do not match PIP_CONSTRAINT
mostly involve configuration files?
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 function got a little confused because I was initially going to throw the deprecation in the isolated build process, but given that doesn't output to the console by default that would have been a poor place to put it.
On reconsideration we should emit a deprecation if PIP_CONSTRAINT
is at all non-empty. I will update.
configuration files?
I was mostly hoping that users aren't using configuration files to have build processes be constrained. I don't really know how to detect that.
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.
Updated, check is far more simple now.
@@ -112,6 +164,8 @@ def install( | |||
kind: str, | |||
for_req: InstallRequirement | None, | |||
) -> None: | |||
self._deprecation_constraint_check() |
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.
We should probably issue this deprecation warning only once. In some scenarios, even a single build will trigger this deprecation twice as the build backend can request additional dependencies dynamically.
... although I say that having tried this locally... I can't get the deprecation to be printed twice. Hmm.
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.
Updated, I'm not so sure this prevents it being called in the isolated build sub-process, but it will only be called once per process at least.
src/pip/_internal/build_env.py
Outdated
class ExtraEnviron(TypedDict, total=False): | ||
extra_environ: dict[str, str] |
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.
What's the benefit of defining this typed dictionary? It seems superfluous IMO.
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 am all open to suggestions that don't make the code more awkward (e.g writing a much larger if block) and keep mypy happy.
I could move it into the if type checking block though to eliminate any run time construction.
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've moved in to type checking block, not sure what else to do.
src/pip/_internal/cli/cmdoptions.py
Outdated
def check_build_constraints(options: Values) -> None: | ||
"""Function for validating build constraint options. |
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.
Good forward thinking here!
Co-authored-by: Richard Si <[email protected]>
Co-authored-by: Richard Si <[email protected]>
I believe I have addressed all of @ichard26's review items as best as I can. |
Fixes #13300
This adds a new
--build-constraint
flag to complement the existing--constraint
flag, at a high level:PIP_CONSTRAINT
now requiresPIP_BUILD_CONSTRAINT
or simply--build-constraint
)--use-feature
flagbuild-constraint
is enabled, as not to break user workflowsPIP_CONSTRAINT
is present and in effect without using the new build constraintsThis change in behavior was desired because:
--constraint
and--build-constraint
)uv pip
has for--constraint
and--build-constraint
so is well verified in the real world