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

feat: move to flit backend #501

Merged
merged 10 commits into from
Mar 10, 2023
Merged

Conversation

henryiii
Copy link
Contributor

@henryiii henryiii commented Jan 27, 2023

This is an updated version and closes #436. Besides rebasing, this moves from using an adaptor of flake8 to using Ruff. Adaptors for pyproject.toml for flake8 are constantly (intentionally?) broken by changes in flake8. Ruff is Rust reimplementation that is 10-100x faster, has no dependencies, can do the work of multiple tools (and versions can't get out of sync, since it is one compiled package - that's why #499 is failing, flake8-bugbear updated), supports auto-fixing, and is natively configured via pyproject.toml. Build & cibuildwheel have already moved to Ruff.

  • Switched build backend to flit (original rebased and squashed commit from Switched build backend to flit #436)
  • chore: move to using Ruff
  • fix: include everything but setup.py in standards-based SDist builds

This solves the chicken-and-egg problem with setuptools and wheel when bootstrapping. Build and packaging have already moved to flit-core.

@codecov
Copy link

codecov bot commented Jan 27, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (182efc0) 68.35% compared to head (ce34fad) 68.35%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #501   +/-   ##
=======================================
  Coverage   68.35%   68.35%           
=======================================
  Files          12       12           
  Lines         929      929           
=======================================
  Hits          635      635           
  Misses        294      294           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@henryiii henryiii force-pushed the henryiii/feat/flit-backend branch 6 times, most recently from 3772ab3 to 7edc0c3 Compare January 27, 2023 21:20
@agronholm
Copy link
Contributor

Before you go any further, I would probably be only interested in switching to flit in v1.0.0 (the publicapi branch). I don't want to risk any compatibility issues in the v0.X series.

@henryiii
Copy link
Contributor Author

henryiii commented Jan 27, 2023

I would probably be only interested

IMO, it would be easier to separate changes (this, then publicapi in a subsequent step). Also the wheel of wheel is unchanged; this only affects packaging distributors who want to build from sources, or a pure SDist build. It should not be a breaking change to anyone else, because it's not a user-facing change. Doing everything in one massive "1.0" release will likely make it unclear where breakages come from.

We've moved over packaging and build without any major issues, so I think this is fairly safe, and finally breaks the wheel - setuptools dependency cycle. Up to you, though.

Also, I'm following your lead of adding the backward-compatibility "setup.py" in the SDist. I don't think it's very useful, someone can use installer or bootstrap flit-core, but it's still there.

@henryiii henryiii marked this pull request as ready for review January 27, 2023 22:26
@agronholm
Copy link
Contributor

this only affects packaging distributors who want to build from sources, or a pure SDist build.

And this precisely has been the source of trouble every time I try making a "radical" change to wheel.

@henryiii
Copy link
Contributor Author

henryiii commented Jan 30, 2023

Yes, because you tried to move a year or two ago before anyone else had moved. Now packaging and build have moved, so I think most of the early-adopter pains have been dealt with - most distros should know how to package an non-setuptools package now. I can't promise it will be perfectly smooth, but it should be much easier than before. And one step at a time will be easier too - it's easier to deal with packagers and users separately - making a change that breaks both of them at the same time isn't ideal. ;)

FYI, running isort via pre-commit was broken two days ago by Poetry-core 1.5.0 (and they are the ones pushing for major version pinning, 🤣) - this branch works, but the main branch has a broken pre-commit due to isort. (noticed working on #422). Updating isort fixes it.

@agronholm
Copy link
Contributor

Yes, because you tried to move a year or two ago before anyone else had moved. Now packaging and build have moved, so I think most of the early-adopter pains have been dealt with - most distros should know how to package an non-setuptools package now. I can't promise it will be perfectly smooth, but it should be much easier than before. And one step at a time will be easier too - it's easier to deal with packagers and users separately - making a change that breaks both of them at the same time isn't ideal. ;)

Neither packaging or build are used (vendored versions don't count) by the people who were previously affected by this. The ones immediately affected were people doing pip install --no-binary=:all:. I do indeed hope that things have improved since my last attempt. I will at least give people a heads-up at discuss.python.org a week before the next release that includes this PR.

FYI, running isort via pre-commit was broken two days ago by Poetry-core 1.5.0 (and they are the ones pushing for major version pinning, rofl) - this branch works, but the main branch has a broken pre-commit due to isort. (noticed working on #422). Updating isort fixes it.

I'm curious: what does this have to do with major version pinning?

@henryiii
Copy link
Contributor Author

This should help with pip install --no-binary=:all:, as you no longer have a circular dependency with setuptools. This will affect users who are not using isolated builds, which is heavily packaging distributors. Those distributors are (hopefully) shipping at least those packages, and probably more, so have learned how to ship a package with a non-setuptools backend by now.

Poetry-core made a breaking change in a minor release that broke at least one major package (isort - which happens to be used via pre-commit, which builds from SDists unless you make a mirror). Poetry strongly insists on following SerVer and major version pinning (like poetry-core<2), while a lot of people (like me, I also link to others) really, really don't like. And they claim it protects you from exactly what they just did. :)

@agronholm
Copy link
Contributor

Poetry-core made a breaking change in a minor release that broke at least one major package (isort - which happens to be used via pre-commit, which builds from SDists unless you make a mirror). Poetry strongly insists on following SerVer and major version pinning (like poetry-core<2), while a lot of people (like me, I also link to others) really, really don't like. And they claim it protects you from exactly what they just did. :)

That depends on whether or not they knew that the change was breaking. SemVer does give consumers some degree of protection from breaking changes, but that doesn't automatically protect from developer mistakes. I take it that they reverted the breaking change soon after? That is at least how SemVer is supposed to work.

@agronholm
Copy link
Contributor

I went and fixed the pre-commit failure on main, and that caused the conflict you're seeing here. Sorry about that.
I'll give this PR a review either today or tomorrow.

@henryiii
Copy link
Contributor Author

That's fine, I don't mind rebasing.

I take it that they reverted the breaking change

Nope, they considered it isort's problem.

Copy link
Contributor

@agronholm agronholm left a comment

Choose a reason for hiding this comment

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

Some questions, remarks and change requests.

.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
tests/test_sdist.py Show resolved Hide resolved
.github/workflows/publish.yml Show resolved Hide resolved
.pre-commit-config.yaml Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
@henryiii
Copy link
Contributor Author

henryiii commented Jan 31, 2023

I resolved the things I addressed with code changes. If I just respond, I didn't resolve. Tox's pyproject.toml support is... interesting. But I guess it works. :)

@agronholm
Copy link
Contributor

Tox was supposed to get proper pyproject.toml support in v4.0 but IIRC it was deferred to a later release.

vishwin added a commit to vishwin/freebsd-ports that referenced this pull request Feb 5, 2023
Per PEP-518 [0], for projects using setuptools as the build backend,
both setuptools and wheel are to be specified, as wheel is a plugin
for setuptools to support the format at all. Additionally, wheel
itself will soon [1] not depend on setuptools, but rather the
dependency tree will reverse.

With hat:	python
Approved by:	mentors (implicit), swills (maintainer, implicit)
References:	https://peps.python.org/pep-0518/#build-system-table [0]
		pypa/wheel#501 [1]
@vishwin
Copy link

vishwin commented Feb 24, 2023

Is test_sdist the only remaining blocker?

pyproject.toml Outdated Show resolved Hide resolved
agronholm and others added 3 commits March 10, 2023 11:48
Fixed flit_scm installation

Fixed FreeBSD test suite

Switched build backend to flit_core

Removed configuration required previously by flit-scm
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
henryiii and others added 4 commits March 10, 2023 11:49
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
@agronholm agronholm merged commit 0de0c1a into pypa:main Mar 10, 2023
@agronholm
Copy link
Contributor

Thanks!

@SpecLad
Copy link

SpecLad commented Mar 10, 2023

Shouldn't there be a runtime dependency on setuptools? The wheel convert command doesn't work without it:

> .\venv\Scripts\wheel.exe convert
Traceback (most recent call last):
  File "<frozen runpy>", line 198, in _run_module_as_main
  File "<frozen runpy>", line 88, in _run_code
  File ".\venv\Scripts\wheel.exe\__main__.py", line 7, in <module>
  File ".\venv\Lib\site-packages\wheel\cli\__init__.py", line 91, in main
    args.func(args)
  File ".\venv\Lib\site-packages\wheel\cli\__init__.py", line 29, in convert_f
    from .convert import convert
  File ".\venv\Lib\site-packages\wheel\cli\convert.py", line 10, in <module>
    from ..bdist_wheel import bdist_wheel
  File ".\venv\Lib\site-packages\wheel\bdist_wheel.py", line 24, in <module>
    import pkg_resources
ModuleNotFoundError: No module named 'pkg_resources'

@agronholm
Copy link
Contributor

I think you should open a separate issue for that. Ideally, nothing else than bdist_wheel should depend on setuptools.

@henryiii
Copy link
Contributor Author

Would this be fixed by #508, I wonder?

@henryiii henryiii deleted the henryiii/feat/flit-backend branch March 10, 2023 21:03
@agronholm
Copy link
Contributor

No, that still imports setuptools.

@SpecLad
Copy link

SpecLad commented Mar 10, 2023

Opened #510.

@henryiii
Copy link
Contributor Author

henryiii commented Mar 10, 2023

Ahh, it’s via importing bdist_wheel.

@nanonyme
Copy link

nanonyme commented Mar 11, 2023

To be fair, it might have been more sustainable to split this to wheel-core which does not runtime depend on setuptools and wheel that does. They can live in same repo. The core version would provide API for setuptools to build itself.

@agronholm
Copy link
Contributor

The long term plan is to move bdist_wheel to setuptools, and refactor the wheel convert command to work independently of setuptools.

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.

5 participants