python module: migrate away from the use of distutils #11133
Conversation
Codecov ReportPatch coverage:
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
☔ View full report in Codecov by Sentry. |
|
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 |
mesonbuild/modules/python.py
Outdated
|
|
||
| with importlib.resources.path('mesonbuild.scripts', 'python_info.py') as f: | ||
| cmd = self.get_command() + [f] | ||
| p, stdout, stderr = mesonlib.Popen_safe(cmd) |
There was a problem hiding this comment.
importlib.resources.path() returns pathlib.Path object. The subprocess modules (used under the hood in mesonlib.Popen_safe()) accepts path objects as part of the command line only starting with Python 3.8. This should probably be
cmd = self.get_command() + [os.fspath(f)]or something similar.
| # on versions supporting python-embed.pc, this is the non-embed lib | ||
| if sys.version_info >= (3, 8): | ||
| variables = sysconfig.get_config_vars() | ||
| return bool(variables.get('LIBPYTHON', 'yes')) |
There was a problem hiding this comment.
Just nitpicking, but
bool(variables.get('LIBPYTHON', True))seems more idiomatic to me.
There was a problem hiding this comment.
Unfortunately this does not work on PyPy. On PyPy 3.9 and 3.10 sysconfig.get_config_vars() does not contain a LIBPYTHON key, thus .get() returns the default value also on Linux and macOS. Either the fallback to distutils is applied for Python versions up to 3.10, or it is also made conditional on the interpreter being PyPy.
There was a problem hiding this comment.
The current code assumes that if the LIBPYTHON variable is not present, the code is being executed on Windows, and thus libpython needs to be linked. Therefore, another possibility would be to do not make this assumption and make the check explicit by replacing
return bool(variables.get('LIBPYTHON', 'yes'))with
return sys.platform == 'win32' or bool(variables.get('LIBPYTHON'))There was a problem hiding this comment.
Do we know what PyPy did across various different historic versions?
There was a problem hiding this comment.
I've tested only pypy3.9 and pypy3.10
There was a problem hiding this comment.
So how are consumers meant to know the name of the import lib and its path on windows? The discussion at the top of the sysconfig rewrite suggests
Is a shared libpython available? What's its name? Location?
- definition: Yes if built with --enable-shared
- currently:
- (available?) bool(sysconfig.get_config_var('LDLIBRARY'))
- (name?) sysconfig.get_config_var('LDLIBRARY')
- (location?) sysconfig.get_config_var('LIBDIR')
but I don't think reality concurs with the "currently" status. Or maybe I don't understand the intent.
There was a problem hiding this comment.
Please note that the variable names in the text you quote from the CPython PR and the one used here are different, thus I'm not sure how that fits here. A bit of magic goes into obtaining the library name to use and its location. This is because there isn't a reliable way to get that information that works on all architectures across all versions of Python supported by Meson. This is unfortunate and a bit more complex that what is inferred from the CPython PR you link to.
I don't think that the quirks of how the information required for building extension modules can be retrieved from a CPython installation are nice or the best way things could be, but having all Python implementation implement the same interface helps the consumers of such information. At the same time, there is no fixing for the already released PyPy versions, thus Meson will need to find a way to work with what is out there.
There was a problem hiding this comment.
So how are consumers meant to know the name of the import lib and its path on windows? The discussion at the top of the sysconfig rewrite suggests
You are quite right, CPython doesn't really expose this information at all for Visual Studio solutions file builds and that is partially because those builds are a really odd duck in a whole bunch of ways and partially because there isn't really an official way to get this information at all.
The sysconfig variables we're discussing here are the best we have and they don't really work at all on Windows...
... which is why I suggested that once we are adding stuff manually to PyPy's sysconfigdata, it might be good to go the extra mile and provide this information even on Windows, where CPython doesn't provide it. :D
There was a problem hiding this comment.
but I don't think reality concurs with the "currently" status. Or maybe I don't understand the intent.
It tries to show how people generally get that value at the moment, not to suggest how they should do it. It is also very Posix-centric as that's my background and that issue was not meant to be an extensive document — just help to paint the picture. PEP 720 OTOH tries to do that, but it still lacks a lot of information.
This PR is a good example of why the current way is problematic, which is what we want to fix.
So how are consumers meant to know the name of the import lib and its path on windows?
I don't think there is a portable/reliable way, unfortunately.
python/cpython#110049 will help with that, but only on >=3.13.
There was a problem hiding this comment.
One of the advantanges of PyPy is that I can make changes that will be available in a nightly build of python3.9/3.10, and these can be easily tried out in CI. So if the direction in PR 10049 looks correct, I can add that to PyPy so that, at least on PyPy, meson could get the values from the installed python.
Signed-off-by: Filipe Laíns <[email protected]>
Meson up to version 1.0.0 uses distuitls for introspection of the Python interpreter it is compiling for. distutils has been removed from the Python standard library in version 3.12. distutils has been effectively embedded into setuptools thus depending on setuptools works around the issue. See mesonbuild/meson#11133.
Meson up to version 1.0.0 uses distuitls for introspection of the Python interpreter it is compiling for. distutils has been removed from the Python standard library in version 3.12. distutils has been effectively embedded into setuptools thus depending on setuptools works around the issue. See mesonbuild/meson#11133.
| @@ -58,10 +58,15 @@ def get_install_paths(): | |||
| return paths, install_paths | |||
|
|
|||
| def links_against_libpython(): | |||
| from distutils.core import Distribution, Extension | |||
There was a problem hiding this comment.
I don't understand the reference to "embed" in the commit message. The code using distutils probes the build_ext command, which AFAIU doesn't have anything to do with embedding python.
There was a problem hiding this comment.
Right, this is the non-embed probe. The embed: false probe.
That could indeed use rewording.
There was a problem hiding this comment.
Yes, that is what I meant. I didn't understand what "stop using distutils "embed" probe" in the commit message means, because AFAICT, this does not probe for the build options to embed Python.
|
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 |
|
@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? |
|
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. |
|
My understanding is that there is no CPython core developer with the interest and the knowledge to get the required information into a consistent |
Meson up to version 1.0.0 uses distuitls for introspection of the Python interpreter it is compiling for. distutils has been removed from the Python standard library in version 3.12. distutils has been effectively embedded into setuptools thus depending on setuptools works around the issue. See mesonbuild/meson#11133.
I am working on this.
Which information specifically? |
|
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. |
|
@eli-schwartz You mentioned having a rebase of this on master. Can you please push it? I would like to test it in mesonbuild/meson-python#492 instead of using my own rebase. Thanks |
|
@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. |
|
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:
Not on-topic for this PR:
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. |
|
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 We do not use setuptools for anything, and only lightly use distutils. Unpredictable issues can occur due to setuptools monkey-patching, which interferes with our intended use. Tell setuptools to simply never get involved. Note: while it's otherwise possible to check if the probe is run using sys.executable and avoid forking, setuptools unconditionally injects itself at startup in a way that requires subprocess isolation to disable.
Since 3.10.3, Debian finally started patching sysconfig with custom paths, instead of just distutils. This means we can now go use that instead. It reduces our reliance on the deprecated distutils module. Partial fix for mesonbuild#7702
…nt python On python >=3.8, this information is expected to be encoded in the sysconfig vars. In distutils, it is always necessary to link to libpython on Windows; for posix platforms, it depends on the value of LIBPYTHON (which is the library to link to, possibly the empty string) as generated by configure.ac and embedded into python.pc and python-config.sh, and then coded a second time in the distutils python sources. There are a couple of caveats which have ramifications for Cygwin and Android: - python.pc and python-config.sh disagree with distutils when python is not built shared. In that case, the former act the same as a shared build, while the latter *never* links to libpython - python.pc disagrees with python-config.sh and distutils when python is built shared. The former never links to libpython, while the latter do The disagreement is resolved in favor of distutils' behavior in all cases, and python.pc is correct for our purposes on python 3.12; see: python/cpython#100356 python/cpython#100967 Although it was not backported to older releases, Cygwin at least has always patched in a fix for python.pc, which behavior is now declared canonical. We can reliably assume it is always correct. This is the other half of the fix for mesonbuild#7702
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 I think this can be merged. The pylint error is clearly unrelated. |
|
I submitted the pylint error as pylint-dev/pylint#9095 since it seems to be incorrectly detected. |
Testing the correctness of the `modules: ` kwarg can be done with other guaranteed stdlib modules that are even more guaranteed since they didn't get deprecated for removal.
meson itself runs okay on 3.12, and the last issue for *probing* against 3.12 is solved. Tests pass here locally.
|
@eli-schwartz I think this the "work in progress" status may be removed now |
Well, it is needed to get Python 3.12 working at all. |
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 |
|
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. |
Sure, that works (with the caveats you highlighted) but then meson would need to depend on |
|
@nirbheek backport branch is up at https://github.com/eli-schwartz/meson/tree/pep632-backport |
Fixes #7702
See python/cpython#99942 though.