Page MenuHomePhabricator

[CMake] Python bindings generation polishing
AcceptedPublic

Authored by sgraenitz on Wed, Dec 5, 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.

Event Timeline

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

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

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

Avoid conflicts: updating diff for recent changes on master

CMakeLists.txt
134–135

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
134–135

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.Mon, Dec 10, 8:08 AM

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

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

Address feedback and minor improvements

sgraenitz marked an inline comment as done.Mon, Dec 10, 8:49 AM
sgraenitz added inline comments.
CMakeLists.txt
134–135

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)Mon, Dec 10, 12:45 PM
sgraenitz marked an inline comment as done.Wed, Dec 12, 11:01 AM
sgraenitz added inline comments.
CMakeLists.txt
134–135

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

aprantl accepted this revision.Thu, Dec 13, 10:59 AM
This revision is now accepted and ready to land.Thu, Dec 13, 10:59 AM
JDevlieghere accepted this revision.Thu, Dec 13, 11:34 AM