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

Prefer importing build_py and sdist from setuptools #6007

Merged
merged 2 commits into from
Nov 5, 2021

Conversation

grlee77
Copy link
Contributor

@grlee77 grlee77 commented Nov 4, 2021

Description

closes #6006

This PR updates some imports to use setuptools own versions rather than the ones from distutils. This is needed for compatibility with recent setuptools (>58.5.0) which made this breaking change: pypa/setuptools@2e66eb7

Checklist

  • Docstrings for all functions
  • Gallery example in ./doc/examples (new features only)
  • Benchmark in ./benchmarks, if your changes aren't covered by an
    existing benchmark
  • Unit tests
  • Clean style in the spirit of PEP8
  • Descriptive commit messages (see below)

For reviewers

  • Check that the PR title is short, concise, and will make sense 1 year
    later.
  • Check that new functions are imported in corresponding __init__.py.
  • Check that new features, API changes, and deprecations are mentioned in
    doc/release/release_dev.rst.

needed for compatibility with setuptools >= 58.5.0
@grlee77 grlee77 added the 🩹 type: Bug fix Fixes unexpected or incorrect behavior label Nov 4, 2021
@grlee77
Copy link
Contributor Author

grlee77 commented Nov 4, 2021

Upstream issue: pypa/setuptools#2849

@grlee77
Copy link
Contributor Author

grlee77 commented Nov 4, 2021

This may end up being resolved upstream in which case we can just disallow 58.5.0 and 58.5.1. We may still want to make this change, though?

pkgw added a commit to pkgw/pywwt that referenced this pull request Nov 4, 2021
The most recent version of setuptools has a bug that breaks our
install: pypa/setuptools#2849

Here we copy the solution from
scikit-image/scikit-image#6007 .
@abravalheri
Copy link

This may end up being resolved upstream in which case we can just disallow 58.5.0 and 58.5.1. We may still want to make this change, though?

The support for commands inheriting directly from distutils is considered deprecated by setuptools (and distutils itself will be removed from Python as per PEP 632), so I think this is a positive change :)

@hmaarrfk
Copy link
Member

hmaarrfk commented Nov 5, 2021

do we need the try statement? i feel like with distutils being deprecated we should move away from it completely.

@grlee77
Copy link
Contributor Author

grlee77 commented Nov 5, 2021

Not sure. I hadn't checked if these are present in our oldest supported setuptools.

Copy link
Member

@rfezzani rfezzani 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 @grlee77

verified install with older setuptools (0.45)
@grlee77
Copy link
Contributor Author

grlee77 commented Nov 5, 2021

Do we need the try statement? i feel like with distutils being deprecated we should move away from it completely.

It works without the try/except for setuptools 0.45, so I have removed it. I'm not sure about how to best avoid importing the CompileError or LinkError classes from distutils. As far as I could tell those are not in the public API of setuptools (they are under setuptools._distutils.errors).

@abravalheri
Copy link

abravalheri commented Nov 5, 2021

I'm not sure about how to best avoid importing the CompileError or SetupError classes from distutils. As far as I could tell those are not in the public API of setuptools (they are under setuptools._distutils.errors).

Maybe the best is to open an issue to setuptools asking for suggestions? They might think the best is to expose these in the public API.

@grlee77
Copy link
Contributor Author

grlee77 commented Nov 5, 2021

Maybe the best is to open an issue to setuptools asking for suggestions? They might think the best is to expose these in the public API.

Just found this issue that seems to cover it already: pypa/setuptools#2698

@abravalheri
Copy link

abravalheri commented Nov 5, 2021

Ok, it should be a matter of PR then. I am sure the folks in setuptools would find it reasonable, or at least provide a clear alternate path.

Maybe this part can be addressed in the future, when it is available (aka TODO)?

@grlee77
Copy link
Contributor Author

grlee77 commented Nov 5, 2021

Agreed. I will merge this as is today once the CI completes.

@grlee77 grlee77 merged commit c073f0e into scikit-image:main Nov 5, 2021
@abravalheri
Copy link

abravalheri commented Nov 7, 2021

@grlee77, there you go: pypa/setuptools#2858.

It should be available soon in a release 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🩹 type: Bug fix Fixes unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

installation issues with setuptools 58.5.x
4 participants