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

📝 Switch packaging metadata to use PEP 621 #690

Merged
merged 3 commits into from
Apr 23, 2024
Merged

Conversation

jaraco
Copy link
Member

@jaraco jaraco commented Apr 18, 2024

This change is Reviewable

@jaraco
Copy link
Member Author

jaraco commented Apr 18, 2024

Note, I'm not planning on re-writing the commit messages. Feel free to re-write them to satisfy the linter.

@webknjaz
Copy link
Member

webknjaz commented Apr 18, 2024

@jaraco the CI check is mostly indicative, which is why I didn't mark it as a blocker in the branch protection configuration. I'm thinking of how to improve it making it more useful. In case of obviously bad messages, I sometimes opt for using the squash "merge" but most of the time, using the regular natural merge is fine.

Anyway, this PR has a more serious problem — it breaks the CI fully and it aborts very early, failing to produce dists with expected names and version: https://github.com/cherrypy/cheroot/actions/runs/8744542826/job/23997691523?pr=690#step:15:41

I suspect this is because the pinned setuptools version in the lockfile is too old: https://github.com/cherrypy/cheroot/blob/3591a1c/requirements/dist-build-constraints.txt#L19C13-L19C19. Bumping it in this place will fix the CI if I'm right.

pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@webknjaz
Copy link
Member

Could you include some reasoning on PEP 621 vs. setup.cfg to address my concerns from #689 (comment)?

Also, the PR title suggests that it's about PEP 621 but it seems like this PR goes beyond that by also moving other things like [bdist_wheel] and other backend-specific configuration to pyproject.toml.
Do I understand correctly that PEP 621 only standardizes the packaging metadata bits but not the backend-specific ones? Is it possible to still keep something in a separate config? If so, would you have any objections?

pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@webknjaz
Copy link
Member

@jaraco I forgot to mention that I finally integrated Towncrier in Cheroot, with some extra categories compared to what you're used to: https://cheroot.cherrypy.dev/en/latest/contributing/guidelines/#alright-so-how-do-i-add-a-news-fragment.

We'll need to add a docs/changelog-fragments.d/690.packaging.rst to this pull request before merging. I haven't yet enabled my changelog enforcement bot that would block PRs when change notes are missing so checking its presence manually for now.

pyproject.toml Outdated Show resolved Hide resolved
@jaraco
Copy link
Member Author

jaraco commented Apr 19, 2024

Could you include some reasoning on PEP 621 vs. setup.cfg to address my concerns from #689 (comment)?

Also, the PR title suggests that it's about PEP 621 but it seems like this PR goes beyond that by also moving other things like [bdist_wheel] and other backend-specific configuration to pyproject.toml. Do I understand correctly that PEP 621 only standardizes the packaging metadata bits but not the backend-specific ones? Is it possible to still keep something in a separate config? If so, would you have any objections?

PEP 621 only defines the metadata bits, but Setuptools' implementation of it includes support for loading from pyproject.toml.

I'm not sure if it's possible to keep separate configs. It probably is. After 3e4ed4f, however, there aren't any settings left that are setuptools-specific.

I don't have any objections to keeping some config in setup.cfg. You're welcome to do that if/as needed.

@webknjaz
Copy link
Member

I don't have any objections to keeping some config in setup.cfg. You're welcome to do that if/as needed.

I thought I did have objections initially, but I think I don't actually have a strong case in support of that anymore. I'd keep other tooling separate where possible.

@webknjaz webknjaz self-assigned this Apr 19, 2024
@webknjaz
Copy link
Member

I'll try to address that final bit that blocks the CI...

@webknjaz
Copy link
Member

I just realize that there's another forgotten file that is still present in the repo — setup.py.

@webknjaz
Copy link
Member

Alright.. dist build job now works. But I'll postpone merging until I figure out the Sphinx problem that is unrelated.

webknjaz added a commit that referenced this pull request Apr 19, 2024
@webknjaz
Copy link
Member

We'll need to add a docs/changelog-fragments.d/690.packaging.rst to this pull request before merging.

I addressed this too.

webknjaz added a commit that referenced this pull request Apr 19, 2024
It's added to the `packaging` category as the downstreams are the
ones that would care about this happening most.
@webknjaz webknjaz changed the title Feature/pep 621 📝 Switch packaging metadata to use PEP 621 Apr 20, 2024
webknjaz added a commit that referenced this pull request Apr 20, 2024
It's added to the `packaging` category as the downstreams are the
ones that would care about this happening most.
webknjaz added a commit that referenced this pull request Apr 22, 2024
It's added to the `packaging` category as the downstreams are the
ones that would care about this happening most.
@webknjaz
Copy link
Member

Another error noticed in the CI:

intersphinx inventory 'https://cherrypy.rtfd.io/en/latest' not readable due to ValueError: unknown or unsupported inventory version: ValueError('invalid inventory header: ')
/home/runner/work/cheroot/cheroot/.tox/build-docs/bin/python: can't open file '/home/runner/work/cheroot/cheroot/docs/../setup.py': [Errno 2] No such file or directory
Extension error (jaraco.packaging.sphinx):
Handler <function load_config_from_setup at 0x7f355768e3b0> for event 'builder-inited' threw an exception (exception: Command '['/home/runner/work/cheroot/cheroot/.tox/build-docs/bin/python', '/home/runner/work/cheroot/cheroot/docs/../setup.py', '--name', '--version', '--url', '--author']' returned non-zero exit status 2.)

Perhaps one of the non-bumped extensions (jaraco.packaging.sphinx?) calls setup.py.

@jaraco
Copy link
Member Author

jaraco commented Apr 22, 2024

Another error noticed in the CI:

intersphinx inventory 'https://cherrypy.rtfd.io/en/latest' not readable due to ValueError: unknown or unsupported inventory version: ValueError('invalid inventory header: ')
/home/runner/work/cheroot/cheroot/.tox/build-docs/bin/python: can't open file '/home/runner/work/cheroot/cheroot/docs/../setup.py': [Errno 2] No such file or directory
Extension error (jaraco.packaging.sphinx):
Handler <function load_config_from_setup at 0x7f355768e3b0> for event 'builder-inited' threw an exception (exception: Command '['/home/runner/work/cheroot/cheroot/.tox/build-docs/bin/python', '/home/runner/work/cheroot/cheroot/docs/../setup.py', '--name', '--version', '--url', '--author']' returned non-zero exit status 2.)

Perhaps one of the non-bumped extensions (jaraco.packaging.sphinx?) calls setup.py.

Seems likely. jaraco.packaging.sphinx 9 added this change.

@webknjaz
Copy link
Member

Seems likely. jaraco.packaging.sphinx 9 added this change.

Yeah, I bumped it locally a few hours ago but then stepped away from the computer and only committing it now.

@webknjaz
Copy link
Member

Meanwhile, I'll focus on remaking the sphinxcontrib.spelling integration.

webknjaz added a commit that referenced this pull request Apr 22, 2024
It's added to the `packaging` category as the downstreams are the
ones that would care about this happening most.
Copy link

codecov bot commented Apr 22, 2024

Codecov Report

Merging #690 (0b75287) into main (dc9f895) will increase coverage by 5.14%.
Report is 20 commits behind head on main.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #690      +/-   ##
==========================================
+ Coverage   78.52%   83.66%   +5.14%     
==========================================
  Files          27       28       +1     
  Lines        4111     4169      +58     
==========================================
+ Hits         3228     3488     +260     
+ Misses        883      681     -202     

It's added to the `packaging` category as the downstreams are the
ones that would care about this happening most.
@webknjaz
Copy link
Member

Meanwhile, I'll focus on remaking the sphinxcontrib.spelling integration.

That was caused by a Sphinx upgrade that happened because I didn't make a lockfile for that env. So I made it now with a lower Sphinx version to make the CI happy, but will patch the local extension with the necessary later which will allow upgrading.

jaraco and others added 2 commits April 23, 2024 03:10
This patch migrated most related content to `pyproject.toml`
semi-automatically. The change was produced by using
`jaraco.develop.migrate-config` and `ini2toml`.

As a part of this change previously used for packaging configuration
`setup.cfg` and `setup.py` files have been removed.
`setup.py` hasn't been needed for quite a while but kept for backward
compatibility. We don't care about that anymore.

Co-authored-by: Sviatoslav Sydorenko <[email protected]>
This is apparently necessary since the older versions of
`jaraco.packaging.sphinx` call `setup.py` directly which is
deprecated and is being removed.
@webknjaz webknjaz enabled auto-merge April 23, 2024 01:15
@webknjaz webknjaz merged commit 808ce78 into main Apr 23, 2024
73 of 74 checks passed
@webknjaz webknjaz deleted the feature/pep-621 branch April 23, 2024 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants