[Lldb-commits] [PATCH] D61952: [CMake] Stabilize install process for LLDB.framework

[Stefan Gränitz via Phabricator via lldb-commits]

sgraenitz abandoned this revision.
sgraenitz added a comment.

Closing this in favor of the reduced proposal


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61952/new/

https://reviews.llvm.org/D61952



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D61952: [CMake] Stabilize install process for LLDB.framework

[Stefan Gränitz via Phabricator via lldb-commits]

sgraenitz added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61952/new/

https://reviews.llvm.org/D61952



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D61952: [CMake] Stabilize install process for LLDB.framework

[Alex Langford via Phabricator via lldb-commits]

xiaobai added a comment.

In D61952#1506608 , @sgraenitz wrote:

> @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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61952/new/

https://reviews.llvm.org/D61952



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D61952: [CMake] Stabilize install process for LLDB.framework

[Stefan Gränitz via Phabricator via lldb-commits]

sgraenitz added a comment.

>> In D61952#1503551 , @JDevlieghere 
>> wrote:
>> 
>>> 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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61952/new/

https://reviews.llvm.org/D61952



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D61952: [CMake] Stabilize install process for LLDB.framework

[Stefan Gränitz via Phabricator via lldb-commits]

sgraenitz added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61952/new/

https://reviews.llvm.org/D61952



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D61952: [CMake] Stabilize install process for LLDB.framework

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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...

In D61952#1505027 , @sgraenitz wrote:

> 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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61952/new/

https://reviews.llvm.org/D61952



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D61952: [CMake] Stabilize install process for LLDB.framework

[Stefan Gränitz via Phabricator via lldb-commits]

sgraenitz added a comment.

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).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61952/new/

https://reviews.llvm.org/D61952



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D61952: [CMake] Stabilize install process for LLDB.framework

[Stefan Gränitz via Phabricator via lldb-commits]

sgraenitz added a comment.

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.

In D61952#1504991 , @labath wrote:

> 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.**

In D61952#1504941 , @sgraenitz wrote:

> In D61952#1504346 , @labath wrote:
>
> > 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..


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61952/new/

https://reviews.llvm.org/D61952



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D61952: [CMake] Stabilize install process for LLDB.framework

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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?


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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61952/new/

https://reviews.llvm.org/D61952



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D61952: [CMake] Stabilize install process for LLDB.framework

[Stefan Gränitz via Phabricator via lldb-commits]

sgraenitz added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61952/new/

https://reviews.llvm.org/D61952



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D61952: [CMake] Stabilize install process for LLDB.framework

[Stefan Gränitz via Phabricator via lldb-commits]

sgraenitz added a comment.

In D61952#1504346 , @labath wrote:

> 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.

In D61952#1503551 , @JDevlieghere 
wrote:

> 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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61952/new/

https://reviews.llvm.org/D61952



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D61952: [CMake] Stabilize install process for LLDB.framework

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61952/new/

https://reviews.llvm.org/D61952



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D61952: [CMake] Stabilize install process for LLDB.framework

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61952/new/

https://reviews.llvm.org/D61952



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D61952: [CMake] Stabilize install process for LLDB.framework

[Stefan Gränitz via Phabricator via lldb-commits]

sgraenitz created this revision.
sgraenitz added reviewers: xiaobai, compnerd, JDevlieghere, labath.
Herald added subscribers: kristof.beyls, javed.absar, mgorny, ki.stfu.
Herald added a project: LLDB.

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( ...)`.

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 
` ` pairs to "strip" from the framework before install
- eventually, the final list of ` ` 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=install/Developer/usr \
  -DLLDB_FRAMEWORK_INSTALL_DIR=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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D61952

Files:
  lldb/CMakeLists.txt
  lldb/cmake/modules/AddLLDB.cmake
  lldb/cmake/modules/LLDBConfig.cmake
  lldb/cmake/modules/LLDBFramework.cmake
  lldb/cmake/modules/LLDBFrameworkInstall.cmake
  lldb/tools/argdumper/CMakeLists.txt
  lldb/tools/darwin-debug/CMakeLists.txt
  lldb/tools/debugserver/source/CMakeLists.txt
  lldb/tools/driver/CMakeLists.txt
  lldb/tools/lldb-mi/CMakeLists.txt
  lldb/tools/lldb-server/CMakeLists.txt
  lldb/tools/lldb-vscode/CMakeLists.txt

Index: lldb/tools/lldb-vscode/CMakeLists.txt
===
--- lldb/tools/lldb-vscode/CMakeLists.txt
+++ lldb/tools/lldb-vscode/CMakeLists.txt
@@ -29,7 +29,3 @@
   LINK_COMPONENTS
 Support
   )
-
-if(LLDB_BUILD_FRAMEWORK)
-  lldb_setup_framework_rpaths_in_tool(lldb-vscode)
-endif()
Index: lldb/tools/lldb-server/CMakeLists.txt
===
--- lldb/tools/lldb-server/CMakeLists.txt
+++ lldb/tools/lldb-server/CMakeLists.txt
@@ -77,3 +77,7 @@
 )
 
 target_link_libraries(lldb-server PRIVATE ${LLDB_SYSTEM_LIBS})
+
+if(LLDB_BUILD_FRAMEWORK)
+  lldb_add_to_framework(lldb-server)
+endif()
Index: lldb/tools/lldb-mi/CMakeLists.txt
===
--- lldb/tools/lldb-mi/CMakeLists.txt
+++ lldb/tools/lldb-mi/CMakeLists.txt
@@ -93,7 +93,3 @@
   LINK_COMPONENTS
 Support
   )
-
-if(LLDB_BUILD_FRAMEWORK)
-  lldb_setup_framework_rpaths_in_tool(lldb-mi)
-endif()
Index: lldb/tools/driver/CMakeLists.txt
==