clang/cmake: Fix incorrectly disabling tests when LLVM_EXTERNAL_LIT is used

[tstellar]

Summary

This fixes a bug where tests would be disabled when LLVM_EXTERNAL_LIT
was set if lit couldn't be found in any of the standard search paths.

[phosek]

LGTM

[mgorny]

clang/CMakeLists.txt

97

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.

[kwk]

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 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 shared 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.

clang/CMakeLists.txt

97

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.

[tstellar]

In D138258#3936260, @kwk wrote:

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 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 shared CMake code between subprojects, right @phosek (didn't you create /cmake in the first place)?

My eventual goal is to move this out into a separate CMake file, but I was hoping to simplify the code first before doing this.

[tstellar]

clang/CMakeLists.txt

97

@mgorny The 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."

clang/CMakeLists.txt

if(LLVM_INCLUDE_TESTS)
     # Check prebuilt llvm/utils.
     if(EXISTS ${LLVM_TOOLS_BINARY_DIR}/FileCheck${CMAKE_EXECUTABLE_SUFFIX}
         AND EXISTS ${LLVM_TOOLS_BINARY_DIR}/count${CMAKE_EXECUTABLE_SUFFIX}
         AND EXISTS ${LLVM_TOOLS_BINARY_DIR}/not${CMAKE_EXECUTABLE_SUFFIX})
       set(LLVM_UTILS_PROVIDED ON)
     endif()
 
     # Seek installed Lit.
-    find_program(LLVM_LIT
+    if (NOT LLVM_EXTERNAL_LIT)
+      find_program(LLVM_EXTERNAL_LIT
                    NAMES llvm-lit lit.py lit
                    PATHS "${LLVM_MAIN_SRC_DIR}/utils/lit"
                    DOC "Path to lit.py")
+    endif()
 
     if(EXISTS ${LLVM_MAIN_SRC_DIR}/utils/lit/lit.py)
       # Note: path not really used, except for checking if lit was found
       if(EXISTS ${LLVM_MAIN_SRC_DIR}/utils/llvm-lit)
         add_subdirectory(${LLVM_MAIN_SRC_DIR}/utils/llvm-lit utils/llvm-lit)
       endif()
       if(NOT LLVM_UTILS_PROVIDED)
         add_subdirectory(${LLVM_MAIN_SRC_DIR}/utils/FileCheck utils/FileCheck)
         add_subdirectory(${LLVM_MAIN_SRC_DIR}/utils/count utils/count)
         add_subdirectory(${LLVM_MAIN_SRC_DIR}/utils/not utils/not)
         set(LLVM_UTILS_PROVIDED ON)
         set(CLANG_TEST_DEPS FileCheck count not)
       endif()
       set(UNITTEST_DIR ${LLVM_THIRD_PARTY_DIR}/unittest)
       if(EXISTS ${UNITTEST_DIR}/googletest/include/gtest/gtest.h
           AND NOT EXISTS ${LLVM_LIBRARY_DIR}/${CMAKE_STATIC_LIBRARY_PREFIX}gtest${CMAKE_STATIC_LIBRARY_SUFFIX}
           AND EXISTS ${UNITTEST_DIR}/CMakeLists.txt)
         add_subdirectory(${UNITTEST_DIR} third-party/unittest)
       endif()
     endif()
 
-    if(LLVM_LIT)
+    if(LLVM_EXTERNAL_LIT)
       # Define the default arguments to use with 'lit', and an option for the user
       # to override.
       set(LIT_ARGS_DEFAULT "-sv")
       if (MSVC OR XCODE)
         set(LIT_ARGS_DEFAULT "${LIT_ARGS_DEFAULT} --no-progress-bar")
       endif()
       set(LLVM_LIT_ARGS "${LIT_ARGS_DEFAULT}" CACHE STRING "Default options for lit")