Page MenuHomePhabricator

[lldb] [cmake] Fix installing Python modules on systems using /usr/lib
ClosedPublic

Authored by mgorny on Sep 22 2019, 8:36 AM.

Details

Summary

Fix installing Python modules on systems that use /usr/lib for Python
while installing other libraries in /usr/lib64. Rewrite CMake logic
to query correct directories from Python, similarly to how
prepare_binding_Python.py does it. Furthermore, change the regex used
in get_relative_lib_dir.py to allow 'lib' without suffix.

I think that the code can be further improved but I'd like to take
this enterprise in smaller steps in case one of them breaks something.

Diff Detail

Repository
rL LLVM

Event Timeline

mgorny created this revision.Sep 22 2019, 8:36 AM
labath accepted this revision.Sep 23 2019, 4:37 AM
labath added a subscriber: ZeGentzy.

This looks like a good idea to me, but given how fiddly this code seems to be, lets give some time for @hhb and @ZeGentzy to try this out.
The logical next step seems to be to pass this information into prepare_binding_Python.py to avoid recomputing the same thing twice. After that, we could try to support the cross-compile scenario by detecting these paths based on the target python (with the worst case being, making this a cache variable and having the user override in a cross-scenario).

lldb/scripts/CMakeLists.txt
45–50 ↗(On Diff #221218)

For my own education, is it possible that the result of the get_python_lib call will fundamentally differ depending on the value of the third argument. I.e., is there any case where ${SWIG_PYTHON_DIR} will be different from ${CMAKE_BINARY_DIR}/${SWIG_INSTALL_DIR} ?

This revision is now accepted and ready to land.Sep 23 2019, 4:37 AM
mgorny marked an inline comment as done.Sep 23 2019, 4:55 AM

I think it will be also nice to eliminate get_relative_lib_dir.py long term.

lldb/scripts/CMakeLists.txt
45–50 ↗(On Diff #221218)

The former will be an absolute path while the latter is just the relative path. But if you mean whether they could have a different tail: I don't think so, at least with CPython. PyPy had some nasty logic, so I'd have to check if we ever decided to support that.

labath added inline comments.Sep 23 2019, 6:08 AM
lldb/scripts/CMakeLists.txt
45–50 ↗(On Diff #221218)

Right now that doesn't matter, but I am thinking ahead about the cross-compilation case. If we turn out to need a cache variable for this, it would be nice if it would be a single variable that the user could set (instead of two of them). For that to work, we'd need to be able to compute the result of one of these calls using the value of the other one.

mgorny marked an inline comment as done.Sep 23 2019, 6:26 AM
mgorny added inline comments.
lldb/scripts/CMakeLists.txt
45–50 ↗(On Diff #221218)

I suppose you mean having a variable specifying path relative to prefix/build dir, I presume? I suppose we could build the whole thing around a cache var defaulting to the sysconfig call as proposed here for SWIG_INSTALL_DIR (note it's passing empty prefix to get_python_lib() in order to get relative path).

Thinking about it, maybe it'd indeed be better for me to prepare a more complete patch built around that, and focus on testing that instead. I was mostly worried/confused by Linux-specific code hanging around.

labath added inline comments.Sep 23 2019, 6:41 AM
lldb/scripts/CMakeLists.txt
45–50 ↗(On Diff #221218)

I suppose you mean having a variable specifying path relative to prefix/build dir, I presume?

Probably something like that, though I am still kind of hoping that there will be some way to detect this from cmake. However, that is looking less and less likely the more time I spend looking for a way to do it.

Thinking about it, maybe it'd indeed be better for me to prepare a more complete patch built around that, and focus on testing that instead.

That could work too, though I think the current version is an improvement in itself, and I agree we should do this slowly.

mgorny marked an inline comment as done.Sep 23 2019, 7:02 AM
mgorny added inline comments.
lldb/scripts/CMakeLists.txt
45–50 ↗(On Diff #221218)

I've grepped the FindPython modules and couldn't think anything useful. I suppose the external call to Python is as close as we can get. This is roughly what autotools is doing, except they go for full path and I tried to use relative path. The former is probably more correct (as Python won't look for the module in non-standard CMAKE_INSTALL_PREFIX) but I went for preserving existing behavior.

+1 on passing this information to the Python script. Personally I'd like to move more of that logic into CMake so we can properly track dependencies and don't have to have dummy targets that are always out of date. This would be a step in the right direction.

hhb added inline comments.Sep 23 2019, 10:15 AM
lldb/scripts/CMakeLists.txt
45–50 ↗(On Diff #221218)

I suppose you mean having a variable specifying path relative to prefix/build dir, I presume?

Sounds good to me and we do need cross compiling from Linux to Windows.

lldb/scripts/get_relative_lib_dir.py
26 ↗(On Diff #221218)

If we go this way, should we always use LLDB_PYTHON_RELATIVE_LIBDIR in ScriptInterpreterPython.cpp, and add some code to make sure it is defined? Because all assumption of the path can be wrong.

After the change here, I think POSIX will always use LLDB_PYTHON_RELATIVE_LIBDIR. But for windows, the path is still hard coded to lib/site-packages.

(maybe finishSwigPythonLLDB.py / make_symlink() can also be updated to use os.path.relpath? )

hhb added a comment.Sep 23 2019, 10:16 AM

And thanks for this and it is definitely an improvement!

mgorny marked an inline comment as done.
mgorny added inline comments.
lldb/scripts/get_relative_lib_dir.py
26 ↗(On Diff #221218)

Actually, I think we can kill all this logic by simply passing '' as prefix, as I did in the CMake part.

hhb added inline comments.Sep 23 2019, 10:26 AM
lldb/scripts/get_relative_lib_dir.py
26 ↗(On Diff #221218)

I'm not sure. On my machine:

$ python3
Python 3.6.8 (default, Jan 3 2019, 03:42:36)
[GCC 8.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
\>>> import distutils.sysconfig
\>>> distutils.sysconfig.get_python_lib(True, False)
'/usr/lib/python3/dist-packages'
\>>> distutils.sysconfig.get_python_lib(True, False, '')
'lib/python3/dist-packages'
\>>> distutils.sysconfig.get_python_lib(True, False, '/src/lib')
'/src/lib/lib/python3.6/site-packages'

mgorny marked an inline comment as done.Sep 23 2019, 10:31 AM
mgorny added inline comments.
lldb/scripts/get_relative_lib_dir.py
26 ↗(On Diff #221218)

Hm, that's interesting. The documentation says:

If 'prefix' is supplied, use it instead of sys.base_prefix or sys.base_exec_prefix -- i.e., ignore 'plat_specific'.

So apparently first arg being true is meaningless then. Maybe we should go for get_python_lib(False, False, '')?

hhb added inline comments.Sep 23 2019, 10:45 AM
lldb/scripts/get_relative_lib_dir.py
26 ↗(On Diff #221218)

Well I checked the code. plat_specific IS ignored. But it will test whether prefix "is default".

is_default_prefix = not prefix or os.path.normpath(prefix) in ('/usr', '/usr/local')

And do things differently based on that. Sigh..

mgorny marked an inline comment as done.Sep 23 2019, 10:54 AM
mgorny added inline comments.
lldb/scripts/get_relative_lib_dir.py
26 ↗(On Diff #221218)

I'm looking through the code of CPython and I don't see dist-packages anywhere. Is this some local distro patching or something?

What I'm really wondering is whether we need to split .so and .py modules. Technically distutils does that but it seems to use the same path for both on all platforms I see in INSTALL_SCHEMES.

hhb added inline comments.Sep 23 2019, 10:58 AM
lldb/scripts/get_relative_lib_dir.py
26 ↗(On Diff #221218)

It is an Debian patch.
https://bugs.launchpad.net/ubuntu/+source/python3-defaults/+bug/1814653

What make things worse, the path returned by get_python_lib() may not be in sys.path(). But I guess that's their issue.
https://bugs.launchpad.net/ubuntu/+source/python3-defaults/+bug/1814653

26 ↗(On Diff #221218)
mgorny marked an inline comment as done.Sep 23 2019, 12:00 PM
mgorny added inline comments.
lldb/scripts/get_relative_lib_dir.py
26 ↗(On Diff #221218)

Looking at it, it still uses the same location for .py and .so, just uses dist-packages instead of site-packages for stuff that's supposed to be installed by .deb packages. Do you expect LLDB to use dist-packages or site-packages by default? I think their intent is the former.

mgorny updated this revision to Diff 221388.Sep 23 2019, 12:35 PM
mgorny retitled this revision from [lldb] [cmake] Fix installing Python modules on systems using /usr/lib to [lldb] [cmake] Unify and correct Python module installation paths.
mgorny edited the summary of this revision. (Show Details)

O-kay, please try this patch instead. I think it should fix all the issues and avoid inconsistencies between various places this is used. However, I'm not sure if it doesn't break exotic platforms.

hhb added inline comments.Sep 23 2019, 2:42 PM
lldb/scripts/Python/prepare_binding_Python.py
284 ↗(On Diff #221388)

Just bail out in this situation? So that the error can be more obvious. E.g. someone changed cmake file but didn't know to update here.

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
326 ↗(On Diff #221388)

Also update the path for windows using LLDB_PYTHON_RELATIVE_LIBDIR?

hhb added inline comments.Sep 23 2019, 2:54 PM
lldb/scripts/Python/prepare_binding_Python.py
284 ↗(On Diff #221388)

finishSwigPythonLLDB.py also need to be updated similar to what you did here. Specially get_framework_python_dir() and make_symlink().

This is looking very good to me. Adding some windows folks to help with trying this out on that platform.

lldb/scripts/Python/prepare_binding_Python.py
284 ↗(On Diff #221388)

+1 for bailing out here.

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
326 ↗(On Diff #221388)

Possibly, though i think this could also be done in a separate patch.

sgraenitz resigned from this revision.Sep 24 2019, 6:02 AM
mgorny updated this revision to Diff 221555.Sep 24 2019, 10:49 AM
mgorny marked 8 inline comments as done.
mgorny retitled this revision from [lldb] [cmake] Unify and correct Python module installation paths to [lldb] [cmake] Fix installing Python modules on systems using /usr/lib.
mgorny edited the summary of this revision. (Show Details)

This turned out much harder than expected. Let's go with my original patch instead, and in the meantime I'll try to figure out how to properly use relative libdir everywhere.

hhb accepted this revision.Sep 24 2019, 8:16 PM
labath accepted this revision.Sep 25 2019, 1:53 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 25 2019, 2:49 AM

I'm a tad late, but I just wanted to confirm for everyone that this works. Thanks everybody!