[CMake] add SKIP to add_lit_testsuites

[makslevental]

This PR adds SKIP to add_lit_testsuites. The purpose is to let people filter the currently exhaustive inclusion of subdirectories which might not all depend on the same DEPENDS. The immediate use case is MLIR where we have a collection of Python tests that currently trigger rebuilds of various tools (like mlir-opt) which they do not depend on. That collection of tests is updated here too.

[llvmbot]

@llvm/pr-subscribers-mlir

Author: Maksim Levental (makslevental)

Changes

This PR adds SKIP to add_lit_testsuites. The purpose is to let people filter the current exhaustive inclusion of subdirectories which might not all depend on the same DEPENDS. The immediate usecase is MLIR where we have a collection of Python tests that currently trigger rebuilds of various tools (like mlir-opt) which they do not depend on. That collection of tests is updated here too.

---

Full diff: https://github.com/llvm/llvm-project/pull/157176.diff

3 Files Affected:

• (modified) llvm/cmake/modules/AddLLVM.cmake (+13-1)
• (modified) mlir/test/CMakeLists.txt (+1)
• (modified) mlir/test/python/CMakeLists.txt (+5)

diff --git a/llvm/cmake/modules/AddLLVM.cmake b/llvm/cmake/modules/AddLLVM.cmake
index 835750e2d2a13..9058ed3831b9c 100644
--- a/llvm/cmake/modules/AddLLVM.cmake
+++ b/llvm/cmake/modules/AddLLVM.cmake
@@ -2192,7 +2192,7 @@ endfunction()
 
 function(add_lit_testsuites project directory)
   if (NOT LLVM_ENABLE_IDE)
-    cmake_parse_arguments(ARG "EXCLUDE_FROM_CHECK_ALL" "FOLDER;BINARY_DIR" "PARAMS;DEPENDS;ARGS" ${ARGN})
+    cmake_parse_arguments(ARG "EXCLUDE_FROM_CHECK_ALL" "FOLDER;BINARY_DIR" "PARAMS;DEPENDS;ARGS;SKIP" ${ARGN})
 
     if (NOT ARG_FOLDER)
       get_subproject_title(subproject_title)
@@ -2208,6 +2208,18 @@ function(add_lit_testsuites project directory)
       endif()
       string(FIND ${lit_suite} Inputs is_inputs)
       string(FIND ${lit_suite} Output is_output)
+      set(_should_skip FALSE)
+      foreach(skip ${ARG_SKIP})
+        string(FIND ${lit_suite} ${skip} is_skip)
+        if (NOT is_skip EQUAL -1)
+          set(_should_skip TRUE)
+          break()
+        endif()
+      endforeach()
+      if (_should_skip)
+        continue()
+      endif()
+      string(FIND ${lit_suite} Output is_output)
       if (NOT (is_inputs EQUAL -1 AND is_output EQUAL -1))
         continue()
       endif()
diff --git a/mlir/test/CMakeLists.txt b/mlir/test/CMakeLists.txt
index 8ddc620ae33be..c415e65043d0f 100644
--- a/mlir/test/CMakeLists.txt
+++ b/mlir/test/CMakeLists.txt
@@ -247,4 +247,5 @@ add_lit_testsuite(check-mlir "Running the MLIR regression tests"
 
 add_lit_testsuites(MLIR ${CMAKE_CURRENT_SOURCE_DIR}
   DEPENDS ${MLIR_TEST_DEPENDS}
+  SKIP python
 )
diff --git a/mlir/test/python/CMakeLists.txt b/mlir/test/python/CMakeLists.txt
index 1db957c86819d..b1d126754517f 100644
--- a/mlir/test/python/CMakeLists.txt
+++ b/mlir/test/python/CMakeLists.txt
@@ -10,3 +10,8 @@ mlir_tablegen(lib/PythonTestTypes.cpp.inc -gen-typedef-defs)
 add_public_tablegen_target(MLIRPythonTestIncGen)
 
 add_subdirectory(lib)
+
+add_lit_testsuite(check-mlir-python "Running the MLIR Python regression tests"
+  ${CMAKE_CURRENT_BINARY_DIR}
+  DEPENDS MLIRPythonModules
+)
\ No newline at end of file

[joker-eph]

Suggested change

	foreach(skip ${ARG_SKIP})
	string(FIND ${lit_suite} ${skip} is_skip)
	if (NOT is_skip EQUAL -1)
	foreach(skip IN LISTS ARG_SKIP)
	if(lit_suite MATCHES "${skip}")
	set(_should_skip TRUE)

Would that work?

(You're using FIND which I'm not sure if an exact match)

[makslevental]

i moved the check down to after name_slash (and made the check on name_slash MATCHES skip) so now i think we have the effect you wanted (ability to precisely match using regex).

[joker-eph]

I'm not quite following the effect of not doing it actually.
What happens if I take this patch and remove just this line?

[makslevental]

nothing? there will be no string(FIND ${lit_suite} ${skip} is_skip); is_skip > -1 and so no subdirs will be skipped.

[joker-eph]

nothing?

I mean surely this line is added for having an effect, otherwise we can just remove it right?

so no subdirs will be skipped.

I get that, I just don't quite figure what is the effect of skipping it. Can you describe a command I can run and what I should observe before and after?
(my question applies to the revise pattern as well)

[joker-eph]

Is it that when you do:

add_lit_testsuite(check-mlir-python

later, cmake will complain that the target already exists?

[makslevental]

Yes

[christopherbate]

The change makes sense to me. IIUC, it only affects the generation of "convenience targets" for subfolders, but "check-mlir" will still run the Python tests. The only comment I have is that we should start adding documentation to the affected functions in AddLLVM.cmake.

[s-barannikov]

@jurahul FYI

[jurahul]

@jurahul FYI

Thanks. Let me adopt this #155929