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

Try replacing setup.* with pyproject.toml #144

Merged
merged 21 commits into from
Dec 16, 2023

Conversation

Mr0grog
Copy link
Member

@Mr0grog Mr0grog commented Dec 1, 2023

Direct usage of setup.py (that is calling python setup.py some_command to do builds and such) has been deprecated and no longer supported for quote a while, and I’ve been meaning to try to move everything to pyproject.toml (the new format for declaring package metadata). It seems like pyproject.toml supports all the nice things now, so this is my experimental attempt to switch to it and drop setup.py and setup.cfg entirely.

(This seems like a good time to do this, since there are some other semi-breaking changes on tap, like rethinking how rate limit errors are handled.)

This doesn’t quite work yet — the main sticking point is Versioneer. When pip or build try and make an isolated environment, it’s missing the runtime requirements for the package and can’t import the version from the code.

This might be fixable by:

  • Making a separate _build_version.py file that pulls in the version info the same as __init__.py does but without requiring any other dependencies.
  • Bringing back a minimal version of setup.py just to run Versioneer. (But it seems like this shouldn’t be necessary since Versioneer 0.29 claims support for pyproject.toml-only build systems: https://github.com/python-versioneer/python-versioneer/releases/tag/0.29)
  • Not doing isolated builds? Same as today, I guess, but disappointing.

Need to experiment here.

Obviously this will need CI changes and maybe Dependabot config, too.

Also Needs testing to make sure builds based on this are still installable and workable on Python 3.6 (assuming a recent version of pip is being used to install).

@danielballan
Copy link
Contributor

I was just thinking of dipping a toe back in here, starting with this. May I toss some commits here on the versioneer aspect?

@Mr0grog
Copy link
Member Author

Mr0grog commented Dec 1, 2023

Dan!! Absolutely, please do add some commits

@Mr0grog
Copy link
Member Author

Mr0grog commented Dec 1, 2023

I was just reading through the PRs that went into v0.29 of Versioneer and python-versioneer/python-versioneer#371 makes it sound like maybe pyproject.toml-only is only supported for the pdm backend, and not setuptools, but 🤷

@danielballan
Copy link
Contributor

Hello! It's been too long!

If I recall correctly, we based this repo on a template and set of recommendations that I used to work from for most of my projects. In May, I got together with Henry Schreiner for week, and we produced updated recommendations. We cover four build backends, including setuptools, but I've been moving my projects over to hatchling and the hatch-vcs extension in place of versioneer.

Its builds are isolated. I've been happy with the git integration. In addition to versioning, it uses .gitignore as a base determination of what files should be including in the package, supplanting MANFIEST.in.

If that sounds reasonable I'll push some commits to try that on here.

@Mr0grog
Copy link
Member Author

Mr0grog commented Dec 1, 2023

@danielballan that sounds pretty reasonable to me! (I think last time I was playing with newer Python tools, I liked PDM more than Hatch, but that was mainly because I didn’t think Hatch managed venvs, and it looks like that’s either changed or I understood it wrong back then. I also liked that it did PEP 582, but that got killed and I am sad.)

One question about Python versions, though:

We currently support Python 3.6, and I think Hatch/Hatchling are 3.7+ (I guess the latest Hatchling looks like just 3.8+). I think the build-system deps are only needed during builds, so this isn’t a big deal (since I have been building from a newer version), but will this be a problem if a Python 3.6 user winds up installing from an sdist instead of a wheel?

IIRC we support 3.6 here because Google Colab took a long time to update, but I just checked and it looks like Colab is on Python 3.10 now. So it’s probably OK to change our minimum Python version. Just want to flag it as a breaking change to avoid if it’s not too hard.

@danielballan
Copy link
Contributor

Good note. Python 3.6 reached end-of-life (no more security patches) in 2021. Python 3.7 reached end-of-life in June 2023. I've been in the habit of dropping Python versions from the test matrix once they are end-of-lifed. Naturally, users stuck on all versions can resort to using older releases of wayback.

Would that policy create a problem for known users?

@Mr0grog
Copy link
Member Author

Mr0grog commented Dec 1, 2023

Would that policy create a problem for known users?

Nah, I think that’s ok here. The only holdup for known users (mainly @edsu and @ericnost) was Google Colab, and that’s no longer an issue.

@danielballan
Copy link
Contributor

In the ~5 years it's been since I showed my face here, my commit bit went away (which is very reasonable). I started a PR into this branch over on my fork: #145

@Mr0grog
Copy link
Member Author

Mr0grog commented Dec 1, 2023

Do you want it back? I’m happy to add you as a committer/maintainer/admin (depending on how responsible you want to be 😉).

@danielballan
Copy link
Contributor

Thanks. Let’s give it a couple months and see if I manage to stay active. 🙂

@Mr0grog Mr0grog force-pushed the can-we-drop-setuppy-lets-find-out branch from 30e4243 to e891218 Compare December 16, 2023 20:59
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated
Comment on lines 19 to 24
# FIXME: how does this show up in PyPI? In setup.py we had:
# license="BSD (3-clause)"
# But that's not the right format here. And there's no trove classifier for it.
# Maybe this should be:
# license = {text = "BSD (3-clause)"}
license = {file = "LICENSE"}
Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this should probably change. Here’s how the the file reference shows up on test.pypi.org:

Screenshot from PyPI showing several lines of the full license text

vs. what we currently have with setup.py:

Screenshot from PyPI showing just the name of the license

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed this and uploaded to test.pypi.org; it fixes the issue!

@Mr0grog Mr0grog marked this pull request as ready for review December 16, 2023 21:18
@Mr0grog
Copy link
Member Author

Mr0grog commented Dec 16, 2023

Yowza, this dropped 3,025 lines of code! Granted that was mostly Versioneer, but nice to see it go. 👋

@Mr0grog Mr0grog merged commit 790ad74 into main Dec 16, 2023
@Mr0grog Mr0grog deleted the can-we-drop-setuppy-lets-find-out branch December 16, 2023 21:30
node: $Format:%H$
node-date: $Format:%cI$
describe-name: $Format:%(describe:tags=true,match=*[0-9]*)$
ref-names: $Format:%D$

Choose a reason for hiding this comment

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

@Mr0grog FYI it was later determined that having ref-names is problematic, and it's no longer recommended to include it into this template: https://setuptools-scm.rtfd.io/en/latest/usage/#git-archives / pypa/setuptools-scm#1033.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, that is interesting, thanks very much. I’ll pull it out of the main development branch. 🙇

That said, please post things like this as a new issue (or an existing open issue if applicable) rather than re-opening discussion on a completed PR in the future. It makes it a little easier to track things that need to get done (and attribution for release notes). 🙂

Copy link

@webknjaz webknjaz Jun 24, 2024

Choose a reason for hiding this comment

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

Yeah, it was more of a drive-by discovery, a thing that I noticed in passing so I put a comment where some context exists. I'm not a user of this project, but when I notice something in places I visit while doing git paleontology, and don't have time to spend on writing a long explanation, this seems like an acceptable middle ground :)

Mr0grog added a commit that referenced this pull request Jun 24, 2024
It turns out the way we populated the `ref-names` field causes its value to change depending on whether the commit a Git archive was built from was the head of a branch when it was built (which is often true when we first cut a release, but ceases to be true soon afterward. If someone downloads an archive *later* and tries to compare its signature with the one we released, it won’t match because this field has changed, and that’s a significant issue.

Unfortunately, there’s no good alternative that resolves this issue, so the best solution is to just remove the field. On the up-side, `describe-name` carries the more critical info about the the current tag or release version, so this isn’t a huge loss.

Thanks to @webknjaz for pointing this out: #144 (comment)
Mr0grog added a commit that referenced this pull request Jun 25, 2024
It turns out the way we populated the `ref-names` field causes its value to change depending on whether the commit a Git archive was built from was the head of a branch when it was built (which is often true when we first cut a release, but ceases to be true soon afterward. If someone downloads an archive *later* and tries to compare its signature with the one we released, it won’t match because this field has changed, and that’s a significant issue.

Unfortunately, there’s no good alternative that resolves this issue, so the best solution is to just remove the field. On the up-side, `describe-name` carries the more critical info about the the current tag or release version, so this isn’t a huge loss.

Thanks to @webknjaz for pointing this out: #144 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants