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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits