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

modules/python: fix path introspection on virtual environments on Debian #9388

Closed
wants to merge 2 commits into from

Conversation

FFY00
Copy link
Member

@FFY00 FFY00 commented Oct 11, 2021

Signed-off-by: Filipe Laíns lains@riseup.net

FFY00 added 2 commits October 11, 2021 13:56
Signed-off-by: Filipe Laíns <lains@riseup.net>
Signed-off-by: Filipe Laíns <lains@riseup.net>
@FFY00 FFY00 requested a review from jpakkane as a code owner October 11, 2021 13:16
@FFY00
Copy link
Member Author

FFY00 commented Oct 11, 2021

On top of #9335 to prevent conflicts when that gets merged.

@FFY00
Copy link
Member Author

FFY00 commented Oct 11, 2021

@eli-schwartz can you have a look?

cc @rgommers @ev-br
This should fix ev-br/mc_lib#45

@codecov
Copy link

codecov bot commented Oct 11, 2021

Codecov Report

Merging #9388 (6a04f9c) into master (88d9646) will decrease coverage by 3.13%.
The diff coverage is n/a.

❗ Current head 6a04f9c differs from pull request most recent head 1add8a2. Consider uploading reports for the commit 1add8a2 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9388      +/-   ##
==========================================
- Coverage   67.29%   64.16%   -3.14%     
==========================================
  Files         396      198     -198     
  Lines       85410    42662   -42748     
  Branches    17481     8730    -8751     
==========================================
- Hits        57474    27372   -30102     
+ Misses      23246    12992   -10254     
+ Partials     4690     2298    -2392     
Impacted Files Coverage Δ
mesonlib/universal.py 75.76% <0.00%> (-0.09%) ⬇️
mesonbuild/backend/vs2017backend.py
mesonbuild/interpreter/primitives/integer.py
mesonbuild/templates/objctemplates.py
mesonbuild/interpreter/compiler.py
mesonbuild/compilers/detect.py
mesonbuild/_typing.py
mesonbuild/compilers/rust.py
mesonbuild/modules/keyval.py
mesonbuild/interpreterbase/_unholder.py
... and 189 more

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 88d9646...1add8a2. Read the comment docs.

@ev-br
Copy link

ev-br commented Oct 11, 2021

Confirm that it fixes ev-br/mc_lib#45.

The warning is still there,

WARNING: Broken python installation detected. Python files installed by Meson might not be found by python interpreter.
 This warning can be avoided by setting "python.platlibdir" option.
WARNING: Broken python installation detected. Python files installed by Meson might not be found by python interpreter.

but the installed files are in .../python3.8/site-packages.

@FFY00
Copy link
Member Author

FFY00 commented Oct 11, 2021

Since these are just heuristics, and might break, it makes sense to keep the warning.

@eli-schwartz
Copy link
Member

The heuristic is correctly reporting that this PR doesn't actually fix anything, because the files are still installed to a location that is not on sys.path for the detected python installation.

What's the goal here?

@FFY00
Copy link
Member Author

FFY00 commented Oct 13, 2021

sigh

@FFY00 FFY00 closed this Oct 13, 2021
@eli-schwartz
Copy link
Member

To elaborate: while it would be nice to teach the python module to install to the virtualenv location, it doesn't seem like this commit does that.

Instead, non-Debian systems are unaffected, and Debian systems revert back to installing to /usr/lib/python3.9/site-packages/.

Can you explain what workflow this commit is trying to unblock, and how it accomplishes it?

@FFY00
Copy link
Member Author

FFY00 commented Oct 14, 2021

That is not true.

root@ccbe2535e901:/# python3
Python 3.7.3 (default, Jan 22 2021, 20:04:44)
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> def debian_distutils_deb_system():
...     # The easiest way to see if the deb_system scheme is in use is by checking
...     # the value of get_python_lib.
...     # https://salsa.debian.org/cpython-team/python3/-/blob/python3.6/debian/patches/distutils-install-layout.diff#L140
...     import distutils.command.install
...     import distutils.sysconfig
...     return (
...         'deb_system' in distutils.command.install.INSTALL_SCHEMES
...         and distutils.sysconfig.get_python_lib().endswith('dist-packages')
...     )
...
>>> debian_distutils_deb_system()
True
>>> import sys
>>> sys.path
['', '/usr/lib/python37.zip', '/usr/lib/python3.7', '/usr/lib/python3.7/lib-dynload', '/usr/local/lib/python3.7/dist-packages', '/usr/lib/python3/dist-packages']
root@ccbe2535e901:/# python3 -m venv test
root@ccbe2535e901:/# test/bin/python
Python 3.7.3 (default, Jan 22 2021, 20:04:44)
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> def debian_distutils_deb_system():
...     # The easiest way to see if the deb_system scheme is in use is by checking
...     # the value of get_python_lib.
...     # https://salsa.debian.org/cpython-team/python3/-/blob/python3.6/debian/patches/distutils-install-layout.diff#L140
...     import distutils.command.install
...     import distutils.sysconfig
...     return (
...         'deb_system' in distutils.command.install.INSTALL_SCHEMES
...         and distutils.sysconfig.get_python_lib().endswith('dist-packages')
...     )
...
>>> debian_distutils_deb_system()
False
>>> import sys
>>> sys.path
['', '/usr/lib/python37.zip', '/usr/lib/python3.7', '/usr/lib/python3.7/lib-dynload', '/test/lib/python3.7/site-packages']

@eli-schwartz
Copy link
Member

But the python module will still install to /usr(/local)?/lib/python3.7/site-packages rather than /test/lib/python3.7/site-packages, right?

And, even with the PR, the warning message is still appearing which states that self.info['install_paths']['platlib'] doesn't match any location in sys.path. If that warning is appearing, it is supposed to mean that something is actually truly mismatched. So my assumption is the warning message means that there is a problem somewhere, not that it is a heuristic which should be ignored.

What am I missing here?

@FFY00
Copy link
Member Author

FFY00 commented Oct 14, 2021

No, it will not. You will just be much more likely to run into #9284, which is its own separate issue. Meson will mangle the installation path if it does not match the installation prefix, but that has nothing to with the introspection.

This PR makes the introspection script return the correct path. Currently, the introspection script will use the deb_system scheme, which is incorrect.
You are still required to set --prefix to the correct location so that Meson does not mangle the path, but that is much more correct than using the path from the deb_system scheme and putting the files in the wrong location altogether.

In the future please be more mindful with how you reply. Stating that the "PR doesn't actually fix anything" without any properly looking into the issue, and following that with "What's the goal here?", is not very constructive. Meson makes various wrong assumptions about Python and install paths. This is a complex issue, made even more complex by Meson's inherently incompatible approach with Python's install path design. I have been trying to make it slightly more correct/reliable, but it's a frustrating getting feedback that my small fixes are wrong when the actual issue is the whole approach Meson takes to installing Python modules. I don't know, maybe I am being overly sensitive but, well, that is the current reality of my mental health. This whole ordeal (bad downstream patching of Python locations) has already taken a pretty big toll, being the person that gets to go to the different parties and brings up a bunch of issues and tries to get them to have coherent understandings of how install locations in Python work is not very fun. Anyway, sorry for the small rant, just please don't dismiss things unless you are 100% sure, I am tired from trying to put out fires all over the place.

edit: To clarify, as my message was poorly written
It will not install to the venv path without the prefix being set, which is the whole issue. On top of that you may run into the issue I linked above.

@ev-br
Copy link

ev-br commented Oct 14, 2021

From a user perspective, this PR does fix a rather annoying usability inconvenience.

To elaborate: while it would be nice to teach the python module to install to the virtualenv location, it doesn't seem like this commit does that.

This might be, but that's not the point. What this PR allows is to have meson setup build --prefix=$PWD/install stable and independent on whether I'm in a virtualenv or not, debian or other system etc.

Basically, an expectation is that with an explicit --prefix , I'd expect to find the installed files at ./install/python3.9/site-packages.
The first part of the path, ./install, I'm managing myself (because --prefix), and I'm prepared to use PYTHONPATH, which is a standard way of dealing with non-standard locations.

Instead, non-Debian systems are unaffected, and Debian systems revert back to installing to /usr/lib/python3.9/site-packages/.

Which is exactly the point :-).

W.r.t. the broken python install warning, I think it's unhelpful (the python installation is not broken, it just needs a PYTHONPATH variable, and once I've used --prefix I know it will). But the warning is not a big deal (while python3/dist-packages is).

@rgommers
Copy link
Contributor

W.r.t. the broken python install warning, I think it's unhelpful

I agree - and it's also phrased oddly, to me "broken Python install" means "Python itself is broken somehow" (e.g., missing dev headers that are needed), not "your project may not work if installed". I'd be happy to submit a PR with a rephrase if that is welcome.

@eli-schwartz
Copy link
Member

Basically, an expectation is that with an explicit --prefix , I'd expect to find the installed files at ./install/python3.9/site-packages.
The first part of the path, ./install, I'm managing myself (because --prefix), and I'm prepared to use PYTHONPATH, which is a standard way of dealing with non-standard locations.

I disagree with this expectation and believe that if you "have to" use PYTHONPATH then the warning is correct and the problem is not (fully?) solved.

And the solution to finding the files in a stable location for adding to the PYTHONPATH, is to use -Dpython.platlibdir -Dpython.purelibdir as suggested (e.g. to $PWD/install/pythonpath). If you are using $PYTHONPATH anyway then you can statically code it rather than assuming anything, even the libdir containing a version-specific "python3.9" etc.

The solution to installing the files to the correct OOTB location will involve installing to the venv and completely ignoring the --prefix.

Long term it would be nice for e.g. ninja wheel to create wheels installable by pip install or python -m installer. According to pypa discussions this would also be the best way to handle .pth files (create an "editable wheel" that just has a .pth file) so it would probably make sense to create that as well. But this is all long term as a native build backend is still not hashed out. :(

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.

4 participants