Push down cpython module to the submodule

[dingxiangfei2009]

The fact that it is trying to load from the root Python module constitute a re-import of the lldb module that is still under bootstrapping, whenever lldb is loaded directly from Python. Pushing the CPython module down into the native submodule allows it to avoid the cyclic import error.

Related to #70453

[llvmbot]

@llvm/pr-subscribers-lldb

Author: wieDasDing (dingxiangfei2009)

Changes

The fact that it is trying to load from the root Python module constitute a re-import of the lldb module that is still under bootstrapping, whenever lldb is loaded directly from Python. Pushing the CPython module down into the native submodule allows it to avoid the cyclic import error.

Related to #70453

---

Full diff: https://github.com/llvm/llvm-project/pull/113066.diff

3 Files Affected:

• (modified) lldb/bindings/python/CMakeLists.txt (+4-2)
• (modified) lldb/bindings/python/python.swig (+1-1)
• (modified) lldb/source/API/CMakeLists.txt (+1-1)

diff --git a/lldb/bindings/python/CMakeLists.txt b/lldb/bindings/python/CMakeLists.txt
index 69306a384e0b1c..bd51fdc68900e7 100644
--- a/lldb/bindings/python/CMakeLists.txt
+++ b/lldb/bindings/python/CMakeLists.txt
@@ -60,7 +60,7 @@ function(finish_swig_python swig_target lldb_python_bindings_dir lldb_python_tar
   # Add a Post-Build Event to copy over Python files and create the symlink to
   # liblldb.so for the Python API(hardlink on Windows).
   add_custom_target(${swig_target} ALL VERBATIM
-    COMMAND ${CMAKE_COMMAND} -E make_directory ${lldb_python_target_dir}
+    COMMAND ${CMAKE_COMMAND} -E make_directory ${lldb_python_target_dir}/native/
     DEPENDS ${lldb_python_bindings_dir}/lldb.py
     COMMENT "Python script sym-linking LLDB Python API")
 
@@ -74,6 +74,8 @@ function(finish_swig_python swig_target lldb_python_bindings_dir lldb_python_tar
       "${LLDB_SOURCE_DIR}/source/Interpreter/embedded_interpreter.py"
       "${lldb_python_target_dir}")
 
+  create_python_package(${swig_target} ${lldb_python_target_dir} "native" FILES)
+
   # Distribute the examples as python packages.
   create_python_package(
     ${swig_target}
@@ -141,7 +143,7 @@ function(finish_swig_python swig_target lldb_python_bindings_dir lldb_python_tar
   endif()
   set(LIBLLDB_SYMLINK_OUTPUT_FILE "_lldb${LLDB_PYTHON_EXT_SUFFIX}")
   create_relative_symlink(${swig_target} ${LIBLLDB_SYMLINK_DEST}
-                          ${lldb_python_target_dir} ${LIBLLDB_SYMLINK_OUTPUT_FILE})
+                          ${lldb_python_target_dir}/native/ ${LIBLLDB_SYMLINK_OUTPUT_FILE})
 
 
   if (NOT WIN32)
diff --git a/lldb/bindings/python/python.swig b/lldb/bindings/python/python.swig
index 278c0eed2bab27..23bbe518ad99f2 100644
--- a/lldb/bindings/python/python.swig
+++ b/lldb/bindings/python/python.swig
@@ -50,7 +50,7 @@ Older swig versions will simply ignore this setting.
     import $module
 except ImportError:
     # Relative import should work if we are being loaded by Python.
-    from . import $module"
+    from .native import $module"
 %enddef
 
 // The name of the module to be created.
diff --git a/lldb/source/API/CMakeLists.txt b/lldb/source/API/CMakeLists.txt
index a32bc58507d8eb..1fe57705cd813f 100644
--- a/lldb/source/API/CMakeLists.txt
+++ b/lldb/source/API/CMakeLists.txt
@@ -145,7 +145,7 @@ add_lldb_library(liblldb SHARED ${option_framework}
   ${option_install_prefix}
 )
 
-# lib/pythonX.Y/dist-packages/lldb/_lldb.so is a symlink to lib/liblldb.so,
+# lib/pythonX.Y/site-packages/lldb/_lldb.so is a symlink to lib/liblldb.so,
 # which depends on lib/libLLVM*.so (BUILD_SHARED_LIBS) or lib/libLLVM-10git.so
 # (LLVM_LINK_LLVM_DYLIB). Add an additional rpath $ORIGIN/../../../../lib so
 # that _lldb.so can be loaded from Python.

[JDevlieghere]

Can you reference documentation about the .native import and explain why this is necessary? Intuitively it makes sense but I want to make sure I fully understand this change and having it documented will help the next person that sees this.

[JDevlieghere]

This isn't related to this change, right? This was already the case previously?

[dingxiangfei2009]

No it is not. I will revert it.

I looked it up and it turns out that the dist- qualification is a Debian rule. I will leave this alone for discussion on another day.

[dingxiangfei2009]

Thank you for your review @JDevlieghere.

I think there is still issue with this patch. Until I sort out the root cause, I will close this PR first to reduce review load. Have a nice day!