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

[Tom Stellard via Phabricator via cfe-commits]

tstellar added inline comments.


================
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
----------------
kwk wrote:
> 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.
@mgorny  The [[ http://devdoc.net/linux/cmake-3.9.6/command/find_program.html | documentation ]] confirms this too:

"Once one of the calls succeeds the result variable will be set and stored in the cache so that no call will search again."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138258