[CMake] Include runtimes test suites in check-all

[phosek]

Summary

Prior to this change, we would make check-all depend on check-runtimes
which is a target that runs tests in the runtimes build. This means that
the runtimes tests are going to run prior to other test suites in
check-all, and if one of them fails, we won't run the other test suites
at all.

To address this issue, we instead collect the list of test suites and
their dependencies from the runtimes subbuild, and include them in
check-all, so a failure of runtimes test suite doesn't prevent other
test suites from being executed.

This addresses https://github.com/llvm/llvm-project/issues/54154.

[ldionne]

Interesting -- so you basically write the test dependencies, parameters and test suites i.e. everything that check-runtimes contains into a file:

add_lit_target(check-runtimes
    "Running all regression tests"
    ${RUNTIMES_LIT_TESTSUITES}
    PARAMS ${RUNTIMES_LIT_PARAMS}
    DEPENDS ${RUNTIMES_LIT_DEPENDS}
    ARGS ${RUNTIMES_LIT_EXTRA_ARGS}
)

and then you add that directly to check-all from the main build.

This makes a lot of sense in principle, I'm running some tests right now but this LGTM. I assume you've tested it too?

[ldionne]

When I apply your patch locally, run a bootstrapping build and then invoke check-all, for some reason I still see the check-runtimes target being run before all the other targets. I assume this is not intended?

[phosek]

In D121276#3370263, @ldionne wrote:

When I apply your patch locally, run a bootstrapping build and then invoke check-all, for some reason I still see the check-runtimes target being run before all the other targets. I assume this is not intended?

I see it as well, let me debug this.

In D121276#3370012, @ldionne wrote:

Interesting -- so you basically write the test dependencies, parameters and test suites i.e. everything that check-runtimes contains into a file:

add_lit_target(check-runtimes
    "Running all regression tests"
    ${RUNTIMES_LIT_TESTSUITES}
    PARAMS ${RUNTIMES_LIT_PARAMS}
    DEPENDS ${RUNTIMES_LIT_DEPENDS}
    ARGS ${RUNTIMES_LIT_EXTRA_ARGS}
)

and then you add that directly to check-all from the main build.

We use the same approach to also collect components from the compiler-rt build, see https://github.com/llvm/llvm-project/blob/c3a7627cacc6cbe2301a253daeb3e6953e5e0d1d/runtimes/Components.cmake.in.

[phosek]

@ldionne can you try the updated version?

[ldionne]

In D121276#3370731, @phosek wrote:

@ldionne can you try the updated version?

Just finished bootstrapping, and the tests are starting immediately with 85534 tests to run. That seems right!

[jdenny]

When building with -DLLVM_ENABLE_RUNTIMES=openmp, this patch causes openmp runtime tests to no longer be included by check-all.

I've added a few subscribers who might want to be aware of this change.

[JonChesterfield]

@ronlieb this probably means our CI needs to do different things

[jdenny]

IMHO, check-all should include openmp runtime tests, and this should be fixed. I'm not sure how openmp developers feel.

[jdoerfert]

In D121276#3375544, @jdenny wrote:

IMHO, check-all should include openmp runtime tests, and this should be fixed. I'm not sure how openmp developers feel.

Yes. @phosek, any idea why the OpenMP runtime tests are not picked up with this?

[phosek]

I tested this a bit more and it seems like changes to Tests.cmake aren't picked up by Ninja for some reason. It's not specific only to OpenMP, it seems to be affecting all other runtimes as well. If I manually rerun CMake, the tests are picked up but this should happen automatically.

[jdenny]

In D121276#3380266, @phosek wrote:

I tested this a bit more and it seems like changes to Tests.cmake aren't picked up by Ninja for some reason. It's not specific only to OpenMP, it seems to be affecting all other runtimes as well. If I manually rerun CMake, the tests are picked up but this should happen automatically.

I see the problem when building from scratch. I'm not sure if you are suggesting it happens only for incremental builds.

I have tried only openmp and ninja, not other runtimes or make.

[phosek]

Can you test D121647 to see if it resolves the issue for you?

[jdenny]

In D121276#3380870, @phosek wrote:

Can you test D121647 to see if it resolves the issue for you?

Applying it at 6ac3d8ef9c43 causes check-all to include openmp tests (both libomp and libomptarget) again for me. I did not otherwise review the patch.

Thanks for that fix, and thanks for this patch to merge the test suite runs.

[kamaub]

This change broke the clang-ppc64le-rhel #15899 bot. It actually pass this and every other bot green
but the next build after sanitizer test cases on clang-ppc64le-rhel #15900 for continuous builds fails,
that is, some clean builds pass but mostly continuous builds fail.

I originally though this was an issue with the container and moved the bot to staging server as these failures
did not happen on any other bots as far as I could tell but after this patch was pointed out to me I did some
extensive testing on the container and found this change caused the failures we saw.

I applied D121647 at 6ac3d8ef9c as well and found that these sanitizer test cases only pass when a clean build of
6ac3d8ef9c + D121647 was before so I think once D121647 lands the next clean build should bring out bots back to green,
I hope it lands soon so we can be sure it works and move the bot from staging, I will try to review it now, thank you for the fix.

[phosek]

I need LGTM on D121647, would one of you be willing to accept it?

[jdenny]

In D121276#3386779, @phosek wrote:

I need LGTM on D121647, would one of you be willing to accept it?

It looks like @ldionne handled it before I got to it. Thanks again for these improvements.

[lei]

@phosek It seems the runtime tests are no longer running for us after this patch in PPC bot https://lab.llvm.org/buildbot/#/builders/57. I can see they are still ran when it's part of LLVM_ENABLE_PROJECTS, but this bot specifies compiler-rt as LLVM_ENABLE_RT=compiler-rt.

I was not able to find any AddressSanitizer tests in the check-all log with LLVM_LIT_ARGS:STRING=-v.
Running ninja check-compiler-rt I see:

$ grep AddressSanitizer check-compiler-rt.log| grep -v -c 'LLVM ::'
1534

Unable to find the same list in ninja check-all log:

$ grep AddressSanitizer check-all.log| grep -v -c 'LLVM ::'
0

Also, running with and without this patch showed the same number of regression tests with the exception that before this patch it also showed that there were over 2K compiler-rt tests ran.
Any idea why this is happening?

[kamaub]

@phosek Pinging this patch, our builders which run check-all and expect compiler-rt, enabled as a runtime with -DLLVM_ENABLE_RUNTIMES=compiler-rt, to be covered in testing are not doing this and are still being affected by this changeset.
clang-ppc64le-rhel and clang-ppc64-aix are not currently testing compiler-rt because of this change.
clang-ppc64le-linux-test-suite enables compiler-rt as a project with -DLLVM_ENABLE_PROJECTS=compiler-rt and test it under check-all as expected.
Before this, both bots would test compiler-rt under check-all. Could you please take a look at this?

llvm/CMakeLists.txt

get_property(LLVM_ADDITIONAL_TEST_DEPENDS
                GLOBAL PROPERTY LLVM_ADDITIONAL_TEST_DEPENDS)
   add_lit_target(check-all
     "Running all regression tests"
     ${LLVM_LIT_TESTSUITES}
     PARAMS ${LLVM_LIT_PARAMS}
     DEPENDS ${LLVM_LIT_DEPENDS} ${LLVM_ADDITIONAL_TEST_TARGETS}
     ARGS ${LLVM_LIT_EXTRA_ARGS}
     )
-  if(TARGET check-runtimes)
-    add_dependencies(check-all check-runtimes)
-  endif()
   add_custom_target(test-depends
                     DEPENDS ${LLVM_LIT_DEPENDS} ${LLVM_ADDITIONAL_TEST_DEPENDS})
   set_target_properties(test-depends PROPERTIES FOLDER "Tests")
 endif()
 
 if (LLVM_INCLUDE_DOCS)
   add_subdirectory(docs)
 endif()

llvm/runtimes/CMakeLists.txt

function(runtime_default_target)
   endforeach()
   foreach(component ${LLVM_RUNTIME_DISTRIBUTION_COMPONENTS})
     if(NOT ${component} IN_LIST SUB_COMPONENTS)
       list(APPEND extra_targets ${component} install-${component} install-${component}-stripped)
     endif()
   endforeach()
 
   if(LLVM_INCLUDE_TESTS)
+    include(${LLVM_BINARY_DIR}/runtimes/Tests.cmake OPTIONAL RESULT_VARIABLE have_tests)
+    set_property(DIRECTORY APPEND PROPERTY CMAKE_CONFIGURE_DEPENDS ${LLVM_BINARY_DIR}/runtimes/Tests.cmake)
+    if(have_tests)
+      set_property(GLOBAL APPEND PROPERTY LLVM_LIT_TESTSUITES ${RUNTIMES_LIT_TESTSUITES})
+      set_property(GLOBAL APPEND PROPERTY LLVM_LIT_PARAMS ${RUNTIMES_LIT_PARAMS})
+      set_property(GLOBAL APPEND PROPERTY LLVM_LIT_EXTRA_ARGS ${RUNTIMES_LIT_EXTRA_ARGS})
+    endif()
     list(APPEND test_targets runtimes-test-depends check-runtimes)
   endif()
 
   set(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR_default ON)
   # AIX should fold 32-bit & 64-bit arch libraries into a single archive.
   if (TARGET_TRIPLE MATCHES "aix")
     set(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR_default OFF)
   endif()
function(runtime_register_target name target)
   foreach(component ${LLVM_RUNTIME_DISTRIBUTION_COMPONENTS})
     set(${component}-${name} ${component})
     set(install-${component}-${name} install-${component})
     set(install-${component}-${name}-stripped install-${component}-stripped)
     list(APPEND ${name}_extra_targets ${component}-${name} install-${component}-${name} install-${component}-${name}-stripped)
   endforeach()
 
   if(LLVM_INCLUDE_TESTS)
+    include(${LLVM_BINARY_DIR}/runtimes/${name}/Tests.cmake OPTIONAL RESULT_VARIABLE have_tests)
+    set_property(DIRECTORY APPEND PROPERTY CMAKE_CONFIGURE_DEPENDS ${LLVM_BINARY_DIR}/runtimes/${name}/Tests.cmake)
+    if(have_tests)
+      set_property(GLOBAL APPEND PROPERTY LLVM_LIT_TESTSUITES ${RUNTIMES_LIT_TESTSUITES})
+      set_property(GLOBAL APPEND PROPERTY LLVM_LIT_PARAMS ${RUNTIMES_LIT_PARAMS})
+      set_property(GLOBAL APPEND PROPERTY LLVM_LIT_EXTRA_ARGS ${RUNTIMES_LIT_EXTRA_ARGS})
+    endif()
     set(runtimes-test-depends-${name} runtimes-test-depends)
     set(check-runtimes-${name} check-runtimes)
     list(APPEND ${name}_test_targets runtimes-test-depends-${name} check-runtimes-${name})
     list(APPEND test_targets ${${name}_test_targets})
     foreach(target_name IN LISTS SUB_CHECK_TARGETS)
       set(${target_name}-${name} ${target_name})
       list(APPEND ${name}_test_targets ${target_name}-${name})
       list(APPEND test_targets ${target_name}-${name})
if(NOT LLVM_BUILD_INSTRUMENTED AND CLANG_ENABLE_BOOTSTRAP)
     add_dependencies(clang-bootstrap-deps runtimes-configure)
     # We need to add the runtimes as a dependency because compiler-rt can be
     # built as part of runtimes and we need the profile runtime for PGO
     add_dependencies(clang-bootstrap-deps runtimes)
   endif()
 
   if(LLVM_INCLUDE_TESTS)
     set_property(GLOBAL APPEND PROPERTY LLVM_ADDITIONAL_TEST_DEPENDS runtimes-test-depends)
-    set_property(GLOBAL APPEND PROPERTY LLVM_ADDITIONAL_TEST_TARGETS check-runtimes)
 
     set(RUNTIMES_TEST_DEPENDS
         FileCheck
         count
         llvm-nm
         llvm-objdump
         llvm-xray
         not

runtimes/CMakeLists.txt

if(LLVM_INCLUDE_TESTS)
 
   if (NOT HAVE_LLVM_LIT)
     # If built by manually invoking cmake on this directory, we don't have
     # llvm-lit. If invoked via llvm/runtimes, the toplevel llvm cmake
     # invocation already generated the llvm-lit script.
     add_subdirectory(${LLVM_MAIN_SRC_DIR}/utils/llvm-lit
                      ${CMAKE_CURRENT_BINARY_DIR}/llvm-lit)
   endif()
+
+  if(LLVM_RUNTIMES_TARGET)
+    configure_file(
+      ${CMAKE_CURRENT_SOURCE_DIR}/Tests.cmake.in
+      ${LLVM_BINARY_DIR}/runtimes/${LLVM_RUNTIMES_TARGET}/Tests.cmake)
+  else()
+    configure_file(
+      ${CMAKE_CURRENT_SOURCE_DIR}/Tests.cmake.in
+      ${LLVM_BINARY_DIR}/runtimes/Tests.cmake)
+  endif()
 endif()
 
 get_property(SUB_COMPONENTS GLOBAL PROPERTY SUB_COMPONENTS)
 if(SUB_COMPONENTS)
   list(REMOVE_DUPLICATES SUB_COMPONENTS)
   foreach(component ${SUB_COMPONENTS})
     if(NOT TARGET ${component})
       message(SEND_ERROR "Missing target for runtime component ${component}!")

runtimes/Tests.cmake.in

+set(RUNTIMES_LIT_TESTSUITES @RUNTIMES_LIT_TESTSUITES@)
+set(RUNTIMES_LIT_PARAMS @RUNTIMES_LIT_PARAMS@)
+set(RUNTIMES_LIT_EXTRA_ARGS @RUNTIMES_LIT_EXTRA_ARGS@)