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

[Petr Hosek via Phabricator via llvm-commits]

phosek accepted this revision.
phosek added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/cmake/modules/AddLLVM.cmake:866
 
-macro(add_llvm_executable name)
+macro(setup_llvm_executable name)
   cmake_parse_arguments(ARG
----------------
abrachet wrote:
> 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`.
The part I don't like is that it's not clear from the name what's the relationship between `add_llvm_executable` and `setup_llvm_executable` and when should I call which one (without knowing the details, I'd assume that I have to first `add` before I can `setup`). Maybe `generate_llvm_objects` would be better? This is very minor though.


================
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"
----------------
abrachet wrote:
> 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`.
You could use https://cmake.org/cmake/help/v3.13/command/list.html#filter to filter it out of `${ARGN}` in `add_clang_tool` before passing that to this macro but I'm not sure if that's cleaner.


================
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
----------------
abrachet wrote:
> 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`
> 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.

Thanks for the explanation, I think this alone is a reason to keep `ALWAYS_GENERATE` here.


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

https://reviews.llvm.org/D127800