[Lldb-commits] [PATCH] D74727: Allow customized relative PYTHONHOME

[Pavel Labath via Phabricator via lldb-commits]

labath added a reviewer: ted.
labath added a comment.

+ ted, who I believe has a stake in the relocatable python game

Your use case seems reasonable, but I find it hard to understand the meanings of the options in this patch. IIUC, there are now two ways to create an lldb with a "relocatable" python

1. set `LLDB_RELOCATABLE_PYTHON` to `ON`, and set the `PYTHONHOME` environment variable before launching lldb
2. set `LLDB_RELOCATABLE_PYTHON` to `OFF`, and also set `LLDB_PYTHON_HOME` to a relative path

It took me like five minutes to understand how setting `LLDB_RELOCATABLE_PYTHON=OFF` can help you ship python side-by-side with lldb (i.e. make it position indepent/relocatable). I think the main cause of the confusion is that the name `LLDB_RELOCATABLE_PYTHON` does not express very well what that option does -- it really should be something like `LLDB_EMBED_PYTHON_HOME`. Can we maybe rename it to something like that? Or maybe even just a single LLDB_PYTHON_HOME variable, with the empty value meaning we use the default python mechanism (PYTHONHOME env var, or the baked-in python default)?

The other thing which bugs me is that the relative python path chosen here will likely only be correct once lldb is installed. Will it be possible to run lldb (and its test suite) configured in this way directly from the build tree? I don't know if there's anything we can or should do about that, but this situation seems less than ideal...



================
Comment at: lldb/cmake/modules/LLDBConfig.cmake:68
+
+option(LLDB_RELOCATABLE_PYTHON "Use the PYTHONHOME environment variable to locate Python." {default_relocatable_python})
 option(LLDB_USE_SYSTEM_SIX "Use six.py shipped with system and do not install a copy of it" OFF)
----------------
missing `$` in {default_relocatable_python}


================
Comment at: lldb/cmake/modules/LLDBConfig.cmake:153
+    set(LLDB_PYTHON_HOME "${PYTHON_HOME}" CACHE STRING
+      "Path to use as PYTHONHONE in lldb. If a relative path is specified, \
+      it will be resolved at runtime relative to liblldb directory.")
----------------
typo PYTHONHONE


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp:298
+        llvm::sys::path::append(path, lldb_python_home);
+        llvm::sys::path::remove_dots(path, /* remove_dot_dots = */ true);
+        absolute_python_home = path.c_str();
----------------
Is the `remove_dot_dots` really necessary? It can produce unexpected results when `..`s back up over symlinks...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74727