[PATCH] D137669: clang/cmake: Require pre-built test dependencies for stand-alone builds

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D137669#3920309 , @tstellar wrote:

> In D137669#3915899 , @mstorsjo 
> wrote:
>
>> This does, somewhat, coincide with what I'm trying to do in D131052 
>> . There, I don't point out the binaries 
>> for `FileCheck` and similar, but point out preexisting native `llvm-tblgen`, 
>> `clang-tblgen` etc (to avoid needing to build them in a nested native cmake 
>> when cross compiling) - but if they exist, they would probably be in the 
>> same directory. Would it make sense to try to settle on a common variable 
>> name for both of these? Or is there a case where we specifically need to be 
>> able to differentiate between the two?
>
> Do you mean creating a common variable name for the path to llvm utils?

Yes, exactly. The use cases are maybe slightly separate, but still, in both 
cases it'd be a path to a directory containing native versions of `not`, 
`FileCheck`, `llvm-tblgen` etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137669

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137669: clang/cmake: Require pre-built test dependencies for stand-alone builds

[Tom Stellard via Phabricator via cfe-commits]

tstellar added a comment.

In D137669#3915899 , @mstorsjo wrote:

> This does, somewhat, coincide with what I'm trying to do in D131052 
> . There, I don't point out the binaries for 
> `FileCheck` and similar, but point out preexisting native `llvm-tblgen`, 
> `clang-tblgen` etc (to avoid needing to build them in a nested native cmake 
> when cross compiling) - but if they exist, they would probably be in the same 
> directory. Would it make sense to try to settle on a common variable name for 
> both of these? Or is there a case where we specifically need to be able to 
> differentiate between the two?

Do you mean creating a common variable name for the path to llvm utils?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137669

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137669: clang/cmake: Require pre-built test dependencies for stand-alone builds

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

Also FYI, OpenMP has a bit of prior art in this area - see e.g. 
https://github.com/llvm/llvm-project/blob/main/openmp/cmake/OpenMPTesting.cmake#L27-L36.
 There it prints a warning at cmake time, disabling tests, saying why, and 
giving hints about how to fix it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137669

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137669: clang/cmake: Require pre-built test dependencies for stand-alone builds

[Michał Górny via Phabricator via cfe-commits]

mgorny added a comment.

I don't have a strong opinion. I'm happy as long as there is *anything* telling 
me "tests were disabled because ...".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137669

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137669: clang/cmake: Require pre-built test dependencies for stand-alone builds

[Tom Stellard via Phabricator via cfe-commits]

tstellar added a comment.

In D137669#3916493 , @mgorny wrote:

> Could you issue a warning though? It's really annoying when targets disappear 
> like that and you have to guess which of the checks failed.

What about just making it an error if LLVM_INCLUDE_TESTS is set, but it kind 
find all the tools?  I also don't like the silent disabling of features, but 
I'm not sure how visible the warning would be to users.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137669

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137669: clang/cmake: Require pre-built test dependencies for stand-alone builds

[Michał Górny via Phabricator via cfe-commits]

mgorny added a comment.

Could you issue a warning though? It's really annoying when targets disappear 
like that and you have to guess which of the checks failed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137669

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137669: clang/cmake: Require pre-built test dependencies for stand-alone builds

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added subscribers: smeenai, mstorsjo.
mstorsjo added a comment.

This does, somewhat, coincide with what I'm trying to do in D131052 
. There, I don't point out the binaries for 
`FileCheck` and similar, but point out preexisting native `llvm-tblgen`, 
`clang-tblgen` etc (to avoid needing to build them in a nested native cmake 
when cross compiling) - but if they exist, they would probably be in the same 
directory. Would it make sense to try to settle on a common variable name for 
both of these? Or is there a case where we specifically need to be able to 
differentiate between the two?

CC @smeenai

(I haven't gotten around to refreshing that patch yet, unfortunately.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137669

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137669: clang/cmake: Require pre-built test dependencies for stand-alone builds

[Tom Stellard via Phabricator via cfe-commits]

tstellar created this revision.
tstellar added reviewers: phosek, mgorny, Ericson2314.
Herald added a project: All.
tstellar requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

If the FileCheck, count, and not tools are not found in
LLVM_TOOLS_BINARY_DIR, then tests will be disabled.  CMake will
no longer try to build these tool from the llvm sources.

In theory, since stand-alone builds are meant to be done without
access to the llvm sources, this should be a no-op, but this
change will affect anyone trying to do stand-alone builds with
the full monorepo sources.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D137669

Files:
  clang/CMakeLists.txt


Index: clang/CMakeLists.txt
===
--- clang/CMakeLists.txt
+++ clang/CMakeLists.txt
@@ -86,34 +86,26 @@
   set(LLVM_UTILS_PROVIDED ON)
 endif()
 
-if(EXISTS ${LLVM_MAIN_SRC_DIR}/utils/lit/lit.py)
-  # Note: path not really used, except for checking if lit was found
-  set(LLVM_LIT ${LLVM_MAIN_SRC_DIR}/utils/lit/lit.py)
-  if(EXISTS ${LLVM_MAIN_SRC_DIR}/utils/llvm-lit)
-add_subdirectory(${LLVM_MAIN_SRC_DIR}/utils/llvm-lit utils/llvm-lit)
+# 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 (LLVM_LIT AND LLVM_UTILS_PROVIDED)
+  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()
+set(UNITTEST_DIR ${LLVM_MAIN_SRC_DIR}/utils/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} utils/unittest)
+endif()
   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_MAIN_SRC_DIR}/utils/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} utils/unittest)
-  endif()
-else()
-  # 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")
-endif()
 
-if(LLVM_LIT)
   # Define the default arguments to use with 'lit', and an option for the 
user
   # to override.
   set(LIT_ARGS_DEFAULT "-sv")


Index: clang/CMakeLists.txt
===
--- clang/CMakeLists.txt
+++ clang/CMakeLists.txt
@@ -86,34 +86,26 @@
   set(LLVM_UTILS_PROVIDED ON)
 endif()
 
-if(EXISTS ${LLVM_MAIN_SRC_DIR}/utils/lit/lit.py)
-  # Note: path not really used, except for checking if lit was found
-  set(LLVM_LIT ${LLVM_MAIN_SRC_DIR}/utils/lit/lit.py)
-  if(EXISTS ${LLVM_MAIN_SRC_DIR}/utils/llvm-lit)
-add_subdirectory(${LLVM_MAIN_SRC_DIR}/utils/llvm-lit utils/llvm-lit)
+# 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 (LLVM_LIT AND LLVM_UTILS_PROVIDED)
+  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()
+set(UNITTEST_DIR ${LLVM_MAIN_SRC_DIR}/utils/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} utils/unittest)
+endif()
   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)
-