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

python module: Respect PATH when python is not given in machine file #12116

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

chewi
Copy link

@chewi chewi commented Aug 12, 2023

We should only fall back to the Python interpreter running Meson itself if python3 is not found in the PATH.

See Gentoo bug #912051.

@chewi chewi requested a review from jpakkane as a code owner August 12, 2023 09:07
@chewi
Copy link
Author

chewi commented Aug 12, 2023

Arrgh. I tried to fix the failing Arch/PyPy tests by amending PATH, as they relied on the old behaviour, but this broke other Python-based tools like g-ir-scanner. Gentoo is smart as it uses its python-exec wrapper or otherwise adjusts the shebang to prevent the PATH from screwing things up like this. Unfortunately, Arch is not that smart. Maybe I'll have to fix the tests with a toolchain file instead, but I'm not sure how easy this will be yet.

@Volker-Weissmann
Copy link
Contributor

You should squash both commits into one commit:

https://mesonbuild.com/Contributing.html#strategy-for-merging-pull-requests-to-trunk

@Volker-Weissmann
Copy link
Contributor

You need to update the documentation on how python is found:

https://github.com/mesonbuild/meson/blob/master/docs/markdown/Python-module.md

@Volker-Weissmann
Copy link
Contributor

LGTM

@chewi chewi force-pushed the python-from-path branch 3 times, most recently from 7f4a9a3 to 5746229 Compare August 12, 2023 22:15
@chewi
Copy link
Author

chewi commented Aug 12, 2023

CI is killing me and I'm close to giving up. As far as I can tell, the Arch/PyPy job normally builds the boost test case against CPython because meson.build explicitly asks for python3. By pointing python in the native file to PyPy, this forces it to use that instead, and that somehow causes the detection to fail. I don't know whether this test case is really supposed to use CPython or not. Maybe it doesn't make sense for PyPy and we should skip it? Or are you explicitly testing that it doesn't choose PyPy here? We don't have a way to change the native file for just one test case. Advise appreciated.

@konsolebox
Copy link

konsolebox commented Aug 13, 2023

Maybe keep old behavior instead but add option to make the function follow PATH first? This at least will avoid surprises in existing setups. I'm not sure however how it will help Gentoo's case.

Copy link
Member

@eli-schwartz eli-schwartz left a comment

Choose a reason for hiding this comment

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

I talked about this a bit on the gentoo ticket too.

This (the find_installation code for determining which path to query as name_or_path) is some pretty old code and it's been in the back of my mind to fix it for a while. What actually happens is a bit complex:

  • find_installation() with no arguments will use meson's own sys.executable
  • find_installation('python3') will use whichever python3 executable is on PATH
  • find_installation(get_option('python_bin')) ditto except selected by the project option -Dpython_bin=whatever
  • setting the binary path for python = 'whatever' in a machine file overrides detection

I agree that ideally without arguments we would first look up a python3 from PATH. Also if python3 is explicitly specified but cannot be found (like on crazy FreeBSD systems where they refuse to install a python3 symlink to python3.9 or whatever is current), we should fall back to sys.executable. And the machine file lookup should prefer checking for a versioned python entry before checking for an unversioned entry.

mesonbuild/modules/python.py Outdated Show resolved Hide resolved
@chewi
Copy link
Author

chewi commented Aug 13, 2023

@konsolebox An option seems like an ugly compromise. I'd rather just using a machine file under Gentoo. The only reason I'm proposing this fix is because I feel quite strongly that the existing behaviour makes no sense. The fact that Meson is written in Python is an implementation detail, never mind the Python version it is running under. Muon is a Meson implementation written in C, so what would you expect that to do here?

The output of the failing boost test case was confusing me, so I reproduced the failure with PyPy locally and unearthed another bug. If python was pointed at Python 3 in a machine file, and that Python failed to be recognised as python2 then a subsequent search for python3 would fail because the previous failure was cached. I have attempted to fix this, but Eli has already provided feedback that needs addressing.

I have also had to exclude the boost test case on Arch/PyPy because it simply cannot work against PyPy. It was previously using CPython, despite Meson itself running under PyPy, but the machine file has changed that.

@chewi
Copy link
Author

chewi commented Aug 13, 2023

Arch/PyPy has started passing at last. Not sure why the Windows targets are failing now. 😕

@chewi
Copy link
Author

chewi commented Aug 13, 2023

I've just seen someone else's branch fail on the Windows targets in exactly the same way. Probably not my fault then?

@tristan957
Copy link
Contributor

Known failures.

@tristan957 tristan957 added this to the 1.3.0 milestone Aug 15, 2023
@chewi chewi changed the title python module: Respect PATH when python is not given in toolchain file python module: Respect PATH when python is not given in machine file Aug 15, 2023
If `python` was pointing at Python 3 in a machine file, and that Python
failed to be recognised as the requested `python2` then a subsequent
request for `python3` would fail because the previous failure was
cached against the resolved name/path alone.
We should only fall back to the Python interpreter running Meson itself
if `python3` is not found in the PATH.

A couple of tests relied on the old behaviour. Under Arch/PyPy, the
PATH's `python3` does not point to PyPy. Unfortunately, other
Python-based tools like g-ir-scanner are installed with a shebang of
`/usr/bin/env python3` on Arch, so adjusting the PATH to point to a
different Python breaks such tools. We must therefore specify `python`
in a machine file instead.

We also have to now exclude "test cases/frameworks/1 boost" on Arch/PyPy
because it cannot work against PyPy. It was previously using CPython,
despite Meson itself running under PyPy, but the machine file has
changed that.
@chewi
Copy link
Author

chewi commented Aug 18, 2023

CI has stopped playing up. In case it wasn't clear, I have addressed @eli-schwartz's feedback above. Please merge.

@xclaesse xclaesse modified the milestones: 1.3.0, 1.4.0 Oct 17, 2023
Dudemanguy added a commit to Dudemanguy/mpv that referenced this pull request Oct 29, 2023
Since distutils was dropped from python 3.12, we can't rely on a
standard library implementation for version comparison anymore hence
the switch to packaging instead which is essentially supposed to be the
replacement. This is problematic though because macOS can have several
ways of managing python can result in the build calling the wrong python
binary that doesn't have the actual packaging library installed. This
commit was a workaround for that since it would fetch the python used to
run the build (which is probably the one the user wants), but apparently
upstream doesn't like this and it's subject to change*. So instead,
let's solve try, and solve this a different way.

This reverts commit a57bd8e.

*: mesonbuild/meson#12116 (review)
Dudemanguy added a commit to mpv-player/mpv that referenced this pull request Oct 29, 2023
Since distutils was dropped from python 3.12, we can't rely on a
standard library implementation for version comparison anymore hence
the switch to packaging instead which is essentially supposed to be the
replacement. This is problematic though because macOS can have several
ways of managing python can result in the build calling the wrong python
binary that doesn't have the actual packaging library installed. This
commit was a workaround for that since it would fetch the python used to
run the build (which is probably the one the user wants), but apparently
upstream doesn't like this and it's subject to change*. So instead,
let's solve try, and solve this a different way.

This reverts commit a57bd8e.

*: mesonbuild/meson#12116 (review)
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