[PATCH] D138258: clang/cmake: Fix incorrectly disabling tests when LLVM_EXTERNAL_LIT is used

[Konrad Wilhelm Kleine via Phabricator via cfe-commits]

kwk requested changes to this revision.
kwk added a comment.
This revision now requires changes to proceed.

As much as I would like this to be fixed. I vote against this patch because in `lld/CMakeLists.txt` there's an almost (if not entirely) identical piece of code <https://github.com/llvm/llvm-project/blob/fcd5098a03dadcd11d4cc8b7155a4c07581999de/lld/CMakeLists.txt#L62> that screams to be outsourced into a `/cmake/Modules/FindLit.cmake` (to be created). I'll have a look at this and see if I can come up with a patch for this. Afterall `/cmake` is the central place to distribute share CMake code between subprojects, right @phosek (didn't you create `/cmake` in the first place?

Alternatively, we can copy the code from `lld/CMakeLists.txt` and outsource it afterwards so that the shared code becomes more obvious.

If there're any objections regarding outsourcing to `FindLit.cmake`, then I would only ask to copy the code from `lld/CMakeLists.txt`. There the `find_program` part sits below the if block.



================
Comment at: clang/CMakeLists.txt:97
     # Seek installed Lit.
-    find_program(LLVM_LIT
-                 NAMES llvm-lit lit.py lit
-                 PATHS "${LLVM_MAIN_SRC_DIR}/utils/lit"
-                 DOC "Path to lit.py")
+    if (NOT LLVM_EXTERNAL_LIT)
+      find_program(LLVM_EXTERNAL_LIT
----------------
mgorny wrote:
> I don't think you need to do this if you rename the var, `find_program()` doesn't seem to do any searching when the var is already set.
> 
> I've tested with a dummy CMakeLists:
> 
> ```
> set(FOO /bin/true)
> find_program(FOO NAMES foo)
> message(FATAL_ERROR ${FOO})
> ```
> 
> gives `/bin/true` for me.
> I don't think you need to do this if you rename the var, `find_program()` doesn't seem to do any searching when the var is already set.

This might be true but it is counter intuitive to assume that `find_program` does nothing. So for readability I'd keep the `if (NOT LLVM_EXTERNAL_LIT)`. I know it is not needed but with the `if` it won't look like `find_program(LLVM_EXTERNAL_LIT` is the only/central place to set the variable `LLVM_EXTERNAL_LIT`. With the guard around it, it becomes more obvious that this is more of  a fallback.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138258