PR#437: [NOT SURE] Stop injecting wheel as a build backend dependency fallback. - rpms/pyproject-rpm-macros

rpms / pyproject-rpm-macros

#437 [NOT SURE] Stop injecting wheel as a build backend dependency fallback.

Closed 3 months ago by churchyard. Opened 3 months ago by churchyard.

rpms/ churchyard/pyproject-rpm-macros nowheel into rawhide

Stop injecting wheel as a build backend dependency fallback.

Miro Hrončok • 3 months ago

d99a710

macros.pyproject

file modified

-1

		`@@ -193,7 +193,6 @@`
		`elif [ -f setup.py ]; then`
		`# Note: If the default requirements change, also change them in the script!`
		`echo 'python%{python3_pkgversion}dist(setuptools) >= 40.8'`
		`- echo 'python%{python3_pkgversion}dist(wheel)'`
		`else`
		`echo 'ERROR: Neither pyproject.toml nor setup.py found, consider using %%%%pyproject_buildrequires -N <requirements-file> if this is not a Python package.' >&2`
		`exit 1`

pyproject-rpm-macros.spec

file modified

+6 -1

		`@@ -13,7 +13,7 @@`
		`# Increment Y and reset Z when new macros or features are added`
		`# Increment Z when this is a bugfix or a cosmetic change`
		`# Dropping support for EOL Fedoras is not considered a breaking change`
		`- Version: 1.12.0`
		`+ Version: 1.13.0`
		`Release: 1%{?dist}`

		`# Macro files`
		`@@ -171,6 +171,11 @@`


		`%changelog`
		`+ * Sat Feb 03 2024 Miro Hrončok <mhroncok@redhat.com> - 1.13.0-1`
		`+ - Stop injecting wheel as a build backend dependency fallback.`
		`+ 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.`
		`+`
		`* Fri Jan 26 2024 Miro Hrončok <miro@hroncok.cz> - 1.12.0-1`
		`- Namespace pyproject-rpm-macros generated text files with %%{python3_pkgversion}`
		`- That way, a single-spec can be used to build packages for multiple Python versions`

pyproject_buildrequires.py

file modified

-1

		`@@ -248,7 +248,6 @@`
		`# with pyproject.toml without a specified build backend.`
		`# If the default requirements change, also change them in the macro!`
		`requirements.add('setuptools >= 40.8', source='default build backend')`
		`- requirements.add('wheel', source='default build backend')`

		`requirements.check(source='build backend')`

pyproject_buildrequires_testcases.yaml

file modified

-22

		`@@ -10,7 +10,6 @@`
		`# empty`
		`expected: \|`
		`python3dist(setuptools) >= 40.8`
		`- python3dist(wheel)`
		`except: FileNotFoundError`

		`Insufficient version of setuptools:`
		`@@ -23,7 +22,6 @@`
		`setup.py: \|`
		`expected: \|`
		`python3dist(setuptools) >= 40.8`
		`- python3dist(wheel)`
		`result: 0`

		`No pyproject.toml, empty setup.py:`
		`@@ -35,7 +33,6 @@`
		`expected: \|`
		`python3dist(setuptools) >= 40.8`
		`python3dist(wheel)`
		`- python3dist(wheel)`
		`result: 0`

		`Default build system, empty setup.py:`
		`@@ -50,7 +47,6 @@`
		`expected: \|`
		`python3dist(setuptools) >= 40.8`
		`python3dist(wheel)`
		`- python3dist(wheel)`
		`result: 0`

		`pyproject.toml with build-backend and setup.py:`
		`@@ -215,7 +211,6 @@`
		`expected: \|`
		`python3dist(setuptools) >= 40.8`
		`python3dist(wheel)`
		`- python3dist(wheel)`
		`python3dist(foo)`
		`(python3dist(bar) < 2 or python3dist(bar) > 2)`
		`(python3dist(baz) >= 1.1.1 with python3dist(baz) < 1.2)`
		`@@ -237,7 +232,6 @@`
		`expected: \|`
		`python3dist(setuptools) >= 40.8`
		`python3dist(wheel)`
		`- python3dist(wheel)`
		`python3dist(pyyaml)`
		`python3dist(inst) > 1.0`
		`python3dist(inst2) < 3~~`
		`@@ -290,7 +284,6 @@`
		`expected: \|`
		`python3dist(setuptools) >= 40.8`
		`python3dist(wheel)`
		`- python3dist(wheel)`
		`python3dist(setuptools) >= 40`
		`python3dist(py) >= 1.5`
		`python3dist(six) >= 1.10`
		`@@ -313,7 +306,6 @@`
		`expected: \|`
		`python3dist(setuptools) >= 40.8`
		`python3dist(wheel)`
		`- python3dist(wheel)`
		`python3dist(setuptools) >= 40`
		`python3dist(py) >= 1.5`
		`python3dist(six) >= 1.10`
		`@@ -351,7 +343,6 @@`
		`expected: \|`
		`python3dist(setuptools) >= 40.8`
		`python3dist(wheel)`
		`- python3dist(wheel)`
		`python3dist(dep4)`
		`python3dist(dep4[bar])`
		`python3dist(dep4[foo])`
		`@@ -374,7 +365,6 @@`
		`expected: \|`
		`python3dist(setuptools) >= 40.8`
		`python3dist(wheel)`
		`- python3dist(wheel)`
		`python3dist(setuptools) >= 40`
		`python3dist(pip) >= 19`
		`python3dist(py) >= 1.5`
		`@@ -419,7 +409,6 @@`
		`- \| # tox 3`
		`python3dist(setuptools) >= 40.8`
		`python3dist(wheel)`
		`- python3dist(wheel)`
		`python3dist(tox-current-env) >= 0.0.6`
		`python3dist(toxdep1)`
		`python3dist(toxdep2)`
		`@@ -427,7 +416,6 @@`
		`- \| # tox 4`
		`python3dist(setuptools) >= 40.8`
		`python3dist(wheel)`
		`- python3dist(wheel)`
		`python3dist(tox-current-env) >= 0.0.6`
		`python3dist(tox)`
		`python3dist(toxdep1)`
		`@@ -471,7 +459,6 @@`
		`- \| # tox 3`
		`python3dist(setuptools) >= 40.8`
		`python3dist(wheel)`
		`- python3dist(wheel)`
		`python3dist(tox-current-env) >= 0.0.6`
		`python3dist(toxdep)`
		`python3dist(inst)`
		`@@ -485,7 +472,6 @@`
		`- \| # tox 4`
		`python3dist(setuptools) >= 40.8`
		`python3dist(wheel)`
		`- python3dist(wheel)`
		`python3dist(tox-current-env) >= 0.0.6`
		`python3dist(tox)`
		`python3dist(toxdep)`
		`@@ -528,7 +514,6 @@`
		`- \| # tox 3`
		`python3dist(setuptools) >= 40.8`
		`python3dist(wheel)`
		`- python3dist(wheel)`
		`python3dist(tox-current-env) >= 0.0.6`
		`python3dist(tox) >= 3.999`
		`python3dist(setuptools) > 40.0`
		`@@ -536,7 +521,6 @@`
		`- \| # tox 4`
		`python3dist(setuptools) >= 40.8`
		`python3dist(wheel)`
		`- python3dist(wheel)`
		`python3dist(tox-current-env) >= 0.0.6`
		`python3dist(tox) >= 3.999`
		`python3dist(setuptools) > 40.0`
		`@@ -572,7 +556,6 @@`
		`- \| # tox 3`
		`python3dist(setuptools) >= 40.8`
		`python3dist(wheel)`
		`- python3dist(wheel)`
		`python3dist(tox-current-env) >= 0.0.6`
		`python3dist(tox) >= 3.5`
		`python3dist(setuptools) > 40.0`
		`@@ -582,7 +565,6 @@`
		`- \| # tox 4`
		`python3dist(setuptools) >= 40.8`
		`python3dist(wheel)`
		`- python3dist(wheel)`
		`python3dist(tox-current-env) >= 0.0.6`
		`python3dist(setuptools) > 40.0`
		`python3dist(tox) >= 3.5`
		`@@ -614,14 +596,12 @@`
		`- \| # tox 3`
		`python3dist(setuptools) >= 40.8`
		`python3dist(wheel)`
		`- python3dist(wheel)`
		`python3dist(tox-current-env) >= 0.0.6`
		`python3dist(setuptools) > 40.0`
		`python3dist(wheel) > 2.0`
		`- \| # tox 4`
		`python3dist(setuptools) >= 40.8`
		`python3dist(wheel)`
		`- python3dist(wheel)`
		`python3dist(tox-current-env) >= 0.0.6`
		`python3dist(setuptools) > 40.0`
		`python3dist(wheel) > 2.0`
		`@@ -685,7 +665,6 @@`
		`(python3dist(sqlalchemy) < 1.1~~ with python3dist(sqlalchemy) >= 1.0.10)`
		`python3dist(setuptools) >= 40.8`
		`python3dist(wheel)`
		`- python3dist(wheel)`
		`result: 0`

		`With pyproject.toml, requirements file and with -N option:`
		`@@ -872,7 +851,6 @@`
		`expected: \|`
		`python3dist(setuptools) >= 40.8`
		`python3dist(wheel)`
		`- python3dist(wheel)`
		`result: 0`

		`pyproject.toml with runtime dependencies:`

churchyard commented 3 months ago

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.

See the same change in pip and build:

https://github.com/pypa/pip/pull/12449
https://github.com/pypa/build/pull/716

I believe this is the correct thing to do.
Even the expected outputs in the tests now look more reasonable.

However, this could lead to minor slowdowns in builds for packages with no build backend specified.
Previously, setuptools+wheel would get installed in the first round of %generate_buildrequires.
Now, setuptools gets installed in the first round and wheel in the second.

I slightly prefer correctness over minor speedup, but I am not 100% convinced.

zuul commented 3 months ago

Build succeeded.
https://fedora.softwarefactory-project.io/zuul/buildset/49af323fa1e64749a0bd1c275da96bd3

check-for-arches : SUCCESS in 1m 28s
check-for-eln : SUCCESS in 17s
eln-rpm-scratch-build : SUCCESS in 9m 56s
rpm-scratch-build : SUCCESS in 8m 21s
rpm-scratch-build-s390x : SUCCESS in 7m 04s (non-voting)
rpm-scratch-build-ppc64le : SUCCESS in 9m 32s (non-voting)
rpm-scratch-build-i686 : SUCCESS in 7m 43s (non-voting)
rpm-scratch-build-aarch64 : SUCCESS in 9m 31s (non-voting)
rpm-linter : SUCCESS in 4m 17s (non-voting)
rpm-rpminspect : SUCCESS in 8m 13s (non-voting)
check-for-sti-tests : SUCCESS in 15s
check-for-fmf-tests : SUCCESS in 15s
rpm-install-test : SUCCESS in 1m 20s
rpm-sti-test : SUCCESS in 29m 28s
Skipped 1 job

gotmax23 commented 3 months ago

I think it's a bit more than a minor speedup. An execute rpmbuild, unpack sources, patch sources, run %pyproject_buildrequires, run dnf builddep cycle takes a while. pip and build don't have these considerations as much.

That said, it's still the same question: correctness or speed. Here, I don't really see the purpose in removing wheel given that we know we'll always need it. Tools like pypa build builds wheels and sdists while we only build wheels here.

churchyard commented 3 months ago

OK, let's keep it, but document it: https://src.fedoraproject.org/rpms/pyproject-rpm-macros/pull-request/438

Pull-Request has been closed by churchyard

3 months ago