This makes all dependencies correct.
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)?
Would it be possible to just make the swig step place this file into the correct place directly?
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...
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.
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...
Ok, that sounds fair. I think it would be great if this stuff could be moved to the scripts folder.
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.
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?
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.
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.
This is correct. There is a comment mentioning it here:
This change e.g. is reasonable, but unrelated. It could go into a separate NFC commit.
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.
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...
Please include me as a reviewer when making changes like this to the build system, especially if it affects the framework build. It's only sparsely tested upstream (in part because we have a bunch of downstream changes) but it is the way we ship LLDB to our customers so it is really import to us. If I happen to miss the review I likely won't discover issues until the next time we rebranch at which point it might be a lot harder to figure them out.