Incorrect Dependency on VDT

[krasznaa]

• Checked for duplicates

Describe the bug

The ROOTVecOps library incorrectly declares a PRIVATE dependency on the VDT headers.

https://github.com/root-project/root/blob/master/math/vecops/CMakeLists.txt#L27

(To be correct, it declares its dependency on VDT in a very silly way that does not make any sense to me...)

Even though it clearly "publicly" depends on those headers.

https://github.com/root-project/root/blob/master/math/vecops/inc/ROOT/RVec.hxx#L54

Expected behavior

When using the exported target ROOT::ROOTVecOps, I would expect to be able to use RVec.hxx just by linking my own target against ROOT::ROOTVecOps. Not having to care about the "internal" dependency of the ROOT code on VDT myself.

To fix, the following should happen:

• VDT should be used in the ROOT CMake configuration through an imported target, not through ${VDT_INCLUDE_DIRS} and ${VDT_LIBRARIES}. (Since these make relocation very hard.)
• Once ROOTVecOps depends on a library like VDT::vdt, ROOTConfig.cmake.in will need to include

find_dependency(VDT)

somewhere around here: https://github.com/root-project/root/blob/master/cmake/scripts/ROOTConfig.cmake.in#L88-L99

In such a setup, as long as builtin_vdt is "correctly" installed alongside ROOT, the configuration should start becoming correct under all circumstances.

To Reproduce

Don't really have a reproducer. But a CMake project like:

cmake_minimum_required( VERSION 3.10 )
project( VecOpsTest )

find_package( ROOT REQUIRED )

add_executable( VecOpsTest VecOpsText.cpp )
target_link_libraries( VecOpsTest PRIVATE ROOT::ROOTVecOps )

, with a simple

#include <ROOT/RVec.hxx>

int main() { return 0; }

(as VecOpsTest.cpp) should be able to produce an error. As long as ROOT and VDT headers are not in the same location. Which is usually the case unfortunately...

Setup

This is primarily an issue with "LCG builds" of ROOT. Where currently we have to manually take care of linking our code explicitly against VDT, even if we don't use those headers directly ourselves.

Additional context

The report was triggered by an ATLAS analyser complaining about this issue, with their own analysis code.

[eguiraud]

Thank you for the report Attila, I will take a look asap.

[amadio]

I agree that there is a problem, and it comes from the fact that you cannot easily depend on Vdt via targets when Vdt is builtin. CMake complains about headers in the interface which are in the build directory in that case, so a lot of workarounds are needed and no workaround works reliably across CMake versions. When I last touched this, we decided to use variables and rely on the fact that vdt headers were installed into the same place as ROOT headers when Vdt was builtin. However, when Vdt and ROOT are installed separately and into different locations, this doesn't work so well indeed. Fixing this is overdue, but we unfortunately have to keep it working for both builtin/external Vdt. I think that the solution proposed by @krasznaa is in the right direction.

[amadio]

See also https://sft.its.cern.ch/jira/browse/ROOT-9751

[eguiraud]

Reassigning to Bertrand as this is a more general build system issue than an issue with how libVecOps is set up (as I first assumed).

[amadio]

For anyone trying to fix this, please make sure to try with at least the earliest and latest supported CMake versions, and both for builtin/external Vdt. Maybe the situation improved with CMake 3.16 and above, but when I last touched this stuff, CMake was broken enough that I could not find a way not to do what I did by just using variables. To fix, one could try to revert commit a363eca referenced in the issue above, and try to fix the problem without getting rid of the Vdt::Vdt target.

[krasznaa]

Okay, CMake tutorial time...

You basically have 2 ways in my mind to solve this nicely.

1. Switch to using the VDT::VDT library during the build.

• For this you need to introduce a global imported library for builtin_vdt. Something like:

diff --git a/cmake/modules/SearchInstalledSoftware.cmake b/cmake/modules/SearchInstalledSoftware.cmake
index 7947fddfc0..7ad5fd91af 100644
--- a/cmake/modules/SearchInstalledSoftware.cmake
+++ b/cmake/modules/SearchInstalledSoftware.cmake
@@ -1678,6 +1678,12 @@ if(vdt OR builtin_vdt)
             DESTINATION ${CMAKE_INSTALL_INCLUDEDIR} COMPONENT extra-headers)
     set(vdt ON CACHE BOOL "Enabled because builtin_vdt enabled (${vdt_description})" FORCE)
     set_property(GLOBAL APPEND PROPERTY ROOT_BUILTIN_TARGETS VDT)
+    add_library(VDT::VDT UNKNOWN IMPORTED GLOBAL)
+    set_target_properties(VDT::VDT
+      PROPERTIES
+        IMPORTED_LOCATION "${VDT_LIBRARIES}"
+        INTERFACE_INCLUDE_DIRECTORIES "${VDT_INCLUDE_DIRS}"
+    )
   endif()
 endif()

• At this point you could simplify the build configuration to:

diff --git a/math/vecops/CMakeLists.txt b/math/vecops/CMakeLists.txt
index 09fde3eb40..e15b5ea186 100644
--- a/math/vecops/CMakeLists.txt
+++ b/math/vecops/CMakeLists.txt
@@ -8,10 +8,6 @@
 # CMakeLists.txt file for building ROOT math/vecops package
 ############################################################################
 
-if(builtin_vdt)
-  link_directories(${CMAKE_LIBRARY_OUTPUT_DIRECTORY})
-endif()
-
 ROOT_STANDARD_LIBRARY_PACKAGE(ROOTVecOps
   HEADERS
     ROOT/RVec.hxx
@@ -24,13 +20,7 @@ ROOT_STANDARD_LIBRARY_PACKAGE(ROOTVecOps
 )
 
 if(builtin_vdt OR vdt)
-   target_include_directories(ROOTVecOps PRIVATE ${VDT_INCLUDE_DIRS} INTERFACE $<BUILD_INTERFACE:${VDT_INCLUDE_DIRS}>)
-endif()
-
-if(builtin_vdt)
-  target_link_libraries(ROOTVecOps PRIVATE ${VDT_LIBRARIES})
-elseif(vdt)
-  target_link_libraries(ROOTVecOps PUBLIC ${VDT_LIBRARIES})
+   target_link_libraries(ROOTVecOps PUBLIC VDT::VDT)
 endif()
 
 if(MSVC)

• Finally, you would have to ensure that find_package(ROOT) would look for VDT if it needs to. Like:

diff --git a/cmake/scripts/ROOTConfig.cmake.in b/cmake/scripts/ROOTConfig.cmake.in
index 54f1a17140..054954ac8c 100644
--- a/cmake/scripts/ROOTConfig.cmake.in
+++ b/cmake/scripts/ROOTConfig.cmake.in
@@ -97,6 +97,9 @@ if(ROOT_minuit2_omp_FOUND)
   find_dependency(OpenMP)
   find_dependency(Threads)
 endif()
+if(@vdt@ OR @builtin_vdt@)
+  find_dependency(Vdt)
+endif()
 
 #----------------------------------------------------------------------------
 # Now set them to ROOT_LIBRARIES

Though on this latter part I wasn't sure how you prefer to pass configuration options from the build to the generated ROOTConfig.cmake file. 🤔

2. Just use VDT_INCLUDE_DIRS and VDT_LIBRARIES correctly. Like:

diff --git a/math/vecops/CMakeLists.txt b/math/vecops/CMakeLists.txt
index 09fde3eb40..dd998c1a9b 100644
--- a/math/vecops/CMakeLists.txt
+++ b/math/vecops/CMakeLists.txt
@@ -8,10 +8,6 @@
 # CMakeLists.txt file for building ROOT math/vecops package
 ############################################################################
 
-if(builtin_vdt)
-  link_directories(${CMAKE_LIBRARY_OUTPUT_DIRECTORY})
-endif()
-
 ROOT_STANDARD_LIBRARY_PACKAGE(ROOTVecOps
   HEADERS
     ROOT/RVec.hxx
@@ -23,13 +19,13 @@ ROOT_STANDARD_LIBRARY_PACKAGE(ROOTVecOps
     Core
 )
 
-if(builtin_vdt OR vdt)
-   target_include_directories(ROOTVecOps PRIVATE ${VDT_INCLUDE_DIRS} INTERFACE $<BUILD_INTERFACE:${VDT_INCLUDE_DIRS}>)
-endif()
-
 if(builtin_vdt)
-  target_link_libraries(ROOTVecOps PRIVATE ${VDT_LIBRARIES})
+  target_include_directories(ROOTVecOps PUBLIC $<BUILD_INTERFACE:${VDT_INCLUDE_DIRS}>
+                                               $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>)
+  target_link_libraries(ROOTVecOps PUBLIC $<BUILD_INTERFACE:${VDT_LIBRARIES}>
+                                          $<INSTALL_INTERFACE:${CMAKE_INSTALL_LIBDIR}/${CMAKE_STATIC_LIBRARY_PREFIX}vdt${CMAKE_SHARED_LIBRARY_SUFFIX}>)
 elseif(vdt)
+  target_include_directories(ROOTVecOps PUBLIC ${VDT_INCLUDE_DIRS})
   target_link_libraries(ROOTVecOps PUBLIC ${VDT_LIBRARIES})
 endif()

I personally favour option 1, as it should be a bit more robust in the long run in my mind. But option 2 could work as well. Though it makes relocatabiity harder. (If VDT is in a different place after relocation, the ROOTConfig-targets.cmake file now needs to be manually updated as part of the relocation. Which is not great.)

[amadio]

You option 1 is more or less what we used to do, and it doesn't work when Vdt is builtin, because INTERFACE_INCLUDE_DIRECTORIES will be inside the build directory. Your option 2 will likely not work with static external Vdt because of ${CMAKE_STATIC_LIBRARY_PREFIX}vdt${CMAKE_SHARED_LIBRARY_SUFFIX} used in the install interface. I agree with you, however, if your option one can be made to work correctly when Vdt is external/builtin and the several versions of CMake we support, then that would be my preferred option too.

[krasznaa]

😕 My option 2 has this code:

if(builtin_vdt)
  target_include_directories(ROOTVecOps PUBLIC $<BUILD_INTERFACE:${VDT_INCLUDE_DIRS}>
                                               $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>)
  target_link_libraries(ROOTVecOps PUBLIC $<BUILD_INTERFACE:${VDT_LIBRARIES}>
                                          $<INSTALL_INTERFACE:${CMAKE_INSTALL_LIBDIR}/${CMAKE_STATIC_LIBRARY_PREFIX}vdt${CMAKE_SHARED_LIBRARY_SUFFIX}>)
elseif(vdt)
  target_include_directories(ROOTVecOps PUBLIC ${VDT_INCLUDE_DIRS})
  target_link_libraries(ROOTVecOps PUBLIC ${VDT_LIBRARIES})
endif()

Though I disagree that option 1 would not work. The whole point of using imported targets is that CMake would not care about where they point exactly. As they will need to be re-found on the "installed version" of ROOT anyway.

[amadio]

You are welcome to submit a pull request with option 1, and yes, I misread your option 2, I think that should be doable at least as an intermediate fix.

[amadio]

Looking at the history, commit 378f961#diff-b7e4b08bc7e019d35a141d4c27ebc7e748b5f4580d6f9d840dda0c70cc185cbe is actually wrong, it replaced a public dependency on Vdt with a private (and build-time only) dependency. The last time I touched this file was in commit 7d88a0f, when we had something similar to your option 2 in place.

[hageboeck]

I'm totally +1 for using target-based CMake, and I believe I have fixed the issue of ROOT picking up its own headers here:
#8709 (needs rebasing).

This fixed it at least for many builtins. There still might be more builtins that have the same problem, but let's go one step at a time.

I solved the VDT-related part a bit differently. If I rebased, the diff would approximately read (done manually, sorry for possible indentation errors):

            DESTINATION ${CMAKE_INSTALL_INCLUDEDIR} COMPONENT extra-headers)
     set(vdt ON CACHE BOOL "Enabled because builtin_vdt enabled (${vdt_description})" FORCE)
     set_property(GLOBAL APPEND PROPERTY ROOT_BUILTIN_TARGETS VDT)
-    add_library(VDT::VDT STATIC IMPORTED GLOBAL)
-    set_target_properties(VDT::VDT
-      PROPERTIES
-        IMPORTED_LOCATION "${VDT_LIBRARIES}"
-        INTERFACE_INCLUDE_DIRECTORIES "${VDT_INCLUDE_DIRS}"
+       add_library(VDT IMPORTED SHARED)
+      add_dependencies(VDT BUILTIN_VDT)
+      set_target_properties(VDT PROPERTIES IMPORTED_LOCATION "${VDT_LIBRARIES}")
+      target_include_directories(VDT INTERFACE $<BUILD_INTERFACE:${VDT_INCLUDE_DIR}> $<INSTALL_INTERFACE:include/>)
     )
     endif()
   endif()

Whether the target is declared global, static or shared, I'm not sure if it makes a big difference. I'm happy to call it VDT::VDT, though. I think the important part was to switch the include directories via generator expressions, and to use SYSTEM includes instead of includes in FindVDT.

#11844 is missing the usage of the VDT::VDT target in RooFit and in tmva, but that would come into effect after a rebase of #8709.

I guess therefore that we could proceed with merging #11844 if it's green, and then I rebase #8709, so I have to do the rebase work only once.

Does that sound reasonable for @amadio, @krasznaa, @bellenot ?

Edit

And to be clear, for SearchInstalledSoftware I would leave everything as proposed in Attila's commit but the change to target_include_directories with the two generator expressions.

[bellenot]

@hageboeck you and @amadio know CMake much better than me, so any solution which is working for all possible use cases is fine with me...

[github-actions]

Hi @dpiparo, @bellenot,

It appears this issue is closed, but wasn't yet added to a project. Please add upcoming versions that will include the fix, or 'not applicable' otherwise.

Sincerely,
🤖

[github-actions]

Hi @dpiparo, @bellenot,

It appears this issue is closed, but wasn't yet added to a project. Please add upcoming versions that will include the fix, or 'not applicable' otherwise.

Sincerely,
🤖

[github-actions]

Hi @dpiparo, @bellenot,

It appears this issue is closed, but wasn't yet added to a project. Please add upcoming versions that will include the fix, or 'not applicable' otherwise.

Sincerely,
🤖