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

Move to metadata to pyproject.toml #166

Merged
merged 8 commits into from
Dec 1, 2022
Merged

Move to metadata to pyproject.toml #166

merged 8 commits into from
Dec 1, 2022

Conversation

di
Copy link
Member

@di di commented Mar 24, 2022

Now that PEP 621 support has been merged in pypa/setuptools#2970, we can move project metadata into pyproject.toml and get rid of setup.py and setup.cfg.

The one 'regression' is that external data files are deprecated (https://setuptools.pypa.io/en/latest/userguide/datafiles.html) so support has been removed here.

This should remain a draft PR until pypa/setuptools#2816 is resolved and editable installs work.

Closes #56, closes #57, closes #96.

Copy link

@abravalheri abravalheri left a comment

Choose a reason for hiding this comment

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

Thank you very much for working on this @di.

From my perspective it looks very good (I just added very minor comments).
I also validated the file with validate-pyproject and everything is in shape!

It would be nice though, if possible, to wait at least until the feature stabilises a bit. I think I manage to cover most of unexpected issues in 61.1.1 (there are some very edge-cases left) but we never know... Maybe some users will find other bugs in the following weeks.

pyproject.toml Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated
"Source" = "https://github.com/pypa/sampleproject/"

# To provide executable scripts, use entry points in preference to the
# "scripts" keyword. Entry points provide cross-platform support and allow

Choose a reason for hiding this comment

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

To avoid confusion with project.scripts, setuptools uses tool.setuptools.script-files for the legacy kind of script (which is explicitly deprecated in the docs now).

Maybe we can just skip this advice for the sake of avoiding confusion?
PEP 621 splits general entry-points, console_scripts and gui_scripts in 3 separated fields. There is a chance the wording entry point might be confusing when referring to scripts for new pacakgers.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this whole comment needs either re-wording or removing.

We shouldn't be mentioning the setuptools-specific "scripts" keyword at all here.

pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
@abravalheri
Copy link

Regarding data files, currently there is a proposal pypa/setuptools#3191 for a less problematic and more restricted way of presenting this feature to the users. If you have any thoughts, please feel free to provide some feedback on the discussion, it would be very nice to know what are your impressions.

@di
Copy link
Member Author

di commented Mar 28, 2022

Regarding data files, currently there is a proposal pypa/setuptools#3191 for a less problematic and more restricted way of presenting this feature to the users. If you have any thoughts, please feel free to provide some feedback on the discussion, it would be very nice to know what are your impressions.

I think I'm OK with removing it entirely from this sample regardless. I feel like overall this is an anti-pattern that we probably don't want to be encouraging for the average use case.

@di
Copy link
Member Author

di commented May 31, 2022

The build is failing here due to tox-dev/tox#2429.

Edit: resolved by using the tox4 pre-release.

@di
Copy link
Member Author

di commented Aug 29, 2022

pypa/setuptools#2816 is now resolved, so moving this out of draft status.

@di di marked this pull request as ready for review August 29, 2022 23:09
@mariocj89
Copy link

@di is there anything else blocking this PR? Will this only be landed after setuptools removes the "beta" banner or is it good to land already?

@di
Copy link
Member Author

di commented Oct 6, 2022

I think I was waiting to not have to require the tox prerelease here, but not sure when tox is planning to make that release.

Some reviews/approvals on this would be good as well.

@mariocj89
Copy link

@gaborbernat do you have an ETA for having a final release of tox 4?

@gaborbernat
Copy link

No promises. I'm currently blocked by health issues, so probably not this year (assuming no one else picks up development of tox 4).

@layday
Copy link
Member

layday commented Nov 26, 2022

Could you restore the build-system table here? setuptools falls back on the legacy backend without it.

pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml 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
@di
Copy link
Member Author

di commented Nov 28, 2022

Just to set expectations here: I'm not planning to revisit this PR until:

@layday
Copy link
Member

layday commented Nov 28, 2022 via email

@di
Copy link
Member Author

di commented Nov 28, 2022

PEP 518 explicitly says that "Tools should not require the existence of the [build-system] table".

Since this repo is intended to be an example/template for people completely unfamiliar with Python packaging (including the intricacies of PEP 517/518) in my opinion we should minimize the amount of unnecessary boilerplate if at all possible.

@pfmoore
Copy link
Member

pfmoore commented Nov 28, 2022

Given that we currently have a build-system section, removing it and then claiming that we need to block this until tox can handle the removal, seems a little unreasonable. Why not move this forward with build-system, and then follow up with a PR removing that section, if you really care that strongly?

I should also note that PEP 517 notes:

If the pyproject.toml file is absent, or the build-backend key is missing, the source tree is not using this specification, and tools should revert to the legacy behaviour of running setup.py (either directly, or by implicitly invoking the setuptools.build_meta:legacy backend).

In particular, this means that the legacy setuptools backend will be used in the absence of build-system. And I definitely don't think that's something we want to encourage people to do.

@di
Copy link
Member Author

di commented Nov 28, 2022

Fair points @pfmoore. I think all outstanding comments/suggestions have been resolved.

@layday
Copy link
Member

layday commented Nov 28, 2022

Well, I'll also note that the provision in PEP 518 as I understand it was added to ease the transition, not to elevate setuptools above other backends; the recommendation for new projects has been to include it. In addition, omitting the build system table subtly changes the behaviour of setuptools, placing the project root on the Python path.

@layday
Copy link
Member

layday commented Dec 1, 2022

Who can approve this PR?

Copy link
Member

@pfmoore pfmoore left a comment

Choose a reason for hiding this comment

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

A few comments related to making this PR be solely about moving metadata into pyproject.toml. Can the unrelated changes be moved to their own PR(s), please?

tox.ini Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
tox.ini Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
@pfmoore
Copy link
Member

pfmoore commented Dec 1, 2022

Does #181 help? It gets rid of the various deprecated features that are causing a lot of the debate here.

@pfmoore
Copy link
Member

pfmoore commented Dec 1, 2022

I've fixed up this PR after merging #181 into main. Hope I did it right!

@pfmoore
Copy link
Member

pfmoore commented Dec 1, 2022

It feels a bit off to merge based on my own approval, so @di I'll wait for your OK (or you can merge if you're happy).

@pfmoore
Copy link
Member

pfmoore commented Dec 1, 2022

Hmm, we may have conflicted here. @di I think your force-push blew away some changes I made :-(

@di
Copy link
Member Author

di commented Dec 1, 2022

Oops, sorry @pfmoore, I didn't see your changes here before I rebased on main!

@di
Copy link
Member Author

di commented Dec 1, 2022

@pfmoore I think I restored all your changes here, if it looks good to you, please merge!

@pfmoore
Copy link
Member

pfmoore commented Dec 1, 2022

No worries, I normally do PRs from my own fork, messing about with branches in the main repo is new to me so I didn't know the protocols :-)

@pfmoore
Copy link
Member

pfmoore commented Dec 1, 2022

Looks good to me. I'd dropped the tox.ini changes because they are unrelated to the move of the metadata, but I'm not going to make a big deal about it either way.

I'll merge this once checks are complete.

@pfmoore pfmoore merged commit aeeb50a into main Dec 1, 2022
@pfmoore pfmoore deleted the pep-621 branch December 1, 2022 18:49
@pfmoore
Copy link
Member

pfmoore commented Dec 1, 2022

lol looks like they completed while I was typing that!

@@ -1,4 +0,0 @@
[metadata]
Copy link
Member

Choose a reason for hiding this comment

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

FTR this broke the linkcheck in PyPUG

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.

Switch to setup.cfg for metadata?
7 participants