This is an archive of the discontinued LLVM Phabricator instance.

[CMake] Python bindings generation polishing
ClosedPublic

Authored by sgraenitz on Dec 5 2018, 9:00 AM.

Details

Summary

Simplify SWIG invocation and handling of generated files.

The swig_wrapper target can generate LLDBWrapPython.cpp and lldb.py in its own binary directory, so we can get rid of a few global variables and their logic. We can use the swig_wrapper's BINARY_DIR target property to refer to it and liblldb's LIBRARY_OUTPUT_DIRECTORY to refer to the framework/shared object output directory.

Diff Detail

Repository
rLLDB LLDB

Event Timeline

sgraenitz created this revision.Dec 5 2018, 9:00 AM
sgraenitz edited the summary of this revision. (Show Details)Dec 5 2018, 9:02 AM
sgraenitz added a reviewer: xiaobai.
sgraenitz marked an inline comment as done.Dec 5 2018, 9:08 AM
sgraenitz added inline comments.
CMakeLists.txt
152

We depend on swig_wrapper which depends on it/generates it.

sgraenitz updated this revision to Diff 176957.Dec 6 2018, 5:16 AM

Avoid conflicts: updating diff for recent changes on master

CMakeLists.txt
137–138

I have a vague recollection that using TARGET_PROPERTY didn't work for some changes I was working on a few months ago, but I don't remember the details. It would be good to make sure that this works on mac, linux and windows all before you commit.

Thanks for taking a look!

It would be good to make sure that this works on mac, linux and windows all before you commit.

Shortly tested an in-tree lldb with clang, compiler-rt and lld on Windows using the below config and successfully built the BUILD_ALL project (Release), but I didn't manage to run check-lldb as Visual Studio failed to find the executable. How do you run the test suite on Windows?

cmake -G "Visual Studio 15 2017 Win64" -DLLVM_TARGETS_TO_BUILD=host -DPYTHON_HOME="%PYTHON_HOME%" ../llvm

This LGTM if it works on Windows.

CMakeLists.txt
137–138

IIRC and if we’re talking about the same thing, the generator expressions didn’t work because we were using them in a configured file (the lldb-dotest wrapper). Anyway, still worth double checking here.

sgraenitz updated this revision to Diff 177516.Dec 10 2018, 8:08 AM

Account for changes in parent revision to keep individual commits compiling.

sgraenitz updated this revision to Diff 177533.Dec 10 2018, 8:48 AM
sgraenitz marked 3 inline comments as done.

Address feedback and minor improvements

sgraenitz marked an inline comment as done.Dec 10 2018, 8:49 AM
sgraenitz added inline comments.
CMakeLists.txt
137–138

Yes, whenever we use a generator expression, there must be a processing step at generation time that expands the expression. Debugging can become hairy, because message($<GENEX:target>) doesn't have such a step (but the COMMAND in add_custom_target has one).

Anyway, I had another look and indeed the genex's are not necessary here. I would really like to keep the info in the target properties though, because that's where CMake needs it and we can avoid duplicating it to global variables.

I will double check that this works on the common platforms before I commit all my related changes.

sgraenitz edited the summary of this revision. (Show Details)Dec 10 2018, 12:45 PM
sgraenitz marked an inline comment as done.Dec 12 2018, 11:01 AM
sgraenitz added inline comments.
CMakeLists.txt
137–138

BTW removed generator expressions here and now use get_target_property to fetch property values at configuration time.

aprantl accepted this revision.Dec 13 2018, 10:59 AM
This revision is now accepted and ready to land.Dec 13 2018, 10:59 AM
JDevlieghere accepted this revision.Dec 13 2018, 11:34 AM
This revision was automatically updated to reflect the committed changes.
ted added a subscriber: ted.Feb 1 2019, 8:16 AM

@sgraenitz I've found an issue with this patch, using the Visual Studio 2015 generator.

In scripts/CMakeLists.txt the old code (for Windows):

if (CMAKE_CONFIGURATION_TYPES)
  set(SWIG_PYTHON_DIR ${CMAKE_BINARY_DIR}/\${CMAKE_INSTALL_CONFIG_NAME}/lib${LLVM_LIBDIR_SUFFIX}/site-packages)
else()
  set(SWIG_PYTHON_DIR ${CMAKE_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX}/site-packages)
endif()

Was changed to (for everything):

set(SWIG_PYTHON_DIR ${LLVM_LIBRARY_OUTPUT_INTDIR}/${swig_python_subdir})

Which changes <build>/tools/lldb/scripts/cmake_install.cmake from

if("${CMAKE_INSTALL_COMPONENT}" STREQUAL "Unspecified" OR NOT CMAKE_INSTALL_COMPONENT)
  file(INSTALL DESTINATION "${CMAKE_INSTALL_PREFIX}/lib" TYPE DIRECTORY FILES "i:/obj/${CMAKE_INSTALL_CONFIG_NAME}/lib/site-packages")
endif()

to

if("${CMAKE_INSTALL_COMPONENT}" STREQUAL "Unspecified" OR NOT CMAKE_INSTALL_COMPONENT)
  file(INSTALL DESTINATION "${CMAKE_INSTALL_PREFIX}/lib" TYPE DIRECTORY FILES "I:/obj/$(Configuration)/lib/site-packages")
endif()

"$(Configuration)" is a Visual Studio variable, and not available to cmake, so our bots error out with this error:

CMake Error at tools/lldb/scripts/cmake_install.cmake:31 (file):
  file INSTALL cannot find "I:/obj/$(Configuration)/lib/site-packages".
Call Stack (most recent call first):
  tools/lldb/cmake_install.cmake:41 (include)
  tools/cmake_install.cmake:41 (include)
  cmake_install.cmake:45 (include)

Verified with the latest CMake (3.13.3).

Will switching back to the old code for Windows cause any issues?

Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2019, 8:16 AM