This is an archive of the discontinued LLVM Phabricator instance.

[CMake] Stabilize install process for LLDB.framework
AbandonedPublic

Authored by sgraenitz on May 15 2019, 9:32 AM.

Details

Summary

We got used to post-build commands for copying resources into the build-tree framework. Executables were included by adding their target names to the list of LLDB_FRAMEWORK_TOOLS. Install destinations were set in add_lldb_tool(<target-name> ...).

This patch unifies these steps in lldb_add_to_framework() which:

  • adds a copy to the build-tree framework for testing
  • adds a cleanup step to run before install
  • sets the target's destination in the install-tree

Key changes:

  • LLDB_BUILD_FRAMEWORK now disables the default GENERATE_INSTALL logic
  • LLDB_FRAMEWORK_TOOLS is obsolete; instead each tool calls lldb_add_to_framework() individually
  • lldb_setup_framework_rpaths_in_tool() is obsolete; instead targets set them individually using lldb_setup_rpaths (old function will be removed in a separate commit to keep the diff readable)
  • lldb-framework gets built by default
  • the build-tree copy operation is a POST_BUILD command for the individual targets (instead of the lldb-framework target)
  • while configuring the build, lldb_framework_cleanup_list_raw.txt collects <command> <expr> pairs to "strip" from the framework before install
  • eventually, the final list of <command> <path> pairs is stored in lldb_framework_cleanup_list_paths.txt
  • first action for install runs all the cleanup commands (see LLDBFrameworkInstall.cmake)

Test with monorepo:

$ cmake -GNinja -DCMAKE_INSTALL_PREFIX=/path/to/install/Developer/usr \
        -DLLDB_FRAMEWORK_INSTALL_DIR=/path/to/install/SharedFrameworks \
        -DLLVM_TARGETS_TO_BUILD="X86;ARM;AArch64" -DLLVM_INSTALL_TOOLCHAIN_ONLY=ON \
        -DLLVM_ENABLE_PROJECTS="clang;libcxx;libcxxabi;lldb" \
        -DLLDB_BUILD_FRAMEWORK=ON -DLLDB_NO_INSTALL_DEFAULT_RPATH=ON ../llvm-project/llvm
$ ninja check-lldb
$ ninja install

More background:

A framework is represented by the CMake target for the shared library it ships. Installing the target copies the entire bundle from the build-tree to the install-tree. This is mostly good, because we can add additional resource files to the build-tree framework bundle and install will include them automatically. We need the additional resources in the build-tree framework to facilitate testing.

Apart from simple files, however, LLDB.framework ships executable resources that define their own install targets (for extra processing at install-time like RPATH substitution or stripping). Apparently, CMake does not provide a way to run these install targets in the course of the framework install target. Instead they run either before or after framework install. The former is problematic, because the framework install will overwrite correctly processed resource executables with their build-tree pendants. While an install-order oriented fix may sound suitable at first appearance, we should keep in mind that, formally, CMake defines an order only locally:

[install] This command generates installation rules for a project. Rules specified by calls to this command within a source directory are executed in order during installation. The order across directories is not defined.

Event Timeline

sgraenitz created this revision.May 15 2019, 9:32 AM
sgraenitz edited the summary of this revision. (Show Details)May 15 2019, 10:43 AM

How does this cleanup affect dependency tracking? Does the build dir become unusable after running ninja install?

I'm not very familiar with frameworks and complexities involved in creating them, but the fact that we need to delete stuff from the build tree in order to install properly makes me think that there is something fishy going on. Is there no way to arrange things so that this can be avoided? For instance, what if we set the build output paths to be separate and disjoint locations for each target. Then use a separate target, or some POST_BUILD commands to copy/symlink the files to construct an build-tree framework, and have the install targets create the install-tree framework from the original build output paths?

I'm not very familiar with frameworks and complexities involved in creating them, but the fact that we need to delete stuff from the build tree in order to install properly makes me think that there is something fishy going on. Is there no way to arrange things so that this can be avoided?

Well, maybe there are better ways to do it, but we have a number of restrictions.

For instance, what if we set the build output paths to be separate and disjoint locations for each target. Then use a separate target, or some POST_BUILD commands to copy/symlink the files to construct an build-tree framework, and have the install targets create the install-tree framework from the original build output paths?

Ok, so you mean copying the entire framework to a staging directory, something like a "test-tree". Then we add test resources and run the test suite there. Sounds reasonable, I will investigate.
Certainly we had to consider ordering issues too, because all external headers and libraries must be copied in first, before copying to the test-tree.

How does this cleanup affect dependency tracking? Does the build dir become unusable after running ninja install?

Good point. Ideally we'd just copy them in again before running the test suite. Unlike I expected, this doesn't work at the moment. Maybe I should get this done first:

$ cmake -GNinja ...
$ ninja check-lldb
$ ninja install
$ ninja check-lldb

Which other test suite invocation(s) should I verify?

Actually, why not make the copy operations PRE_BUILD actions of the test suite instead of POST_BUILD actions of their executables?

Actually, why not make the copy operations PRE_BUILD actions of the test suite instead of POST_BUILD actions of their executables?

Because one might want to run the lldb executable independently of the test suite?

I thought about one more option, but I don't think it's better than the current proposal: instead of deleting the build-tree resources in the very first install step, I could install the framework manually at this point and skip its regular install target. We would loose implicit stripping and RPATH replacement for the dylib though. Should be possible to do it manually, but if it breaks, we won't notice. So, it's not quite appealing.

Because one might want to run the lldb executable independently of the test suite?

Agreed, it would be inconvenient to have non-functional build results. The solution to the "install -> test" issue would be having both I guess.
Adding this to the current patch still sounds like the most convenient approach to me.

For instance, what if we set the build output paths to be separate and disjoint locations for each target. Then use a separate target, or some POST_BUILD commands to copy/symlink the files to construct an build-tree framework, and have the install targets create the install-tree framework from the original build output paths?

Ok, so you mean copying the entire framework to a staging directory, something like a "test-tree". Then we add test resources and run the test suite there. Sounds reasonable, I will investigate.

In fact, your above comment is more relevant here. I'd have to set all build-tree RPATHs to the staging framework instead of the actual target output. Sounds like a number of non-intuitive edits all over the CMake scripts..

Yet another option: we could install the framework tools to a fake location and copy them to their final destination (overwriting their build-tree pendants) in a manual install step at the very end of the install process. The problem here is, that there may be more things in the build-tree framework than we overwrite, and thus remain in the install-tree (thinking about Swift resources in swift-lldb for example).

Yeah, neither of the options look really appealing... Still, I can't escape the feeling that we are doing something wrong, as the installation process should not be that complicated. Or is the framework support in cmake just not good enough?

Given that Alex (I believe) is the only other major user of the framework build, I'll let you two figure out what's the best way forward here...

The problem here is, that there may be more things in the build-tree framework than we overwrite, and thus remain in the install-tree (thinking about Swift resources in swift-lldb for example).

Why are the swift resources (whatever they are) in the build-tree framework if they should not end up being installed?

Sure, ideally CMake defined a global install order and this order would handle overlap. I think that's unlikely to happen. It took quite some time to find and fix an overlap case downstream and I think it's worth avoiding it in general in the future. OTOH I see that the solution shouldn't be too intrusive. For the swift resources: I am not familiar with the details; all I know is that tests fail if they are missing.

How does this cleanup affect dependency tracking? Does the build dir become unusable after running ninja install?

In D61952#1504942, @sgraenitz wrote:
Actually, why not make the copy operations PRE_BUILD actions of the test suite instead of POST_BUILD actions of their executables?

In D61952#1505019, @sgraenitz wrote:
The solution to the "install -> test" issue would be having both I guess.

TBH, I don't know how to accomplish this in the current state of the build system. I went through various options and didn't find a functioning one that works without, basically, turning everything upside down. We have similar functionality in lldb-framework-headers, but the appraoch only works in the directory where lldb-framework was defined. Furthermore we cannot rely on the existence of lldb-framework as lldb_add_to_framework() may be called before /lldb/source/API is processed (e.g. everything else in /lldb/source).

On one hand, the main use-case works: if the test suite succeeds then run install. And it sounds "kind of" acceptable that it fails the other way around.
On the other hand, seeing the amount of extra effort, "workaround" may be a better term to describe this change then "stabilize".

At the moment I agree, that neither of the options look really appealing. Good point to rethink the approach and find something more solid. If that fails, I might re-evaluate this one.

@xiaobai Out of interest: have you faced overwrite issues when running install and would this patch help?

@xiaobai Out of interest: have you faced overwrite issues when running install and would this patch help?

It's been a while since I've looked at in detail but I don't remember having any issues. It's possible that I don't remember or things have changed while I didn't notice. I'll play with my build and try it out with your patch applied, and then report back.

Ok thanks. I will be OOO next week, so no hurries.

sgraenitz abandoned this revision.May 26 2019, 11:37 PM

Closing this in favor of the reduced proposal