python module: stop using distutils on sufficiently new python #12237
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
dnicolodi
force-pushed
the
rm-distutils
branch
2 times, most recently
from
0056182 to
5403061
Compare
If the found program happens to be sys.executable, we can do an optimization and get the function return value instead of communicating via subprocess + json. The results are identical either way.
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.
Disagreement between distutils and sysconfig have been resolved for Python 3.12, see python/cpython#100356 and python/cpython#100967 This is the other half of the fix for mesonbuild#7702.
dnicolodi
force-pushed
the
rm-distutils
branch
from
5403061 to
dfef1a4
Compare
|
Or you can ask me to rebase. I was already planning on doing it!!! |
|
I did #11133 (comment) But I did not receive an answer. The rebase is not trivial. Feel free to use these commits if you like. I actually don't see the need to close this, as the commits all preserve the original metadata... but whatever. |
|
FWIW this passes all the |
Apologies for not publicly responding. Though in general I'd appreciate in such a situation if you asked first for my schedule and made it clear you were considering to copy it into a new PR if necessary to fit some timeframe of your own, as I was very startled by this... FWIW there is some context here on the lag-- I'd originally planned to try getting this in back in February but due to harassment I got burnt out from it. It has always been my plan to get this done in time for python 3.12, and 6 months later I am in fact feeling a bit better about doing so... The rebase isn't a problem, at least. I have had the patches in my local worktree all this time -- I haven't worked on it or sanity checked it, but I have rebased it along with everything else I have as local patches -- I have followed the same general workflow as described here, since probably 2015: https://drewdevault.com/2020/04/06/My-weird-branchless-git-workflow.html In terms of the rebase you have done, there are also tweaks that I don't really understand but which differ from what I have done. e.g. I'm not sure whether to keep using distutils on python 3.11 is the best choice? (I had an explicit rationale for choosing 3.8, and my concern was less about fixing CPython than about declaring a certain behavior canonical.) I'm also regretting the fourth patch and have an alternative patch that obsoletes it. |
That is what I exactly did. Knowing the backstory a bit I thought that insisting in getting a reply from you was counterproductive. I don't think that publishing a PR with a rebase of some patches is the big deal that you seem to be making of it. I thought that help could be appreciated, I didn't want to step on anyone's toes.
I probably misremember, but I purposely introduced only three changes to the patches. I'll mark them in the diff with my reasoning for them.
I based that on the fact that your fixes for CPython (PRs linked in the commit message) AFAIK have been merged only for Python 3.12.
After rebasing there are only three patches. I think you mean the one running the introspection in-process? I've included that mainly to minimize the deviations from your original series as it does not affect functionality. |
| else: | ||
| import importlib.resources | ||
| with importlib.resources.path('mesonbuild.scripts', 'python_info.py') as f: | ||
| cmd = self.get_command() + [os.fspath(f)] |
There was a problem hiding this comment.
Here I've added the os.fspath(). It is needed because on Python 3.7 it is not possible to use a pathlib.Path as returned from importlib.resources.path() as a command array element for subprocess.run() and friends (which I believe bake the implementation of mesonlib.Popen_safe()). mypy complains if the os.fspath() is not present.
| cmd.ensure_finalized() | ||
| return bool(cmd.get_libraries(Extension('dummy', []))) | ||
| # on versions supporting python-embed.pc, this is the non-embed lib | ||
| if sys.version_info >= (3, 12): |
There was a problem hiding this comment.
I've changed this from (3, 8) (I think) to (3, 12). I've already explained the reason.
| # on versions supporting python-embed.pc, this is the non-embed lib | ||
| if sys.version_info >= (3, 12): | ||
| variables = sysconfig.get_config_vars() | ||
| return bool(variables.get('LIBPYTHON', True)) |
There was a problem hiding this comment.
I've changed this to this form from
return bool(variables.get('LIBPYTHON', 'yes'))because the yes there always makes me pause and wonder if someone expects LIBPYTHON to be set to no and have bool() return false in that case. It is clearly not the case, but True avoids the mental overhead.
|
BTW, the "Arch / PyPy" failure looks very much related. |
|
I've extended the test matrix for |
It was caused by a rebase error. It should be fixed now. |
This is a rebase of #11133 on current master. @eli-schwartz I took the freedom of updating this PR of yours that haven't seen activity since quite a while. With the Python 3.12 release approaching it makes sense to push this across the finish line sooner rather than later.