[Lldb-commits] [PATCH] D67890: [lldb] [cmake] Fix installing Python modules on systems using /usr/lib

[Haibo Huang via Phabricator via lldb-commits]

hhb added inline comments.


================
Comment at: lldb/scripts/CMakeLists.txt:45-50
+  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)
----------------
mgorny wrote:
> labath wrote:
> > mgorny wrote:
> > > labath wrote:
> > > > mgorny wrote:
> > > > > labath wrote:
> > > > > > 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}` ?
> > > > > 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.
> > > > 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.
> > > 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.
> > > 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.
> 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.
> 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.


================
Comment at: lldb/scripts/get_relative_lib_dir.py:26
     split_libdir = arch_specific_libdir.split(os.sep)
-    lib_re = re.compile(r"^lib.+$")
+    lib_re = re.compile(r"^lib.*$")
 
----------------
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? )


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67890/new/

https://reviews.llvm.org/D67890