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

Replaced all uses of distutils with setuptools #428

Merged
merged 14 commits into from Dec 30, 2021
Merged

Conversation

agronholm
Copy link
Contributor

This gets rid of any (direct) distutils dependencies by relying on setuptools instead. It does mean we depend on setuptools as an install dependency but I don't see that as a problem.

Distutils has been deprecated and will be removed in Python 3.12.
The sysconfig.get_platform() and distutils.util.get_platform() functions return "linux-x64_64", not "linux_x64_64" which the previous code and tests assumed.
@codecov
Copy link

codecov bot commented Dec 24, 2021

Codecov Report

Merging #428 (d859302) into main (afc0d5e) will increase coverage by 0.54%.
The diff coverage is 82.92%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #428      +/-   ##
==========================================
+ Coverage   67.03%   67.57%   +0.54%     
==========================================
  Files          11       12       +1     
  Lines         904      916      +12     
==========================================
+ Hits          606      619      +13     
+ Misses        298      297       -1     
Impacted Files Coverage Δ
src/wheel/cli/__init__.py 23.52% <ø> (-3.26%) ⬇️
src/wheel/cli/convert.py 38.70% <66.66%> (-0.40%) ⬇️
src/wheel/bdist_wheel.py 49.63% <69.23%> (ø)
src/wheel/_setuptools_logging.py 91.66% <91.66%> (ø)
src/wheel/util.py 100.00% <100.00%> (ø)
src/wheel/wheelfile.py 100.00% <100.00%> (ø)

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 afc0d5e...d859302. Read the comment docs.

@agronholm
Copy link
Contributor Author

I missed one distutils import in wheelfile.py. Fixing.

Setuptools will now always be present so this is pointless. The pkg_resources module was never imported here anyway.
@@ -30,3 +31,14 @@ def as_bytes(s):
return s.encode("utf-8")
else:
return s


def log(msg, *, error=False):
Copy link
Member

Choose a reason for hiding this comment

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

Rather than implementing a custom logger, I'd prefer to re-use Python's logging module. Does that not work in this scenario because the logger is not initialized? :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. Even setuptools still uses the distutils logger internally.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is a defect that needs to be addressed in Setuptools. It should supply a log object similar to what distutils does (but probably using Python's logging module).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, but until that's in place, do you see any problem with using this approach now? It should be functionally equivalent to the distutils logging code (just with a newer syntax and leveraging print().

Copy link
Member

Choose a reason for hiding this comment

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

In pypa/setuptools#2973, I've captured the need for a replacement. I realize that Setuptools won't be able to supply a replacement for distutils in such a way that it's available on older Setuptools releases, so wheel will probably not be able to rely on it for a while. Therefore, I'd recommend that wheel provide an interface-compatible fallback, and perhaps just a copy of the implementation that makes it into Setuptools. I may be able to get that implementation out today.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, looking forward to it.

Copy link
Member

Choose a reason for hiding this comment

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

In pypa/setuptools#2974, I believe I have a technique that will configure the Python logging mechanism to log to stderr/stdout as distutils.log currently does.

Then, in https://github.com/pypa/wheel/tree/python-logging, I illustrate how this approach could be employed in wheel to log through that mechanism. I could have even restored the logger name and % substitutions, making the diff from main very small, but opted to leave those changes.

The problem with that approach is that for older Setuptools, nothing will have configured the logger, so the default configuration of the logger will apply. In https://github.com/pypa/wheel/tree/python-logging-compat, I apply another commit that copies the code that configures the logger and run that. That approach will give a compatible effect, but still won't honor any calls to distutils.log.set_threshold on older Setuptools. That seems like an acceptable tradeoff (and one this PR has already made).

Please have a look at the Setuptools PR and then consider pulling one or both of the aforementioned commits for this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I'll look at those branches later.

@henryiii
Copy link
Contributor

Not for right now, but would using pypa/flit instead of setuptools be better? I know a few other tools are planning on switching (packaging IIRC?), and that would get rid of the chicken/egg problem and allow a pyproject.toml file again. flit_core is entirely self contained, I believe. Just a thought.

@agronholm
Copy link
Contributor Author

Can we rely on ubiquitous PEP 517 support everywhere? That would be a requirement for using flit, yes? Anybody whose systems run python setup.py install or similar would have a bad time. Maybe it's best to ditch setup.py altogether then?

@henryiii
Copy link
Contributor

Packages will need to soon. It would be great if installer was ready - maybe that's the last piece needed? Systems are beginning to get serious about ditching setup.py commands - possibly spurred on by https://blog.ganssle.io/articles/2021/10/setup-py-deprecated.html, and possibly by the inclusion of Flit into the PyPA.

@henryiii
Copy link
Contributor

Here's the discussion I was remembering: https://discuss.python.org/t/debundling-the-next-pip-release-will-require-handling-pyproject-toml-based-build-backends/12329

packaging and build likely will make the switch at some point in 2022. If you are in the PyPA discord, we just had a bit of discussion in off-topic.

@henryiii
Copy link
Contributor

Adding future annotations works in the wild, it seems. :)

@agronholm
Copy link
Contributor Author

If you are in the PyPA discord, we just had a bit of discussion in off-topic.

I didn't know there is a PyPA discord server. Where was that announced?

@henryiii
Copy link
Contributor

I think there's a discuss.python.org about it? There's a badge in the readme here: https://github.com/pypa/setuptools. The PyPA projects are encouraged to add badges and request channels. There's a "nice" URL for it, but I don't remember what it is.

@henryiii
Copy link
Contributor

@agronholm
Copy link
Contributor Author

@jaraco I think this is ready for merging, yes?

@jaraco
Copy link
Member

jaraco commented Dec 30, 2021

I say go for it!

@agronholm agronholm merged commit 31d8972 into main Dec 30, 2021
@agronholm agronholm deleted the remove-distutils branch December 30, 2021 22:48
@agronholm
Copy link
Contributor Author

Thanks everyone!

@agronholm
Copy link
Contributor Author

Not for right now, but would using pypa/flit instead of setuptools be better? I know a few other tools are planning on switching (packaging IIRC?), and that would get rid of the chicken/egg problem and allow a pyproject.toml file again. flit_core is entirely self contained, I believe. Just a thought.

My plan is to have every packaging back-end depend on wheel after the public API is released. That would mean that the back-end wheel itself uses would have to vendor wheel. That's a bummer, but I can't immediately think of a way around that.

@henryiii
Copy link
Contributor

henryiii commented Jan 2, 2022

flit-core already venders dependencies (only packaging, IIRC)

@pfmoore
Copy link
Member

pfmoore commented Jan 2, 2022

An alternative would be to include a custom in-tree backend. It only has to implement build_sdist and build_wheel, and you could hard-code the files to include and use the wheel API to create the wheel. (That's actually an interesting project - I might look at writing a "self-hosting" backend like this myself...)

@henryiii
Copy link
Contributor

henryiii commented Jan 2, 2022

This is how flit-core itself is built (using itself): https://github.com/pypa/flit/blob/main/flit_core/pyproject.toml

@pfmoore
Copy link
Member

pfmoore commented Jan 2, 2022

Oh cool! I hadn't realised flit had done that (yet). I knew they had been doing some work on bootstrapping, but not that they'd separated the build backend out totally.

I guess this means that flit-core won't ever use the API from wheel to build wheels, but that seems totally fine to me, for something that is deliberately intended as a "lowest possible level" backend. After all, the whole point in having the standard is that we don't tie people into having to use a single implementation.

@agronholm
Copy link
Contributor Author

I guess this means that flit-core won't ever use the API from wheel to build wheels

If this is true, then it means I could restore pyproject.yaml for the upcoming release.

@pfmoore
Copy link
Member

pfmoore commented Jan 2, 2022

If you switch to flit for the build backend, then yes, I guess it does.

@agronholm
Copy link
Contributor Author

I have a PR for that now: #436. Reviews are welcome.

@mayeut
Copy link
Member

mayeut commented Jan 3, 2022

@gaborbernat, this might break pypa/virtualenv in some corner cases in the future:

  • Using --no-setuptools without --no-wheel
  • Using a version of setuptools not compatible with wheel (the specifier here is quite old already so, as long as it is the case, shouldn't be much of an issue but if it changes, that might be another story, e.g. a patch version of wheel requires a version of setuptools that's too recent to be considered by periodic updates => virtual environment is broken, wheel is not usable)

IMHO those are really corner cases but I thought I'd give you a head's up.

@gaborbernat
Copy link

Thanks for the update, if this goes ahead we should disallow installing wheel without setuptools in virtualenv. A perhaps in that sense at that point in time would be welcome, but indeed is an edge case.

@agronholm
Copy link
Contributor Author

Thanks for the update, if this goes ahead we should disallow installing wheel without setuptools in virtualenv

This adds an install dependency of setuptools to wheel, so do you need to do anything?

@gaborbernat
Copy link

Of course, virtualenv provisions seed packages that until today have no dependencies. If you're adding dependencies now virtualenv needs to make sure they are satisfied post provision. This adds a whole new complexity to the project. We've had this discussion when you added packaging dependency to the project a while back.

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

6 participants