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

Apply PEP-621 #2145

Closed
wants to merge 14 commits into from
Closed

Apply PEP-621 #2145

wants to merge 14 commits into from

Conversation

Kludex
Copy link
Sponsor Member

@Kludex Kludex commented Mar 25, 2022

This was brought on encode/starlette#1359.

setuptools released 0.61.0 yesterday, and brought to light the PEP-621 which makes this PR possible. Most of the PR was inspired on this page: https://setuptools.pypa.io/en/latest/userguide/pyproject_config.html

Notes:

  • I'll continue later on. I need to check if everything is included on the metadata files.

How to test?

  • ./scripts/build to see that wheel and tar.gz are generated
  • python -m pip install -e . for editable installs

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

Kludex commented Mar 26, 2022

Oh... setuptools doesn't support Python 3.6 anymore.

@johtso
Copy link
Contributor

johtso commented Mar 26, 2022

Discussion on dropping 3.6 support #2136

@florimondmanca
Copy link
Member

florimondmanca commented Mar 30, 2022

What do we think about this bit at the top of the setuptools docs page…?

Note
New in 61.0.0 (experimental)

Warning
Support for declaring project metadata or configuring setuptools via pyproject.toml files is still experimental and might change (or be removed) in future releases.

Presumably the idea if we go with this, we'd switch all Encode projects over as well eventually. Is now a good time? Also, what does this get us compared to the current setup.py configuration?

@Kludex
Copy link
Sponsor Member Author

Kludex commented Apr 5, 2022

Is now a good time?

Maybe not. Let's wait for others to test this first 😅 👍

Also, what does this get us compared to the current setup.py configuration?

One less file (setup.py), and compliance with the mentioned PEP.

@florimondmanca
Copy link
Member

For another point of comparison, I played with pyproject.toml-based packaging as well and here's what I came up with: https://github.com/florimondmanca/asgi-htmx/blob/master/pyproject.toml

setup.py Show resolved Hide resolved
scripts/build Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
Kludex and others added 6 commits July 1, 2022 06:25
Co-authored-by: Florimond Manca <florimond.manca@protonmail.com>
Co-authored-by: Florimond Manca <florimond.manca@protonmail.com>
Co-authored-by: Florimond Manca <florimond.manca@protonmail.com>
@Kludex
Copy link
Sponsor Member Author

Kludex commented Jul 1, 2022

Thanks for the review @florimondmanca 🙇

I've updated the PR, sync with the changes that happened on setup.py, and I've added the setup.py back.

Copy link
Member

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally happy with this. Still happy to read other people's opinion.

My last question would be -- have we checked py.typed is present in the resulting sdist? (If not, releasing would break type hints.)

@Kludex
Copy link
Sponsor Member Author

Kludex commented Jul 15, 2022

It adds correctly the py.typed.

I've added the content-type on the readme and bumped twine, as I was having a warning.

@Kludex
Copy link
Sponsor Member Author

Kludex commented Jul 15, 2022

Let's wait for @tomchristie 's opinion here?

@@ -0,0 +1,61 @@
[build-system]
requires = ["setuptools", "setuptools-scm"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add a minimal version of setuptools as a build requirement to ensure that a compatible version of setuptools is installed.

Suggested change
requires = ["setuptools", "setuptools-scm"]
requires = ["setuptools>=61", "setuptools-scm"]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed

@Kludex Kludex mentioned this pull request Aug 9, 2022
@Kludex
Copy link
Sponsor Member Author

Kludex commented Aug 15, 2022

@Kludex Kludex closed this Aug 15, 2022
@Kludex Kludex deleted the chore/apply-pep-621 branch August 15, 2022 17:22
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.

None yet

6 participants