Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
GH-37929: [Python] begin moving static settings to pyproject.toml #41041
GH-37929: [Python] begin moving static settings to pyproject.toml #41041
Changes from all commits
bdb87f2
608cbb2
63d04ae
62ce04d
4259cf7
15c5c7a
327dd38
3fccbd2
2b5a063
9d6eaf2
f5fb1ed
74d0f29
50f8f3e
83140a3
d9bd3ce
11e0a32
07489af
b04b1fa
482eac1
3e82475
2c879c6
2c343e6
6f75c2b
9adab26
50a35c1
24054ef
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Any specific reason for this change? (I don't care to be clear, but I don't know how our general coverage is of ubuntu versions, and we should probably still test somewhere with focal)
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 are already testing Ubuntu 20.04 on test-ubuntu-20.04-python-3 .
I did update the minimal_build to 22.04 because there was a minor issue building and was easier to update than investigate to fix it and as we are already testing 20.04 I knew it was a CI set-up. I could investigate but I do think that we have to start updating our default jobs to 22.04 so I preferred to update it.
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.
Is this addition still needed? (my understanding is that
pip install
should not need it, since we also don't list it as a build dependency)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.
Ah, I see we actually do list
wheel
as a build requirement ;) But I think nowadays the recommendation is not listwheel
explicitly.While I can't find an explicit reference for this, the original PEP text included
wheel
in the simple example (https://peps.python.org/pep-0518/#build-system-table), but the examples for using setuptools in the packaging guide or on the setuptools documentation (https://setuptools.pypa.io/en/latest/userguide/quickstart.html#basic-use) no longer includewheel
.So I think we should remove
wheel
frombuild-system.requires
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.
Done
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.
@jorisvandenbossche It seems wheel is required otherwise we get the following failure:
See: https://github.com/ursacomputing/crossbow/actions/runs/9348063637/job/25726407074
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.
Ah, OK, diving in a bit more to understand what is happening: this is because we are using
--no-build-isolation
here. So it seems that setuptools will (in normal, isolated mode) automatically injectwheel
as a dependency when building a wheel, but so that doesn't happen in non-isolated mode.So it's still a good change to exlude
wheel
from the build requirements in pyproject.toml, but then we need to add it to the environments that use--no-build-isolation
.Note that explains this a bit in the setuptools docs (https://setuptools.pypa.io/en/latest/userguide/quickstart.html#basic-use):
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 may have missed something but is it really useful to exclude it from the build requirements? It's a pure Python package with zero non-trivial dependency, AFAICT.
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.
For example:
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.
That's the recommendation from setuptools ..
(I agree it shouldn't hurt given it's an easy to install package, although I also noticed in the setuptools tracker that the next version of setuptools will actually remove
wheel
usage all together)