[PATCH] D127800: [llvm-driver] Generate symlinks instead of executables for tools

[Alex Brachet via Phabricator via llvm-commits]

abrachet added a comment.

Looks like I forgot to click send on these comments.

In D127800#3650787 <https://reviews.llvm.org/D127800#3650787>, @mgorny wrote:

> Since you're working on this again, could you please kindly fix the regressions you've introduced?
>
> https://reviews.llvm.org/D109977#3611683
>
> We can't build clang for over a month now.

I've responded there



================
Comment at: llvm/cmake/modules/AddLLVM.cmake:866
 
-macro(add_llvm_executable name)
+macro(setup_llvm_executable name)
   cmake_parse_arguments(ARG
----------------
phosek wrote:
> This name isn't particularly descriptive, how about `setup_llvm_driver`?
I don't think `setup_llvm_driver` makes sense because this is used mostly for `add_llvm_executable`.


================
Comment at: llvm/cmake/modules/AddLLVM.cmake:868-870
     "DISABLE_LLVM_LINK_LLVM_DYLIB;IGNORE_EXTERNALIZE_DEBUGINFO;NO_INSTALL_RPATH;SUPPORT_PLUGINS;GENERATE_DRIVER"
     "ENTITLEMENTS;BUNDLE_PATH"
     "DEPENDS"
----------------
phosek wrote:
> Most of these aren't being used in this function, could we prune this list and only keep the ones that are needed here?
Sure, that's a lot cleaner. The one hiccup I had was when this is called from `add_clang_tool`, `SUPPORT_PLUGINS` was getting propagated down, and there's no clean way (that I could think of) to eat that arg and not pass it to `setup_llvm_executable`. So I just parse it here even though it is unused. The alternative I could think of is to parse it in `add_clang_tool`.


================
Comment at: llvm/tools/llvm-driver/CMakeLists.txt:34
+  foreach(name IN LISTS LLVM_DRIVER_TOOLS LLVM_DRIVER_TOOL_SYMLINKS)
+    add_llvm_tool_symlink(${name} llvm-driver ALWAYS_GENERATE)
+    # Always generate install targets
----------------
phosek wrote:
> The issue you described with subdirectory is a known limitation of CMake when using post build commands.
> 
> Do you need `ALWAYS_GENERATE` here? Have you tried it without that option. That would remove the limitation and I //think// it should be sufficient.
> Do you need ALWAYS_GENERATE here? Have you tried it without that option.
It's been a while so I don't totally remember every reason why I landed here. Though I remember this taking most of my time on the patch. I did start without it because the name made me think it likely wasn't what I wanted but for a couple of reasons I ended up using it.

The first big problem is there is both a library and clang symlink named `clang-cpp`, which without `ALWAYS_GENERATE` caused cmake to fail. It's not clear why that library is even called `libclang-cpp` it was changed in D64278 with no context as to why. It seemed they just changed it without too much fuss so likely it isn't a deal breaker if we really wanted to. But certainly annoying.

Also, if you do `ninja llvm-cxxfilt` it will build `llvm` but doesn't create the `llvm-cxxfilt` symlink. With `ALWAYS_GENERATE` it creates the symlink as I would expect.

FWIW, `add_{clang,lld,flang}_symlink` also all opt to use `ALWAYS_GENERATE`, though I suspect the latter two just copied what clang did.

> That would remove the limitation and I think it should be sufficient.
I verified that you are right that I can get away without `generate_driver_tool_targets` in llvm/tools/CMakeLists.txt instead of hear if I omit the `ALWAYS_GENERATE`


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

https://reviews.llvm.org/D127800