Page MenuHomePhabricator

[lldb] Refactor all POST_BUILD commands into targets
AcceptedPublic

Authored by hhb on Oct 29 2019, 3:31 PM.

Details

Summary

This makes all dependencies correct.

Event Timeline

hhb created this revision.Oct 29 2019, 3:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 29 2019, 3:31 PM
labath added a subscriber: labath.

This looks reasonable to me. I'm just wondering, now that these are separate targets, I guess that means they can be run in random order, right? Will that cause any problems, for instance when creating a package and its subpackage (formatters and formatters/cpp maybe)?

lldb/CMakeLists.txt
137

Would it be possible to just make the swig step place this file into the correct place directly?

hhb marked an inline comment as done.Oct 30 2019, 10:02 AM

This looks reasonable to me. I'm just wondering, now that these are separate targets, I guess that means they can be run in random order, right? Will that cause any problems, for instance when creating a package and its subpackage (formatters and formatters/cpp maybe)?

I try to avoid that problem by creating directory every time. Other than that I don't think there's any dependencies between two subpackages...

But in general, yes, that's something we should be careful of...

lldb/CMakeLists.txt
137

The difficulty is that, there is a config time path dependency like this:

swig_wrapper -> liblldb -> finish_swig (renamed to lldb_python_packages in this change)

Where liblldb uses the path of LLDBWrapPython.cpp in swig_wrapper. And here we reference the path of liblldb, to calculate the right path for python packages.

That means when swig_wrapper is created, we don't have a good way to figure out the right path for python packages and put the file there directly.

The best way to solve this is to hard code liblldb path for framework build. I.e. replace line 92 here:

get_target_property(liblldb_build_dir liblldb LIBRARY_OUTPUT_DIRECTORY)

With something more constant. This removes the liblldb -> finish_swig dependency. Then all the code here can be moved to scripts/CMakeLists.txt (I think they belong there better anyway). And swig_wrapper can get access to python package path.

The only question is whether we can / should "predict" liblldb path for framework... I'll look more into this. But it probably deserves a separate change.

hhb updated this revision to Diff 227204.Oct 30 2019, 5:10 PM

When creating symlink, make the target depends on relative target path. So that if the target file doesn't exist, the build will fail.

labath accepted this revision.Oct 31 2019, 2:00 AM

This looks great to me. Maybe wait a while to see if anyone else has any thoughts on this?

This not your fault, but the thing that bothers me about this build process (and which this patch makes visible) is how the locations of the python files in source bear absolutely no resemblance to the locations the files will end up installed. For instance, I would never have expected that something from the "examples" folder ends up in an actual shippable artifact. I think it would be great if we could rearrange the sources so that their source layout roughly matches the way in which they are going to be used...

lldb/CMakeLists.txt
137

Ok, that sounds fair. I think it would be great if this stuff could be moved to the scripts folder.

This revision is now accepted and ready to land.Oct 31 2019, 2:00 AM
aadsm requested changes to this revision.EditedNov 3 2019, 3:23 PM
aadsm added a subscriber: aadsm.

I'm sorry to reject but this diff breaks install-distribution on Darwin. Here's how to repro:

$ mkdir -p ~/Projects/llvm-build/Release && cd ~/Projects/llvm-build/Release
$ cmake -G Ninja \
 -DLLVM_ENABLE_PROJECTS="clang;lldb" \
 -DCMAKE_BUILD_TYPE=Release \
 -DLLVM_ENABLE_ASSERTIONS=OFF \
 -DLLDB_INCLUDE_TESTS=OFF \
 -DLLVM_DISTRIBUTION_COMPONENTS="lldb;liblldb" \
 -C ../../llvm-project/lldb/cmake/caches/Apple-lldb-macOS.cmake \
 -S../../llvm-project/llvm
$ DESTDIR=$PWD/install/usr ninja install-distribution
ninja: error: 'bin/LLDB.framework/LLDB', needed by 'bin/LLDB.framework/Resources/Python/lldb/_lldb.so', missing and no known rule to make it

The reason I found this is because I was having an issue where _lldb.so was being generated on the build but it was not being copied to the install directory.
After some mucking around I realized that the install-liblldb target installs the python scripts but it's lldb that generates the symlink as a POST_BUILD command (before this diff). I think in some situations the install-liblldb runs before that POST_BUILD command has run. At least I'm able to repro this quite consistently and once ninja install-distribution is finished _lldb.so exists on the build directory but not on the install one (and I can see all files in the python module being copied except that one in the ninja log when running the install step).

I fixed this locally by doing this (this fix doesn't really feel good but not sure what's the best way to do it):

if(LLDB_BUILD_FRAMEWORK)
  # When building as framework liblldb is responsible to install the python
  # scripts. However, we need to make sure that all swig components have been
  # properly finished.
  add_dependencies(install-liblldb finish_swig)
  add_dependencies(install-liblldb-stripped finish_swig)
else()
  # Install the LLDB python module
  ...
endif()

Then I found this diff that hasn't landed yet and tried it (I think there's a race condition caused by the POST_BUILD command but not sure) out and stumbled upon this.

This revision now requires changes to proceed.Nov 3 2019, 3:23 PM
mattd added a subscriber: mattd.Nov 4 2019, 9:13 AM
hhb added a comment.Nov 4 2019, 4:15 PM

I fixed this locally by doing this (this fix doesn't really feel good but not sure what's the best way to do it):

I found it confusing too. So finish_swig should depends on liblldb, because at least for windows, liblldb has to be exist before copying to python package path. And lldb correctly depends on both of them.

Then install-liblldb depends on liblldb. Which doesn't guarantee the build of finish_swig... Indeed your change may be the only way out...

This issue should be there before all recent changes. I'm not sure why it works before. Maybe by luck?

Do you want to upload a patch?

hhb added a comment.Nov 4 2019, 4:24 PM

Alternatively we should not depends on install-liblldb to install python packages, but have a install command similar to what we did below for other platforms.

hhb added a comment.Nov 4 2019, 5:01 PM

See D68370. We probably need to do the same thing for darwin...

hhb added a comment.Nov 4 2019, 5:06 PM

Sent D69834. Haven't get a chance to test but you can see the idea...

Also please add lldb-python-scripts to your LLVM_DISTRIBUTION_COMPONENTS when testing.

aadsm accepted this revision.Nov 4 2019, 6:52 PM

This is great! it's so much better. Thank so much for looking into this and fixing it.

This revision is now accepted and ready to land.Nov 4 2019, 6:52 PM
aadsm added a comment.Nov 4 2019, 10:51 PM

@hhb fwiw, I still get the same error with this diff (after applying it on top of D69834). But D69834 by itself works great!

hhb updated this revision to Diff 227958.Nov 5 2019, 1:35 PM

Rebase

hhb updated this revision to Diff 227974.Nov 5 2019, 3:31 PM

Fix some naming

hhb added a comment.Nov 5 2019, 3:37 PM

@hhb fwiw, I still get the same error with this diff (after applying it on top of D69834). But D69834 by itself works great!

Hmmm this change should not break it again... I rebased and cleaned up a little. Can you help try again?

aadsm added a comment.Nov 5 2019, 3:45 PM

@hhb I just did, and get the same error as well :(
ninja: error: 'bin/LLDB.framework/LLDB', needed by 'bin/LLDB.framework/Resources/Python/lldb/_lldb.so', missing and no known rule to make it

hhb added a comment.Nov 6 2019, 12:43 PM

@hhb I just did, and get the same error as well :(
ninja: error: 'bin/LLDB.framework/LLDB', needed by 'bin/LLDB.framework/Resources/Python/lldb/_lldb.so', missing and no known rule to make it

Thanks for the testing. It is interesting and I can reproduce now by build, remove LLDB.framework and build again.

LLDB.framework/LLDB symlink is not created by any target I can find. I tried liblldb or lldb. Seems only a full build will create it? I'm not familiar with framework build. Will spend some time to look into it.

aadsm added a comment.Nov 6 2019, 3:07 PM

Yeah, weird. I wonder if this is something cmake does implicitly when the target is defined as FRAMEWORK: https://cmake.org/cmake/help/latest/prop_tgt/FRAMEWORK.html

I'd recommend to be careful when making a change of this size. There are a lot of possible configurations.
If you split it up into a few smaller and more dedicated commits, you will make your life easier. Otherwise someone might just revert everything if this happens to break a bot.

Yeah, weird. I wonder if this is something cmake does implicitly when the target is defined as FRAMEWORK: https://cmake.org/cmake/help/latest/prop_tgt/FRAMEWORK.html

This is correct. There is a comment mentioning it here:
https://github.com/llvm/llvm-project/blob/276a5b2d5f1f/lldb/cmake/modules/LLDBFramework.cmake#L44

lldb/CMakeLists.txt
204

This change e.g. is reasonable, but unrelated. It could go into a separate NFC commit.

tatyana-krasnukha added inline comments.
lldb/CMakeLists.txt
114

This is quite off-topic, but it would be great if this function could handle whole directories as well as separate files. So adding a script will not require to change this file.

labath added inline comments.Dec 5 2019, 4:38 AM
lldb/CMakeLists.txt
114

It isn't really off topic, but I don't think it's really feasible, or a good idea even :P.

Think of this as the list of cpp files in the cmake add_library calls. Though there are ways to avoid listing the files there (via some sort of globbing, generally), they usually come with their own problems. Either the glob is evaluated at configuration time, and then you have to remember to re-run cmake to pick up the newly added files; or the glob is done at build time, which means that even a noop build needs to reglob everything...