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

[mgorny]

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.

[labath]

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} ?

[mgorny]

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]

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]

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]

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]

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.

[JDevlieghere]

+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]

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]

And thanks for this and it is definitely an improvement!

[mgorny]

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]

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]

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]

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]

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]

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)

Sorry first link is wrong. Debian patch:
https://salsa.debian.org/cpython-team/python3-stdlib/blob/master/debian/patches/3.6/distutils-install-layout.diff

[mgorny]

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]

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]

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]

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().

[labath]

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.

[mgorny]

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.

[ZeGentzy]

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

lldb/trunk/scripts/CMakeLists.txt

add_custom_command(
   COMMENT "Python script building LLDB Python wrapper")
 
 add_custom_target(swig_wrapper ALL DEPENDS
   ${CMAKE_CURRENT_BINARY_DIR}/LLDBWrapPython.cpp
   ${CMAKE_CURRENT_BINARY_DIR}/lldb.py
 )
 
 if(NOT LLDB_BUILD_FRAMEWORK)
-  if(CMAKE_SYSTEM_NAME MATCHES "Windows")
-    set(swig_python_subdir site-packages)
-  else()
-    set(swig_python_subdir python${PYTHON_VERSION_MAJOR}.${PYTHON_VERSION_MINOR})
-  endif()
-
-  set(SWIG_PYTHON_DIR ${LLVM_LIBRARY_OUTPUT_INTDIR}/${swig_python_subdir})
-  set(SWIG_INSTALL_DIR lib${LLVM_LIBDIR_SUFFIX})
+  execute_process(
+    COMMAND ${PYTHON_EXECUTABLE}
+        -c "import distutils.sysconfig, sys; print(distutils.sysconfig.get_python_lib(True, False, sys.argv[1]))"
+        ${CMAKE_BINARY_DIR}
+    OUTPUT_VARIABLE SWIG_PYTHON_DIR
+    OUTPUT_STRIP_TRAILING_WHITESPACE)
+  execute_process(
+    COMMAND ${PYTHON_EXECUTABLE}
+        -c "import distutils.sysconfig; print(distutils.sysconfig.get_python_lib(True, False, ''))"
+    OUTPUT_VARIABLE SWIG_INSTALL_DIR
+    OUTPUT_STRIP_TRAILING_WHITESPACE)
 
   # Install the LLDB python module
-  install(DIRECTORY ${SWIG_PYTHON_DIR} DESTINATION ${SWIG_INSTALL_DIR})
+  install(DIRECTORY ${SWIG_PYTHON_DIR}/ DESTINATION ${SWIG_INSTALL_DIR})
 endif()

lldb/trunk/scripts/get_relative_lib_dir.py

def get_python_relative_libdir():
 
     # We currently have a bug in lldb -P that does not account for
     # architecture variants in python paths for
     # architecture-specific modules.  Handle the lookup here.
     # When that bug is fixed, we should just ask lldb for the
     # right answer always.
     arch_specific_libdir = distutils.sysconfig.get_python_lib(True, False)
     split_libdir = arch_specific_libdir.split(os.sep)
-    lib_re = re.compile(r"^lib.+$")
+    lib_re = re.compile(r"^lib.*$")
 
     for i in range(len(split_libdir)):
         match = lib_re.match(split_libdir[i])
         if match is not None:
             # We'll call this the relative root of the lib dir.
             # Things like RHEL will have an arch-specific python
             # lib dir, which isn't 'lib' on x86_64.
             return os.sep.join(split_libdir[i:])