[cmake] Handled utils/unittests before projects

[jchlanda]

Summary

This patch makes sure that utils/unittests are handled before projects in cmake. It allows cmake targets that are created by projects to use utility targets, such as: gtest or gtest_main, that are created in utils/unittests subdirectory.

[jchlanda]

ping

[bader]

LGTM, although it would be good if some else take a look as well.

[beanz]

What are you trying to do in a project that is motivating this?

I don’t have any real objection to this change, but the motivation concerns me. Generally we avoid things that require order-dependence, and setting up target dependencies does not.

[jchlanda]

In D104848#2910218, @beanz wrote:

What are you trying to do in a project that is motivating this?

I don’t have any real objection to this change, but the motivation concerns me. Generally we avoid things that require order-dependence, and setting up target dependencies does not.

This issue was brought forward from Intel's SYCL project, and specifically how it builds check-sycl target, you can follow it here: https://github.com/intel/llvm/pull/3964

[beanz]

This is not the right fix for that bug. If sycl is behaving as an LLVM subproject it should respect LLVM_INCLUDE_TESTS, it should not rely on querying if the testing targets are present.

[jchlanda]

SYCL already predicates building unittests on the value of LLVM_INCLUDE_TESTS (https://github.com/intel/llvm/blob/sycl/sycl/CMakeLists.txt#L212). The problem is that at the point of time when check-sycl is built gtests are not known to CMake (even though SYCL code did check for the existence of the source), so it is impossible to add them to the list of dependencies. Yes they will be built (if the LLVM_INCLUDE_TESTS is set on), but the dependency will not be set up correctly and depending on the order in which the targets are built check-sycl might fail to link. It boils down to inter-target dependencies in CMake.

[beanz]

In D104848#2912827, @jchlanda wrote:

SYCL already predicates building unittests on the value of LLVM_INCLUDE_TESTS (https://github.com/intel/llvm/blob/sycl/sycl/CMakeLists.txt#L212).

This is actually not safe. You're creating a new cached value based on the value of another cached value. Both can be set and updated independently of each other. You should (at least) add a hard error if SYCL_INCLUDE_TESTS=On and LLVM_INCLUDE_TESTS=Off.

The problem is that at the point of time when check-sycl is built gtests are not known to CMake (even though SYCL code did check for the existence of the source), so it is impossible to add them to the list of dependencies. Yes they will be built (if the LLVM_INCLUDE_TESTS is set on), but the dependency will not be set up correctly and depending on the order in which the targets are built check-sycl might fail to link. It boils down to inter-target dependencies in CMake.

I don't at all understand what you're talking about here. In CMake it is totally valid to list a dependency before the target is defined. Meaning:

add_library(foo ...)
target_link_libraries(foo PRIVATE bar)
...
add_library(bar ...)

Foo will link bar, the target, not -lbar.

What you can't do without ordering issues is:

add_library(foo ...)
if(TARGET bar)
  target_link_libraries(foo PRIVATE bar)
endif()
...
add_library(bar ...)

SYCL should be able to do something like:

if(LLVM_INCLUDE_TESTS)
  target_link_libraries(foo PRIVATE gtest)
endif()

Why doesn't that work?

[jchlanda]

This is actually not safe. You're creating a new cached value based on the value of another cached value. Both can be set and updated independently of each other. You should (at least) add a hard error if SYCL_INCLUDE_TESTS=On and LLVM_INCLUDE_TESTS=Off.

That is a good point which will need to be addressed, thank you.

Why doesn't that work?

When creating sycl executable the dependencies are tracked through add_dependencies (https://github.com/intel/llvm/blob/sycl/sycl/cmake/modules/AddSYCLExecutable.cmake#L40) when this line executes with gtestset as <target-dependency> it (gtes) is not a know target, cmake, being cmake is completely happy about it and silently swallows it. When you proceed to building your target the fact that sycl executable does depend on gtest is lost, resulting in sycl executable being built ahead of gtest and a bunch of linker errors. Pleas do let me know if there is anything else that I could clarify.

[beanz]

In D104848#2913981, @jchlanda wrote:

When creating sycl executable the dependencies are tracked through add_dependencies (https://github.com/intel/llvm/blob/sycl/sycl/cmake/modules/AddSYCLExecutable.cmake#L40)

This is also a misuse of CMake. You should be using target_link_libraries instead, it will behave as expected regardless of ordering. See:
https://cmake.org/cmake/help/latest/command/target_link_libraries.html

[beanz]

I think I initially misunderstood what you're saying here

In D104848#2913981, @jchlanda wrote:

When creating sycl executable the dependencies are tracked through add_dependencies (https://github.com/intel/llvm/blob/sycl/sycl/cmake/modules/AddSYCLExecutable.cmake#L40)

add_dependencies actually should work fine, regardless of the ordering. The problem is you're checking for the existence of the target.

Stop doing that.

Instead only add the gtest targets to the list of dependencies if LLVM_INCLIDE_TESTS=On

[jchlanda]

add_dependencies actually should work fine, regardless of the ordering. The problem is you're checking for the existence of the target.

Stop doing that.

I think I know what went wrong in here, someone made an innocent looking typo in the foreach command https://github.com/intel/llvm/blob/sycl/sycl/cmake/modules/AddSYCLExecutable.cmake#L38, the in is incorrect there, as it is effectively being treated as a target, without that check for the existence of the target the token in would have been passed to the add_dependencies which would fail.

You are also right, that without the check add_dependencies "just works", so the patch is not needed at all, abandoning it now, as I can solve it entirely in sycl's codebase.

Thank you for your time/patience @beanz, much appreciated.

[beanz]

In D104848#2916396, @jchlanda wrote:

I think I know what went wrong in here, someone made an innocent looking typo in the foreach command https://github.com/intel/llvm/blob/sycl/sycl/cmake/modules/AddSYCLExecutable.cmake#L38, the in is incorrect there, as it is effectively being treated as a target, without that check for the existence of the target the token in would have been passed to the add_dependencies which would fail.

Ah! Yea, the CMake foreach(<var> ...) and foreach(<var> IN [LISTS|ITEMS] ...) formations are a bit odd and can trip you up!

Thank you for your time/patience @beanz, much appreciated.

Glad you figured it out!

llvm/CMakeLists.txt

 add_subdirectory(lib)
 
 if( LLVM_INCLUDE_UTILS )
   add_subdirectory(utils/FileCheck)
   add_subdirectory(utils/PerfectShuffle)
   add_subdirectory(utils/count)
   add_subdirectory(utils/not)
   add_subdirectory(utils/yaml-bench)
+  if( LLVM_INCLUDE_TESTS )
+    add_subdirectory(utils/unittest)
+  endif()
 else()
   if ( LLVM_INCLUDE_TESTS )
     message(FATAL_ERROR "Including tests when not building utils will not work.
     Either set LLVM_INCLUDE_UTILS to On, or set LLVM_INCLUDE_TESTS to Off.")
   endif()
 endif()
 
 # Use LLVM_ADD_NATIVE_VISUALIZERS_TO_SOLUTION instead of LLVM_INCLUDE_UTILS because it is not really a util
llvm_ExternalProject_Add(test-suite ${LLVM_MAIN_SRC_DIR}/projects/test-suite
       USE_TOOLCHAIN
       EXCLUDE_FROM_ALL
       NO_INSTALL
       ALWAYS_CLEAN)
   endif()
   add_subdirectory(utils/lit)
   add_subdirectory(test)
   add_subdirectory(unittests)
-  if( LLVM_INCLUDE_UTILS )
-    add_subdirectory(utils/unittest)
-  endif()
 
   if (WIN32)
     # This utility is used to prevent crashing tests from calling Dr. Watson on
     # Windows.
     add_subdirectory(utils/KillTheDoctor)
   endif()
 
   # Add a global check rule now that all subdirectories have been traversed