[PATCH] D77110: [CMAKE] Provide default location for llvm-lit for out-of-tree users.

Tue Mar 31 14:17:38 PDT 2020

stephenneuendorffer marked an inline comment as done.
stephenneuendorffer added a comment.

In D77110#1953285 <https://reviews.llvm.org/D77110#1953285>, @tstellar wrote:

> How will LLVM_DEFAULT_EXTERNAL_LIT be used?  Will other sub-projects be udpated to use this instead of the existing heuristics?

LLVM_DEFAULT_EXTERNAL_LIT is an existing mechanism that can be used to point to llvm-lit.   I'd like to see everything use the same mechanism.
This is either going to require a) verifying that this works in other environments (windows?) and updating users to use it, removing usage of llvm-config
in the process, or b) taking one of the existing paths, removing usage of llvm-config and extracting it into something in llvm/cmake/modules
to eliminate the copy-pasta and then rebuilding around that.  Either of these paths is going to require some input and investment from people who
understand more about the global picture here:  I don't have the bandwidth or understanding of the various use cases to take this all on by myself.

================
Comment at: llvm/cmake/modules/CMakeLists.txt:112-116
+# Generate a default location for lit
+if (LLVM_INSTALL_UTILS AND LLVM_BUILD_UTILS)
+  set(LLVM_CONFIG_DEFAULT_EXTERNAL_LIT "${LLVM_CONFIG_TOOLS_BINARY_DIR}/llvm-lit")
+endif()
+
----------------
tstellar wrote:
> Why do we need to set this again?
There are basically 2 copies of the configuration information generated here.  One is describing the build area (allowing one to do an external build with LLVM_DIR pointing at build area), and one which describes an install area.  These are subtly different. Here the difference is that if llvm-lit is not installed (LLVM_INSTALL_UTILS=off) then it won't exist in the build area and we don't set a default.  Users would still be able to specify LLVM_EXTERNAL_LIT to point to any lit that they care to. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77110