[Lldb-commits] [PATCH] D55330: [CMake] Revised RPATH handling

Mon Dec 10 09:05:49 PST 2018

sgraenitz marked 2 inline comments as done.
sgraenitz added a comment.

In D55330#1323525 <https://reviews.llvm.org/D55330#1323525>, @xiaobai wrote:

> Looks good to me overall. You also probably probably also invoke `lldb_setup_rpaths_framework` for the tools included in the framework (argdumper, darwin-debug, debugserver, lldb-server).

Actually, these targets don't need the RPATHs. They are going into the framework bundle, yes, but they are linked entirely statically. So they don't need to find the framework at load time and don't need the RPATH, right?

================
Comment at: tools/driver/CMakeLists.txt:27
+if(LLDB_BUILD_FRAMEWORK)
+  lldb_setup_rpaths_framework(lldb)
+endif()
----------------
JDevlieghere wrote:
> Would it make sense/work to do the check inside this function?
Yes would be possible and it would save some lines of code, but this way it's more visible that it's a very special use-case and shouldn't be copy/pasted into each and every tool. For the general cases the default RPATHs from LLVM are exactly what is necessary. I have the impression that there was some confusing about RPATHs in this code before.

I thought about the alternative approach to query the dependencies for each target in `lldb_add_executable` and apply the RPATH settings to those targets that depend on liblldb (if `LLDB_BUILD_FRAMEWORK`). However, it feels like a bit too much magic under the hood and also it goes a little bit too far in the direction of building our own dependency management in CMake, which is not what CMake is made for.

I made the function name a little bit more explicit and extended the documentation. I would keep it like this now. What do you think?

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

https://reviews.llvm.org/D55330