meson icon indicating copy to clipboard operation
meson copied to clipboard

python module: migrate away from the use of distutils

Open eli-schwartz opened this issue 2 years ago • 2 comments

Fixes #7702

See https://github.com/python/cpython/issues/99942 though.

eli-schwartz avatar Dec 02 '22 04:12 eli-schwartz

Codecov Report

Patch coverage: 85.71% and project coverage change: -1.66% :warning:

Comparison is base (586bd11) 70.33% compared to head (c5b7595) 68.67%.

:exclamation: Current head c5b7595 differs from pull request most recent head 3e20f08. Consider uploading reports for the commit 3e20f08 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #11133      +/-   ##
==========================================
- Coverage   70.33%   68.67%   -1.66%     
==========================================
  Files         218      414     +196     
  Lines       48336    88040   +39704     
  Branches    11463    20784    +9321     
==========================================
+ Hits        33996    60463   +26467     
- Misses      11809    23064   +11255     
- Partials     2531     4513    +1982     
Files Changed Coverage Δ
mesonbuild/modules/python.py 73.15% <80.00%> (-5.74%) :arrow_down:
mesonbuild/scripts/python_info.py 90.19% <90.19%> (+1.55%) :arrow_up:

... and 399 files with indirect coverage changes

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Dec 02 '22 04:12 codecov[bot]

Thanks for the investigative work @eli-schwartz

While considering the issue of determining whether linking to libpython is required, it may be worth checking if we have all the pieces required to support the stable ABI that defines slightly different rules for linking https://peps.python.org/pep-0384/#linkage

I think python_installation.extension_module() could grow a abi kwarg that can be used to request the "stable ABI special treatment": linking with the correct dll on Windows and probably defining the Py_LIMITED_API preprocessor variable appropriately https://docs.python.org/3/c-api/stable.html

dnicolodi avatar Dec 02 '22 09:12 dnicolodi

I tried to get some attention to the Python side of the issue https://discuss.python.org/t/building-extensions-modules-in-a-post-distutils-world/23938

dnicolodi avatar Feb 16 '23 15:02 dnicolodi

@eli-schwartz Things are moving on the Python side. Now that we have some attention from the Python developers, is there anything else other than the two PRs you already opened on cpython that is needed to remove the dependency on distutils at least for Python 3.12 and later?

dnicolodi avatar Feb 17 '23 10:02 dnicolodi

Thanks for getting the ball rolling!

I would still like to make the sysconfig API able to expose the information we are grabbing via heuristics, but that's no longer a merge blocker, just a "good FOSS citizen" thing.

eli-schwartz avatar Feb 17 '23 14:02 eli-schwartz

My understanding is that there is no CPython core developer with the interest and the knowledge to get the required information into a consistent sysconfig API. The only way to get that is to propose patches to sysconfig. Once this PR and #11392 are merged I can work into refactoring some of heuristic into patches for sysconfig (I prefer to wait for thise patches to land because they clean up the implementation quite a bit and they make this job easier). However, Meson would not be able to depend on the improvements on sysconfig for a very long time, thus this would not have high priority.

dnicolodi avatar Feb 17 '23 14:02 dnicolodi

My understanding is that there is no CPython core developer with the interest and the knowledge to get the required information into a consistent sysconfig API.

I am working on this.

I would still like to make the sysconfig API able to expose the information we are grabbing via heuristics, but that's no longer a merge blocker, just a "good FOSS citizen" thing.

Which information specifically?

FFY00 avatar Mar 01 '23 04:03 FFY00

The release date for Python 3.12 is approaching. Having this merged and have a Meson release before then would be great. @eli-schwartz I can push this through, if you like.

dnicolodi avatar Jul 09 '23 12:07 dnicolodi

@eli-schwartz You mentioned having a rebase of this on master. Can you please push it? I would like to test it in https://github.com/mesonbuild/meson-python/pull/492 instead of using my own rebase. Thanks

dnicolodi avatar Sep 11 '23 11:09 dnicolodi

@eli-schwartz It took you less than one hour to close my PR with a rebase of this work. You said that you already have the rebases commits and that if I wanted this PR to be rebased I should have simply asked. I've asked multiple times now. It is fine if you don't want to work on this anymore, but I don't understand why you don't want anyone else to work on it either.

dnicolodi avatar Sep 18 '23 09:09 dnicolodi

I'd originally hoped to move this forward a long time ago, sorry for the lateness. In the interest of ensuring that this PR can have smooth sailing, I'd like to clarify the goals of the PR, and what is and is not on-topic for it.

In particular, the PR topic is "migrate away from the use of distutils", and strictly speaking, that is all It is intended to do -- take existing code, and port it over, bug for bug, from deprecated APIs onto their non-deprecated equivalent APIs.

(Along the way, I have also staged some generic cleanups for the python module, which are however not the purpose of the PR, merely additional extras.)

What this means is that it is on-topic for this PR, to discuss:

  • matters pertaining to effecting a distutils-to-sysconfig migration in a bug-for-bug manner
  • any additional changes present in the PR can be reviewed for their effectiveness in accomplishing the goals that the commits in question propose to accomplish

Not on-topic for this PR:

  • harping on unrelated bits of functionality that simply happen to share the same python module, or utilize the results of the information that is being bug-for-bug ported from distutils to sysconfig

Obviously, meson is happy to hear your (rhet.) bug reports about any topic at all, whatsoever, which is about "meson". It doesn't have to have anything to do with a particular topic. But if you do so, it had better receive its own distinct bug report. Where it will be, crucially, on-topic.

No useful purpose is served in bogging down a PR that is about bug-for-bug porting of distutils to sysconfig, with tangents that are not blockers for bug-for-bug porting of distutils to sysconfig. Nor in wearing out maintainers who just wanted to fix bugs that run contrary to the intentions of the code ("the thing failed with a ModuleNotFoundError" is a heck of a bug, yes?), did not want to be sidetracked into significantly impactful, wide-ranging policy decisions, and decide their time is better spent doing more enjoyable things.

Again, apologies for the lateness and the lack of communication. Thank you for your patience. <3

Let's see if we can get the ball rolling and merged for python 3.12 final release.

eli-schwartz avatar Sep 22 '23 21:09 eli-schwartz

Thanks @eli-schwartz. Is avoiding loading distutils from setuptools done in excess of caution or did you find any instance in which that is actually problematic? I'm not aware of any issue caused by that so far. I don't see any reason why forcing the use of distutils from the standard library could be detrimental, thus adding this extra safety is ok, but I'm curious.

dnicolodi avatar Sep 22 '23 22:09 dnicolodi

The commit message for the first patch is maybe a bit misleading. AFAIK, setuptools does not inject a monkey-patched version of distutils into the import path, it injects a backport of distutils, but distutils in the standard lib hasn't seen much development in the last few Python releases, thus it is basically plain distutils. I'll be very surprised if there is any actual behavior difference between the setuptools's distutils and the standard lib distutils.

dnicolodi avatar Sep 22 '23 22:09 dnicolodi

I have seen it actually error out. https://github.com/pypa/setuptools/issues/3744#issuecomment-1444536003

AttributeError: partially initialized module 'setuptools' has no attribute 'version' (most likely due to a circular import)

Whatever happened there, it's not setuptools' fault. But since we don't need it, I'd prefer to avoid permitting it to complicate the debugging process.

There are also some fascinating errors that happen if, say, you have specific versions of setuptools installed that are pinned by numpy/SciPy, and then you try to compile CPython from source. The official sanctioned CPython solution seems to be that it's setuptools' job to solve that, which I think is odd because it's pretty indicative of a much more major problem: namely that on versions of CPython where the built python is run as part of the build (mainly, where setup.py is used to build much of the stdlib), any python module installed into the user home directory can break the CPython build if it injects a .pth file and runs code on startup. setuptools happens to do so by replacing a stdlib module which fairly well guarantees that the CPython build will import the user homedir code, but it's hardly unique to setuptools.

I actually have a CPython patch to fix this on my laptop, but since it's only applicable to maintenance branches of CPython I wasn't sure whether I should submit it. My impression is that they're unlikely to merge it because it is "too old". I couldn't even get the cygwin fix for python.pc backported, and that's a guaranteed pretty safe fix considering that cygwin has patched it themselves with a local patch for years.

eli-schwartz avatar Sep 22 '23 22:09 eli-schwartz

One nice side effect of inhibiting setutools to load itself when importing distutils is that the import time decreases significantly:

$ time SETUPTOOLS_USE_DISTUTILS=stdlib python3.11 -c 'import distutils; print(distutils.__file__)'
<string>:1: DeprecationWarning: The distutils package is deprecated and slated for removal in Python 3.12. Use setuptools or check PEP 632 for potential alternatives
/usr/local/Cellar/[email protected]/3.11.5/Frameworks/Python.framework/Versions/3.11/lib/python3.11/distutils/__init__.py

real	0m0.070s
user	0m0.037s
sys	0m0.017s
$ time python3.11 -c 'import distutils; print(distutils.__file__)'
/usr/local/lib/python3.11/site-packages/setuptools/_distutils/__init__.py

real	0m0.289s
user	0m0.182s
sys	0m0.056s

dnicolodi avatar Sep 22 '23 22:09 dnicolodi

Python 3.12 has been released today https://www.python.org/downloads/release/python-3120/ To avoid a flood of bug reports it would be great if this PR could be finished and a new meson release cut ASAP. There are a few ways to solve the PyPy issue. The most conservative is to switch away from the distutils probe for Python 3.12 onward only. Anyhow, any of the proposed fix would work.

dnicolodi avatar Oct 02 '23 17:10 dnicolodi

There are a few ways to solve the PyPy issue. The most conservative is to switch away from the distutils probe for Python 3.12 onward only.

I opted instead to try handling it by using the previous logic not just on cpython <=3.7, but also on pypy-any-version. Please take a look.

eli-schwartz avatar Oct 02 '23 22:10 eli-schwartz

I opted instead to try handling it by using the previous logic not just on cpython <=3.7, but also on pypy-any-version. Please take a look.

Thanks @eli-schwartz It looks good to me. I've run the changes through the meson-python CI and everything works as expected also on PyPy.

I think this can be merged. The pylint error is clearly unrelated.

dnicolodi avatar Oct 02 '23 23:10 dnicolodi

I submitted the pylint error as https://github.com/pylint-dev/pylint/issues/9095 since it seems to be incorrectly detected.

eli-schwartz avatar Oct 02 '23 23:10 eli-schwartz

@eli-schwartz I think this the "work in progress" status may be removed now

dnicolodi avatar Oct 03 '23 10:10 dnicolodi

is needed to get python 3.12 working reliably

Well, it is needed to get Python 3.12 working at all.

dnicolodi avatar Oct 03 '23 15:10 dnicolodi

Well, it is needed to get Python 3.12 working at all.

pip install setuptools has entered the chat

Okay okay I jest. In fact, that won't even work in some cases like spack, because... that only works when setuptools is in a site-packages directory, not when it is added via $PYTHONPATH. Those .pth files, huh.

eli-schwartz avatar Oct 03 '23 15:10 eli-schwartz

Marking for the point release milestone. This is needed ASAP for python 3.12, and I think it has very low chance of regression ;) as its effects are essentially entirely within the introspection script.

eli-schwartz avatar Oct 03 '23 15:10 eli-schwartz

pip install setuptools has entered the chat

Sure, that works (with the caveats you highlighted) but then meson would need to depend on setuptools when installed on Python 3.12 (which is doable and is what meson-python has been doing for a while now).

dnicolodi avatar Oct 03 '23 15:10 dnicolodi

@nirbheek backport branch is up at https://github.com/eli-schwartz/meson/tree/pep632-backport

eli-schwartz avatar Oct 03 '23 16:10 eli-schwartz