-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Move metadata to pyproject.toml #9951
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: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #9951 +/- ##
=======================================
Coverage 98.76% 98.76%
=======================================
Files 129 129
Lines 43374 43374
Branches 2323 2323
=======================================
Hits 42837 42837
Misses 383 383
Partials 154 154
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
CodSpeed Performance ReportMerging #9951 will not alter performanceComparing Summary
|
bba81fd
to
f718fa1
Compare
Let's not rush this. Keeping a dedicated config is a conscious choice. Also, PRs must not reformat half a project along with changing the packaging configs. FWIW I'd rather keep it as is for now. Especially, since I was going to sync the configs across the projects and this is going to cause a lot of headache since it conflicts with how things are now. I'm 👎 on this migration right now. It'd be interesting to gather the current dependency limitations in the downstreams to make an informed choice. Additionally, the move would have to happen in the dependencies first. |
That's actually a bug with the current black config. Since it can't detect the lowest supported version, it assumes
I'm probably missing something here. This PR doesn't affect any downstream projects. The project metadata change is minimal and spec compliant. Tbh I just thought it might be a good time to move the static metadata after I learned that it can work fine with There is no strict need to do that now, so if you're opposed feel free to close the PR. |
0d487cf
to
92f533f
Compare
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 personally don't care where the project metadata sits until it works and I personally shouldn't spend additional time on the infrastructure maintenance.
@cdce8p could you please extract a fix for this particular problem into a separate PR? A small patch has a great chance to be reviewed and approved quickly. |
Ooh, sorry. I've pushed the wrong button and closed the PR. Please accept my apologizes. |
92f533f
to
51bb1d2
Compare
Thanks, @cdce8p |
Rebased the PR. Also moved the |
# https://setuptools.readthedocs.io/en/latest/setuptools.html#setting-the-zip-safe-flag | ||
zip_safe = False | ||
include_package_data = True | ||
|
||
install_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.
I would have thought the dependencies would go in the same file as the rest of the metadata..?
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.
Yes they can be moved as well. Just the current tooling expects them to be in setup.cfg
. Since this PR stalled for so long I figured I'd revert that part for now. See my last commit. ec90225
I'd be happy to re-include it here or do it in a followup if there is interest.
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.
Right. I'll defer to @webknjaz here to decide how it should go.
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.
It should move to pyproject.toml
. The tooling should be updated.
build-backend = "setuptools.build_meta" | ||
|
||
[project] |
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.
Does dependabot understand this format?
I know that dependabot for poetry still requires the old [tool.poetry]
section instead of the recommended [project]
, I wonder if the setuptools-based check is different.
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.
Does dependabot understand this format?
Yes, that's a poetry only bug in dependabot.
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 don't think dependabot updates abstract deps anyway. Renovate probably would. Though, in general, this shouldn't be updated automatically. It's not a lock file and mustn't pin things or contain top boundaries in general. This is something that pip-tools constraints would solve, as they'd lock envs w/o the dangers of causing problems in dependency resolution for the end users.
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.
As mentioned earlier, I reverted the part that moved the dependencies to only focus on the static metadata here.
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 didn't notice that you marked those as dynamic.
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.
But still, there's no need to separate those anyway. So let's get them back into this config.
Not sure if this would work well with |
version = {attr = "aiohttp.__version__"} | ||
|
||
[tool.setuptools.packages.find] | ||
include = ["aiohttp*"] |
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 will probably happen automatically.
aiohttp | ||
aiohttp._websocket | ||
# https://setuptools.readthedocs.io/en/latest/setuptools.html#setting-the-zip-safe-flag | ||
zip_safe = False |
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 is no longer necessary?
pyproject.toml
Outdated
[tool.setuptools.package-data] | ||
"*" = ["*.so"] | ||
|
||
[tool.setuptools.exclude-package-data] | ||
"*" = ["*.c", "*.h"] |
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'm not entirely sure if these are still needed. Might need some manual checks.
CHANGES/9951.packaging.rst
Outdated
@@ -0,0 +1 @@ | |||
Moved static metadata from ``setup.cfg`` to ``pyproject.toml`` -- by :user:`cdce8p`. |
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.
Moved static metadata from ``setup.cfg`` to ``pyproject.toml`` -- by :user:`cdce8p`. | |
Moved core packaging metadata from :file:`setup.cfg` to :file:`pyproject.toml` per :pep:`621` | |
-- by :user:`cdce8p`. |
(it's not really just static)
pyproject.toml
Outdated
requires = [ | ||
"setuptools >= 46.4.0", | ||
] | ||
requires = ["setuptools >= 70.0.0"] |
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 specific feature that requires at least v70?
What I meant back then was that I'd like the move to first happen in projects that are direct deps of aiohttp (like yarl). This is because I've implemented some advanced packaging techniques there (like an in-tree build backend). I'm going to eventually migrate aiohttp to use the same model. So packaging updates would flow from those places where improvements were first implemented. Upon reflection, thought, I think that this PR doesn't block that so you're fine. Talking about the downstreams, they've been historically behind some upstream packaging standards, but AFAIK they've mostly implemented their own PEP 517 helpers by now. Still, there might be problems where the setuptools version they ship is older than our spec (it'd conflict, forcing them to patch it out on their side). Also, if they patch it out, there's a possibility that it's older than PEP 621 effectively not producing most of the metadata. So we should at least be careful about https://github.com/aio-libs/aiohttp/pull/9951/files#r2066217591. |
That's for the review @webknjaz! I'll address all comments in one to keep it readable. Please let me know if you have any followup questions.
Sounds good. I'd just recommend to do that in a followup PR. Otherwise the backports PRs will likely not apply cleanly. In the end it's just a revert of ec90225 With
Unfortunately not. Setuptools will throw an error
Yes. From the setuptools docs
Edit: Pushed one more commit to do directly: f06239c. Mainly just to ignore the generated
Double checked that. It's actually https://setuptools.pypa.io/en/latest/history.html#v66-1-0
Yes, I've seen a few reports in the light of the PEP 639 adoption. Support for it was only added in |
Pushed a new commit to incorporate the license metadata changes from #11226. @webknjaz Would you mind taking a look at this again? I believe I've addressed all your comments in #9951 (comment). Please let me know if you have any more questions. |
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'd rather not make unrelated changes. The change note talks about moving the metadata definitions, but the patch has some random changes without any justification. Let's not do this.
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.
Could you specify which parts you're referring to? AFICT all changes are related / a result of the metadata move.
E.g. tool.black.target-version
can be removed now that project.requires-python
is specified.
long_description_content_type = text/x-rst | ||
maintainer = aiohttp team <[email protected]> | ||
maintainer_email = [email protected] | ||
license = Apache-2.0 AND MIT |
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 SPDX identifier got lost somewhere.
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.
It's included as part of project.license.text
.
] | ||
build-backend = "setuptools.build_meta" | ||
|
||
[project] | ||
name = "aiohttp" | ||
license = {text = "Apache-2.0 AND MIT"} |
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 is incorrect. SPDX must not be added as license text: https://packaging.python.org/en/latest/guides/writing-pyproject-toml/#license
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 is only supported in setuptools v77
which most (?) distributors don't support just yet. That's why I've used license.text
which is map to the same metadata as the old license
field.
See also my longer comment about it: #9951 (comment)
All other threads need to be addressed as well. There's a number of those with zero response. |
I've addressed them in #9951 (comment) as I personally find the Github code comments hard to follow when it's a discussion about one or more related topics. |
What do these changes do?
The metadata changes:
Open questions