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

Restore compatability with older setuptools versions and cleanup specfile #296

Merged
merged 2 commits into from Mar 24, 2023

Conversation

gotmax23
Copy link
Contributor

Relates: #294

@gotmax23
Copy link
Contributor Author

/packit retest-failed

Copy link
Collaborator

@TomasTomecek TomasTomecek left a comment

Choose a reason for hiding this comment

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

Sweet! you're on fire, thank you for addressing the compatibility.

Please rebase, I just merged your other PR.

I will leave this open for at least a week so everyone has enough time to comment.

@gotmax23
Copy link
Contributor Author

I've rebased. I am also splitting out the RPM packaging changes to make this easier to review.

setup.cfg Outdated Show resolved Hide resolved
setup.cfg Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
@@ -61,5 +6,5 @@ ansible-bender = "ansible_bender.cli:main"
# setuptools: https://setuptools.pypa.io/en/latest/userguide/pyproject_config.html
# setuptools-scm: https://github.com/pypa/setuptools_scm/#pyprojecttoml-usage
[build-system]
requires = ["setuptools>=61", "setuptools-scm[toml]>=6.2"]
requires = ["setuptools", "setuptools-scm[toml]"]
Copy link
Member

Choose a reason for hiding this comment

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

It's usually a good idea to declare minimum versions that have the required features...
While pypa/build would normally install the latest compatible versions, somebody may want to go for reproducibile builds and pin the build dep versions via PIP_CONSTRAINT, for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd prefer not to arbitrarily pin versions. If there's a feature that the project uses that is known to require a certain minimum version of setuptools(_scm), let me know. We could pin setuptools to whenever build_meta was added, but I don't think that's common practice.

Copy link
Member

Choose a reason for hiding this comment

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

Pinning is a separate topic. It's not something you'd put in pyproject.toml, of course.
But the minimum versions are usually a good idea — it is a metadata bit that shows what build dep versions have the features used.

FWIW, I think that the [toml] extra was added in setuptools-scm v6 and the git archive plugin was absorbed in v7.
As for setuptools, v40.9.0 added the support for setup.py-less projects, though I only documented it in the upstream docs in v59.0.0.

So realistically, this is what's minimal:

Suggested change
requires = ["setuptools", "setuptools-scm[toml]"]
requires = [
"setuptools >= 40.9.0", # required for `setup.py`-less builds
"setuptools-scm[toml] >= 7", # supports .git_archival.txt for non-tagged commits
]

Copy link
Member

Choose a reason for hiding this comment

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

It could also be useful to have a smoke test in GHA with different PIP_CONSTRAINT setting the oldest supported build deps pins and something in the middle, as well as bleeding-edge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pinning is a separate topic. It's not something you'd put in pyproject.toml, of course.

I often say pinning when I mean setting a minimum version.

FWIW, I think that the [toml] extra was added in setuptools-scm v6 and the git archive plugin was absorbed in v7.

I prefer to maintain compatibility and not pin to >= 7.

As for setuptools, v40.9.0 added the support for setup.py-less projects, though I only documented it in the upstream docs in v59.0.0.

The setuptools docs don't recommend this, but I can add that.

It could also be useful to have a smoke test in GHA with different PIP_CONSTRAINT setting the oldest supported build deps pins and something in the middle, as well as bleeding-edge.

That can be separate. PRs should have a clear/limitted scope.

Copy link
Member

@webknjaz webknjaz Mar 17, 2023

Choose a reason for hiding this comment

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

I prefer to maintain compatibility and not pin to >= 7.

Then, specify a lower version. Note that prior to v7, the support for .git_archival.txt comes from a plugin.

Determine which old Python versions you want to work with setuptools-scm-git-archive and use env markers to make it a conditional dependency. And you'll need two conditional entries for setuptools-scm — the one for newer Pythons would require setuptools-scm v7+ and the old would have the same marker as the plugin and could be something like ~= 6.0 (earlier versions would not be able to pick up the config in pyproject.toml, I think). I'm almost sure you can't combine the plugin and the setuptools-scm version that incorporates the same features.

Copy link
Member

Choose a reason for hiding this comment

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

Pinning is a separate topic. It's not something you'd put in pyproject.toml, of course.

I often say pinning when I mean setting a minimum version.

FWIW, I think that the [toml] extra was added in setuptools-scm v6 and the git archive plugin was absorbed in v7.

I prefer to maintain compatibility and not pin to >= 7.

As for setuptools, v40.9.0 added the support for setup.py-less projects, though I only documented it in the upstream docs in v59.0.0.

The setuptools docs don't recommend this, but I can add that.

The docs are sometimes incomplete so we end up relying on the tribal knowledge. I sometimes add the docs or review the docs PRs myself.
The only reason setup.py used to be needed in the last ≈4 years, was for editable installs. But PEP 660 has been supported in pip and setuptools for a while now, making the legacy fallback completely unnecessary. This is the current standard that replaced the old mechanism that never had a standard.

Copy link
Member

Choose a reason for hiding this comment

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

That can be separate. PRs should have a clear/limitted scope.

Absolutely. The PRs should be atomic. I just wanted to include this information while on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer to maintain compatibility and not pin to >= 7.

Then, specify a lower version. Note that prior to v7, the support for .git_archival.txt comes from a plugin.

Determine which old Python versions you want to work with setuptools-scm-git-archive and use env markers to make it a conditional dependency. And you'll need two conditional entries for setuptools-scm — the one for newer Pythons would require setuptools-scm v7+ and the old would have the same marker as the plugin and could be something like ~= 6.0 (earlier versions would not be able to pick up the config in pyproject.toml, I think). I'm almost sure you can't combine the plugin and the setuptools-scm version that incorporates the same features.

I'm fine with .git_archival support not always being available. It doesn't work with Github's generated archives anyways (try pip install https://github.com/pypa/setuptools_scm/archive/main.tar.gz and see what happens). In the unlikely event that someone manually generates a tarball with git archive and pip installs it, they should get the newest setuptools_scm version thanks to build isolation. Linux distribution packagers or regular pip users use the the PyPI sdist that doesn't need this at all.

The only reason setup.py used to be needed in the last ≈4 years, was for editable installs.

or for "stable enterprise" linux distributions :). The way that setuptools now handles editable installs still isn't great (you need --config-settings editable_mode=compat to prevent it from using the runtime import hook approach that breaks static analysis tools), but I digress...

Copy link
Member

Choose a reason for hiding this comment

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

try pip install https://github.com/pypa/setuptools_scm/archive/main.tar.gz and see what happens

Interesting... That's definitely a bug that should be reported upstream. But, for the projects using it, it actually works. Try pip install install https://github.com/python-attrs/attrs/archive/main.tar.gz and you'll see Successfully installed attrs-22.2.1.dev39, for example.

Linux distribution packagers or regular pip users use the the PyPI sdist that doesn't need this at all.

Most. But I've also seen some building from SCM.

or for "stable enterprise" linux distributions :)

There's no reason they can't echo 'import setuptools; setuptools.setup()' > setup.py which is effectively what setuptools does in-memory, anyway.

gotmax23 and others added 2 commits March 15, 2023 09:26
This reverts commit 54252c8 and
restores compatibility with Fedora 36, EL 9, and other distributions
with older setuptools versions.

- Keep the setup.py out. The setuptools PEP 517 backend can read
  configuration from setup.cfg.
- Relax setuptools constraint. The newer version is only needed for PEP
  631 support.
- Relax setuptools_scm constraint. The upstream recommendation is
  arbitrary and older versions still work. EL 9 only has 6.0.1.
- Leave setuptools_scm configuration in pyproject.toml.
The resulting dist shouldn't have the py2 tag since it only declares
support for Python 3.6+.

Co-authored-by: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>
Copy link
Collaborator

@TomasTomecek TomasTomecek left a comment

Choose a reason for hiding this comment

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

Perfect, thank you @gotmax23 for your hard work and @webknjaz for the detailed review \o/.

@TomasTomecek TomasTomecek merged commit 664620e into ansible-community:master Mar 24, 2023
4 checks passed
@gotmax23
Copy link
Contributor Author

Sure! I also realized that this project still claims to support Python 3.6, while setuptools >= 61 only supports Python 3.7 and above, so this change also restores support for Python 3.6.

@webknjaz
Copy link
Member

while setuptools >= 61 only supports Python 3.7 and above, so this change also restores support for Python 3.6.

FWIW, ansible-blender publishes wheels that may be built with Python 3.11 but are still installable on lower Pythons. So this would only be an issue when building sdist specifically under said runtime...

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

3 participants