Skip to content

Apply PEP-621#2145

Closed
Kludex wants to merge 14 commits intomasterfrom
chore/apply-pep-621
Closed

Apply PEP-621#2145
Kludex wants to merge 14 commits intomasterfrom
chore/apply-pep-621

Conversation

@Kludex
Copy link
Copy Markdown
Contributor

@Kludex Kludex commented Mar 25, 2022 •
edited
Loading

This was brought on Kludex/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

florimondmanca
florimondmanca reviewed Mar 25, 2022
View reviewed changes
@Kludex
Copy link
Copy Markdown
Contributor Author

Kludex commented Mar 26, 2022

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

@johtso
Copy link
Copy Markdown
Contributor

johtso commented Mar 26, 2022 •
edited
Loading

Discussion on dropping 3.6 support #2136

@florimondmanca
Copy link
Copy Markdown
Contributor

florimondmanca commented Mar 30, 2022 •
edited
Loading

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
Copy Markdown
Contributor 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
Copy Markdown
Contributor

florimondmanca commented May 30, 2022

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

florimondmanca
florimondmanca reviewed May 30, 2022
View reviewed changes
Kludex and others added 6 commits July 1, 2022 06:25
@Kludex @florimondmanca
Update pyproject.toml
53cf0b2 
Co-authored-by: Florimond Manca <florimond.manca@protonmail.com>
@Kludex @florimondmanca
Update scripts/build
e7995cf 
Co-authored-by: Florimond Manca <florimond.manca@protonmail.com>
@Kludex @florimondmanca
Update requirements.txt
d01587a 
Co-authored-by: Florimond Manca <florimond.manca@protonmail.com>
@Kludex
Add setup.py for editable installs
345ed72
@Kludex
rebase
5bd0468
@Kludex
Update according to new setup.py
03e75ec
@Kludex
Copy link
Copy Markdown
Contributor 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.

florimondmanca
florimondmanca approved these changes Jul 14, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@florimondmanca florimondmanca left a comment •
edited
Loading

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
Copy Markdown
Contributor 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
Copy Markdown
Contributor Author

Kludex commented Jul 15, 2022

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

jhominal
jhominal reviewed Jul 15, 2022
View reviewed changes
pyproject.toml
@@ -0,0 +1,61 @@
[build-system]
requires = ["setuptools", "setuptools-scm"]
Copy link
Copy Markdown

@jhominal jhominal Jul 15, 2022

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
Copy Markdown
Contributor

@adriangb adriangb Aug 13, 2022

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
Merged
@Kludex
Copy link
Copy Markdown
Contributor 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

Reviewers

@lovelydinosaur lovelydinosaur Awaiting requested review from lovelydinosaur

3 more reviewers

@jhominal jhominal jhominal left review comments

@adriangb adriangb adriangb left review comments

@florimondmanca florimondmanca florimondmanca approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Kludex @johtso @florimondmanca @jhominal @adriangb @guyskk