Skip to content
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

Stop injecting wheel as a build dep fallback #12449

Merged
merged 2 commits into from
Jan 9, 2024

Conversation

webknjaz
Copy link
Member

PEP 517 doesn't mandate depending on wheel when a __legacy__ setuptools fallback is used. Historically, it used to be assumed as necessary, but later it turned out to be wrong. The reason is that setuptools' get_requires_for_build_wheel() hook already injects this dependency when building wheels is requested [1]. It also used to have this hint in the docs, but it was corrected earlier [2].

It could be argued that this is an optimization as pip will request building wheels anyway. However, it also shows up in the docs, giving the readers a wrong impression of what to put into [build-system].requires when they start a new project using setuptools.

This patch removes wheel from said requires list fallback in the docs and the actual runtime.

PEP 517 doesn't mandate depending on `wheel` when a `__legacy__` setuptools
fallback is used. Historically, it used to be assumed as necessary, but
later it turned out to be wrong. The reason is that `setuptools`'
`get_requires_for_build_wheel()` hook already injects this dependency when
building wheels is requested [[1]]. It also used to have this hint in
the docs, but it was corrected earlier [[2]].

It could be argued that this is an optimization as `pip` will request
building wheels anyway. However, it also shows up in the docs, giving
the readers a wrong impression of what to put into
`[build-system].requires` when they start a new project using setuptools.

This patch removes `wheel` from said `requires` list fallback in the
docs and the actual runtime.

[1]: https://github.com/pypa/setuptools/blob/v40.8.0/setuptools/build_meta.py#L130
[2]: pypa/setuptools#3056
@webknjaz webknjaz added type: bug A confirmed bug or unintended behavior type: docs Documentation related project: setuptools Related to setuptools PEP implementation Involves some PEP C: PEP 517 impact Affected by PEP 517 processing labels Dec 23, 2023
@webknjaz webknjaz force-pushed the maintenance/pep517-fallback-wheel branch from b6dae52 to 3769ad7 Compare December 23, 2023 01:18
@webknjaz
Copy link
Member Author

Motivation/backstory: I was reading https://gregoryszorc.com/blog/2023/10/30/my-user-experience-porting-off-setup.py/ (it was shared @ pypa/packaging.python.org#1468) and it got me puzzled — I thought I'd gotten rid of all the prominent places newbies may copy-and-paste the suggestion to depend on wheel unconditionally...

@webknjaz
Copy link
Member Author

Similar PR in build: pypa/build#716.

@chrysle
Copy link
Contributor

chrysle commented Jan 9, 2024

This is also still recommended in PyPA's sample project:

https://github.com/pypa/sampleproject/blob/main/pyproject.toml#L153

@pfmoore
Copy link
Member

pfmoore commented Jan 9, 2024

This is also still recommended in PyPA's sample project

That should be raised separately in that repo.

@pfmoore pfmoore merged commit 64d8938 into pypa:main Jan 9, 2024
26 checks passed
@pfmoore
Copy link
Member

pfmoore commented Jan 9, 2024

Thanks for this, @webknjaz!

@webknjaz
Copy link
Member Author

Thanks, @pfmoore!
I noticed that the merge commit's CI workflow run is failing: https://github.com/pypa/pip/commit/64d89385ce4b5ae2d05d176e7746bff0baaabafd/checks.

Is there any chance that could be related to this patch? It doesn't seem to be the cause but is still worth checking...

@webknjaz
Copy link
Member Author

It should be easy to verify by restarting the CI (or at least one job) of the previous commit — https://github.com/pypa/pip/commit/e88d39ae49ab11a8b80609a018e5a36ea4ccad89/checks.

@pfmoore
Copy link
Member

pfmoore commented Jan 12, 2024

I don't believe it was this run, but I've hit restart just to check.

@notatallshaw
Copy link
Member

FYI CI runs are failing because of changes in setuptools which had a significant impact on Pip tests: #12458

@webknjaz
Copy link
Member Author

@notatallshaw thanks for confirming! I had a hunch that maybe the test env isn't pinned well enough, but was not fully sure.

@pfmoore
Copy link
Member

pfmoore commented Jan 12, 2024

Thanks @notatallshaw - I had a suspicion it was that.

@webknjaz I'm not sure we should be pinning setuptools in the test env. The occasional failure, while annoying, is overall probably less so than having to update our pins at the sort of rate setuptools changes (and in general, I think we do want to know we work with the latest setuptools).

@webknjaz
Copy link
Member Author

webknjaz commented Jan 12, 2024

Understandably so. OTOH, there's a middle ground — use pins for PRs and merges but add unpinned tests for nightly/cron runs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bot:chronographer:provided C: PEP 517 impact Affected by PEP 517 processing PEP implementation Involves some PEP project: setuptools Related to setuptools type: bug A confirmed bug or unintended behavior type: docs Documentation related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants