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

Set the minimum required setuptools to v46.4.0 #6205

Merged
merged 6 commits into from Nov 2, 2021

Conversation

jameshilliard
Copy link
Contributor

@jameshilliard jameshilliard commented Nov 2, 2021

In order to avoid a recursive dependency issue when parsing the aiohttp version during install setuptools needs:
pypa/setuptools@c457e68

This implies we need setuptools 46.4.0 or newer.

This should probably also be back-ported as this setuptools version requirement was introduced in 3.8.

What do these changes do?

Adds setuptools >= 46.4.0 requirement to pyproject.toml and readme.

Are there changes in behavior for the user?

Should ensure setuptools version is correct instead of cryptic build dependency errors.

Related issue number

N/A

Checklist

  • [N/A] I think the code is well written
  • [N/A] Unit tests for the changes exist
  • Documentation reflects the changes
  • [N/A] If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Nov 2, 2021
@jameshilliard jameshilliard force-pushed the setuptools-version branch 4 times, most recently from 63bf00e to 6a34b79 Compare November 2, 2021 07:37
@codecov
Copy link

codecov bot commented Nov 2, 2021

Codecov Report

Merging #6205 (2cd7d89) into master (7f0cd0d) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6205   +/-   ##
=======================================
  Coverage   93.29%   93.30%           
=======================================
  Files         103      103           
  Lines       30360    30360           
  Branches     2729     2729           
=======================================
+ Hits        28324    28326    +2     
+ Misses       1858     1857    -1     
+ Partials      178      177    -1     
Flag Coverage Δ
unit 93.22% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
tests/conftest.py 66.66% <0.00%> (-1.71%) ⬇️
tests/test_worker.py 22.44% <0.00%> (+2.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f0cd0d...2cd7d89. Read the comment docs.

Copy link
Member

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

I believe we should specify setuptools version in pyproject.toml, not setup.py.

Please add something like:

[build-system]
requires = [
    "setuptools>=46.4.0",
]
build-backend = "setuptools.build_meta"

@asvetlov
Copy link
Member

asvetlov commented Nov 2, 2021

@webknjaz please check the required minimal setuptools version.
I believe you are aware of setuptools features and changelog better than me.

@jameshilliard
Copy link
Contributor Author

I believe we should specify setuptools version in pyproject.toml, not setup.py.

Yeah, I wasn't sure if something like that worked properly, I thought checking there required a fairly recent setuptools version or something as well or something.

@jameshilliard
Copy link
Contributor Author

I believe you are aware of setuptools features and changelog better than me.

FYI for 3.8 it appears this is the change that bumped the setuptools requirement:
4e39c6b#diff-fa602a8a75dc9dcc92261bac5f533c2a85e34fcceaff63b3a3a81d9acde2fc52R3

@asvetlov
Copy link
Member

asvetlov commented Nov 2, 2021

From my understanding, a relatively modern pip (about 3 years old or even older) looks at pyproject.toml to select build tool version used for installation.
The check from setup.py is redundant.

Did I miss something?

@jameshilliard
Copy link
Contributor Author

From my understanding, a relatively modern pip (about 3 years old or even older) looks at pyproject.toml to select build tool version used for installation.

Would that work for installs that don't use pip?

pyproject.toml Outdated Show resolved Hide resolved
Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

TL;DR I don't see how this patch improves anything we specifically care about, it only seems to make the maintenance burden worse if anything. So I'm strongly opposed to adding the check but updates to pyproject.toml are fine.

PEP 517 build front-end uses pyproject.toml to populate the build env. When setuptools can be specified uncapped, in practice, the front-ends (like pip) will pull in the very latest version of it. So we are covered here.

Invoking setuptools directly is deprecated. Here's why https://blog.ganssle.io/articles/2021/10/setup-py-deprecated.html.
And also distutils has been deprecated for quite a while now so importing it is rather discouraged.

So here's my request:

  1. Drop the changes from setup.py
  2. Move the [build-system] to the top of pyproject.toml
  3. Update the change note to reflect this and use the past tense there (you may find a more comprehensive guide in CHANGES/README.rst

@webknjaz
Copy link
Member

webknjaz commented Nov 2, 2021

Would that work for installs that don't use pip?

The ecosystem is standardized around PEP 517 now. We won't support weird installs and it's up to the users to abuse this in unsupported ways. This standard brings a lot of convenience and even distro packagers follow it (for example, Fedora has their own PEP 517 helper macros, some of the others invoke pip).

There is no point for us to support non-standardized things.

@jameshilliard jameshilliard force-pushed the setuptools-version branch 2 times, most recently from 2e53373 to 96a590a Compare November 2, 2021 10:47
CHANGES/6205.bugfix Outdated Show resolved Hide resolved
@webknjaz
Copy link
Member

webknjaz commented Nov 2, 2021

@jameshilliard please fill out the PR template, it seems you've deleted it so here's where you can recover it from https://github.com/aio-libs/aiohttp/blob/master/.github/PULL_REQUEST_TEMPLATE.md.

@jameshilliard
Copy link
Contributor Author

Invoking setuptools directly is deprecated. Here's why https://blog.ganssle.io/articles/2021/10/setup-py-deprecated.html.
And also distutils has been deprecated for quite a while now so importing it is rather discouraged.

Hmm, I wonder what the recommended way for handling cross compilation without direct invocation would be.

This standard brings a lot of convenience and even distro packagers follow it (for example, Fedora has their own PEP 517 helper macros, some of the others invoke pip).

We're still using setup.py at the moment, I ran into this issue since we still in the process of fully removing python2 and were sharing the setuptools version:
https://github.com/buildroot/buildroot/blob/76c7fed85f7a822663cf0c8828e7d282198d0803/package/pkg-python.mk#L245-L249

There is no point for us to support non-standardized things.

Oh, I assumed supporting those was why there was an explicit python version check in setup.py, was there a different reason for doing it that way?

@jameshilliard jameshilliard changed the title Add check for compatible setuptools version. Add requirement for compatible setuptools version. Nov 2, 2021
In order to avoid a recursive dependency issue when parsing
the aiohttp version during install setuptools needs:
pypa/setuptools@c457e68

This implies we need setuptools 46.4.0 or newer.
@jameshilliard
Copy link
Contributor Author

please fill out the PR template, it seems you've deleted it so here's where you can recover it from

Ok, re-added, figured it was a bit verbose since most sections didn't seem applicable here for a trivial fix like this.

@webknjaz
Copy link
Member

webknjaz commented Nov 2, 2021

please fill out the PR template, it seems you've deleted it so here's where you can recover it from

Ok, re-added, figured it was a bit verbose since most sections didn't seem applicable here for a trivial fix like this.

When it's missing, we still need to ask clarification questions which is basically extracting the same info from you but slower.

@jameshilliard
Copy link
Contributor Author

I guess it depends on what you need. There's also an option to have an in-tree PEP 517 build backend, for example.

Yeah, it's def not an in-tree backend that we need.

We're basically just cross compiling python packages from source and installing them into a target rootfs, we've def had to carry a good number of downstream patches/hacks to the python infrastructure to make this work though.

We use existing buildroot infrastructure for downloading and dependency resolution(needed for system package dependency resolution that python packages often depend on) so pip I think kinda gets in the way there.

@asvetlov asvetlov self-requested a review November 2, 2021 11:31
@asvetlov asvetlov added backport-3.8 backport-3.9 Trigger automatic backporting to the 3.9 release branch by Patchback robot labels Nov 2, 2021
@asvetlov
Copy link
Member

asvetlov commented Nov 2, 2021

@webknjaz I agree with the current PR changes.

Please feel free to merge when PR is ready from your perspective.

CHANGES/6205.bugfix Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
Co-authored-by: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>
@webknjaz
Copy link
Member

webknjaz commented Nov 2, 2021

I guess it depends on what you need. There's also an option to have an in-tree PEP 517 build backend, for example.

Yeah, it's def not an in-tree backend that we need.

We're basically just cross compiling python packages from source and installing them into a target rootfs, we've def had to carry a good number of downstream patches/hacks to the python infrastructure to make this work though.

We use existing buildroot infrastructure for downloading and dependency resolution(needed for system package dependency resolution that python packages often depend on) so pip I think kinda gets in the way there.

So maybe you need to be a front-end, then, in terms of PEP 517. It's pretty well-documented how to call the hooks. You could start by exploring how the Fedora's macros work, as an example: https://src.fedoraproject.org/rpms/pyproject-rpm-macros/blob/rawhide/f/pyproject_buildrequires.py.

pyproject.toml Outdated Show resolved Hide resolved
jameshilliard and others added 2 commits November 2, 2021 05:52
Co-authored-by: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>
CHANGES/6205.bugfix Outdated Show resolved Hide resolved
@webknjaz webknjaz changed the title Add requirement for compatible setuptools version. Set the minimum required setuptools to v46.4.0 Nov 2, 2021
@webknjaz webknjaz enabled auto-merge (squash) November 2, 2021 11:59
CHANGES/6205.misc Outdated Show resolved Hide resolved
Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! I've set this PR to auto-merge meaning that it'll get merged as soon as the CI passes. After that, the backporting bot will pick up the change and create PRs against branches 3.8 and 3.9 so it'll get released with the next v3.8.x.

@webknjaz webknjaz merged commit f8b07dd into aio-libs:master Nov 2, 2021
@patchback
Copy link
Contributor

patchback bot commented Nov 2, 2021

Backport to 3.8: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.8/f8b07dd62facc4c004b349d6e16b790f4b7205c2/pr-6205

Backported as #6225

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

@patchback
Copy link
Contributor

patchback bot commented Nov 2, 2021

Backport to 3.9: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.9/f8b07dd62facc4c004b349d6e16b790f4b7205c2/pr-6205

Backported as #6226

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-3.9 Trigger automatic backporting to the 3.9 release branch by Patchback robot bot:chronographer:provided There is a change note present in this PR infra meta
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants