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

Reply via email to