Skip to content

[lldb] fix python extension debug suffix on Win #89037

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 24, 2024

Conversation

amordo
Copy link
Contributor

@amordo amordo commented Apr 17, 2024

ae389b2 change doesn't cover "_d" suffix for Debug build on Windows.

Fixed #87381.

@amordo amordo requested a review from JDevlieghere as a code owner April 17, 2024 08:39
@llvmbot llvmbot added the lldb label Apr 17, 2024
@llvmbot
Copy link
Member

llvmbot commented Apr 17, 2024

@llvm/pr-subscribers-lldb

Author: Alexander M. (amordo)

Changes

ae389b2 change doesn't cover "_d" suffix for Debug build on Windows.

Fixed #87381.


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

1 Files Affected:

  • (modified) lldb/bindings/python/CMakeLists.txt (+5-1)
diff --git a/lldb/bindings/python/CMakeLists.txt b/lldb/bindings/python/CMakeLists.txt
index 73b1239495e22e..183cee9ea64a06 100644
--- a/lldb/bindings/python/CMakeLists.txt
+++ b/lldb/bindings/python/CMakeLists.txt
@@ -136,7 +136,11 @@ function(finish_swig_python swig_target lldb_python_bindings_dir lldb_python_tar
   else()
     set(LIBLLDB_SYMLINK_DEST "${LLVM_SHLIB_OUTPUT_INTDIR}/liblldb${CMAKE_SHARED_LIBRARY_SUFFIX}")
   endif()
-  set(LIBLLDB_SYMLINK_OUTPUT_FILE "_lldb${LLDB_PYTHON_EXT_SUFFIX}")
+  if(WIN32 AND CMAKE_BUILD_TYPE STREQUAL Debug)
+    set(LIBLLDB_SYMLINK_OUTPUT_FILE "_lldb_d${LLDB_PYTHON_EXT_SUFFIX}")
+  else()
+    set(LIBLLDB_SYMLINK_OUTPUT_FILE "_lldb${LLDB_PYTHON_EXT_SUFFIX}")
+  endif()
   create_relative_symlink(${swig_target} ${LIBLLDB_SYMLINK_DEST}
                           ${lldb_python_target_dir} ${LIBLLDB_SYMLINK_OUTPUT_FILE})
 

@DavidSpickett
Copy link
Collaborator

Looks fine but can you just confirm what

elif args.variable_name == "LLDB_PYTHON_EXT_SUFFIX":
reports?

I assume this does not include _d, otherwise there would be no need for this fix?

@DavidSpickett
Copy link
Collaborator

The best docs I can find are https://peps.python.org/pep-0720/:
"Interpreter-specific Python extension (native module) suffix — generally defined as .{SOABI}.{SHLIB_SUFFIX}."

Which is less than helpful. https://peps.python.org/pep-3149/ mentions Windows uses a different format.

But anyway, just curious exactly what your interpreter returns.

@amordo
Copy link
Contributor Author

amordo commented Apr 17, 2024

Looks fine but can you just confirm what

elif args.variable_name == "LLDB_PYTHON_EXT_SUFFIX":

reports?
I assume this does not include _d, otherwise there would be no need for this fix?

C:\Users\iammorjj\llvm-project\lldb\bindings\python>python get-python-config.py LLDB_PYTHON_EXT_SUFFIX
.cp312-win_amd64.pyd

C:\Users\iammorjj\llvm-project\lldb\bindings\python>python_d get-python-config.py LLDB_PYTHON_EXT_SUFFIX
_d.cp312-win_amd64.pyd

Now I suppose the problem comes from

foreach(var LLDB_PYTHON_RELATIVE_PATH LLDB_PYTHON_EXE_RELATIVE_PATH LLDB_PYTHON_EXT_SUFFIX)
    if(NOT DEFINED ${var} AND NOT CMAKE_CROSSCOMPILING)
      execute_process(
        COMMAND ${Python3_EXECUTABLE}
          ${CMAKE_CURRENT_SOURCE_DIR}/bindings/python/get-python-config.py
          ${var}
        OUTPUT_VARIABLE value
        OUTPUT_STRIP_TRAILING_WHITESPACE)
      file(TO_CMAKE_PATH "${value}" value)
      set(${var} ${value} CACHE STRING ${cachestring_${var}})

where Python3_EXECUTABLE is Release python

@DavidSpickett
Copy link
Collaborator

DavidSpickett commented Apr 17, 2024

This PR fixes debug lldb -> release Python, but if I configured against a debug Python then the suffix would be _d_d.

Debug lldb/release Python is a valid config I think, and likely what 99% of people want anyway. So can we add a check whether the suffix already has _d in it, so that debug lldb/debug python also works?

@amordo amordo force-pushed the lldb-d-win-suffix-fix branch from f9aa019 to 1a2ae41 Compare April 19, 2024 15:33
@amordo
Copy link
Contributor Author

amordo commented Apr 19, 2024

Applied @DavidSpickett's remark

@amordo amordo force-pushed the lldb-d-win-suffix-fix branch from 1a2ae41 to afb03ec Compare April 24, 2024 13:47
ae389b2 relies on
LLDB_PYTHON_EXT_SUFFIX of Python3_EXECUTABLE for naming the result lldb
python lib.

Debug python is used in Debug LLDB on Windows, so the result lib name
requires "_d". LLDB_PYTHON_EXT_SUFFIX doesn't start with "_d" if
Python3_EXECUTABLE wasn't set to Debug executable explicitly. Perhaps a
better solution can be found after solving "debug python executable is
not currently handled" issue
(https://gitlab.kitware.com/cmake/cmake/-/issues/25874#note_1506658).
@amordo amordo force-pushed the lldb-d-win-suffix-fix branch from afb03ec to 3711ce6 Compare April 24, 2024 13:48
Copy link
Collaborator

@DavidSpickett DavidSpickett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I can merge this for you if you don't have commit access, otherwise go ahead and merge it yourself.

@amordo
Copy link
Contributor Author

amordo commented Apr 24, 2024

@DavidSpickett I have no write privilege, so, please, merge it:)

@DavidSpickett DavidSpickett merged commit e0adf63 into llvm:main Apr 24, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[lldb] tests errors ModuleNotFoundError: No module named '_lldb' with BuildType=Debug on Windows
3 participants