Make ACTSVG setup CMake variable less Explicit

[beomki-yeo]

It seems there is an UB in CMake config.

cmake ../detray -DDETRAY_BUILD_CUDA=ON -DCMAKE_BUILD_TYPE=Release
cmake ../detray -DDETRAY_BUILD_BENCHMARKS=ON

The first command runs OK but when I run the second one right after the first one I see the following error:

-- Detecting CUDA compile features - done
-- Configuring done (8.6s)
CMake Error at plugins/svgtools/CMakeLists.txt:17 (target_link_libraries):
  The link interface of target "detray_svgtools" contains:

    actsvg::core

  but the target was not found.  Possible reasons include:

    * There is a typo in the target name.
    * A find_package call is missing for an IMPORTED target.
    * An ALIAS target is missing.

-- Generating done (0.6s)
CMake Generate step failed.  Build files cannot be regenerated correctly.

I think it is because the set commmand does not use CACHE

[beomki-yeo]

I am not sure if this a right approach, and the change seems not even working with -DDETRAY_BUILD_TUTORIALS=ON. I will appreciate for any advice

[sonarcloud[bot]]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[stephenswat]

This is actually not undefined behaviour, it is working exactly as expected. The following sequence of actions works:

$ cmake ../detray -DDETRAY_BUILD_CUDA=ON -DCMAKE_BUILD_TYPE=Release
$ cmake ../detray -DDETRAY_BUILD_BENCHMARKS=ON -DDETRAY_SETUP_ACTSVG=ON

If you do not add DETRAY_BUILD_BENCHMARKS to the first command, you turn off DETRAY_SETUP_ACTSVG. You cannot then turn it back on because it is a cache variable, unless you explicitly tell CMake to do so.

[beomki-yeo]

You are right but i think it is more convenient to skip the svg build flag if possible. I believe that figuring out whether to build svg or not automatically is what is intended in the current CMakeLists.txt:

# Test utilities are needed for tests, tutorials and benchmarks
if ( DETRAY_BUILD_TESTING OR DETRAY_BUILD_BENCHMARKS OR DETRAY_BUILD_TUTORIALS )
   set( DETRAY_BUILD_TEST_UTILS ON )
endif()

# Svg display is needed for the test utilities
if ( DETRAY_BUILD_TEST_UTILS )
   set( DETRAY_SVG_DISPLAY ON )
endif()

The current change enables to skip that flag by removing DETRAY_SETUP_ACTSVG which does not seem to be very necessary :thinking:

[stephenswat]

No, this is certainly not the way to solve it because it will break any projects using detray.

If you want to solve this problem, I recommend using the PROJECT_IS_TOP_LEVEL (https://cmake.org/cmake/help/latest/variable/PROJECT_IS_TOP_LEVEL.html) variable, and then setting up actsvg if either DETRAY_SETUP_ACTSVG is true, or if PROJECT_IS_TOP_LEVEL is true, DETRAY_SVG_DISPLAY is true, and actsvg has not yet been setup.

[beomki-yeo]

No, this is certainly not the way to solve it because it will break any projects using detray.

Apology for my ignorance. Could you explain how it is going to break the downstream project?

[stephenswat]

Could you explain how it is going to break the downstream project?

Of course; to understand how this works, we need to see that detray is used as a dependency in two different ways. The first way is using the so-called install interface which is the way software is used normally. You build the software and then install it somewhere. After that, you can use it like any other installed software. The other way detray is used is through "FetchContent", which is a way in which CMake allows one project to download another project and use it.

They sound similar, but they work completely differently. The install interface uses a super simplified version of the CMake configuration (see https://github.com/acts-project/detray/blob/main/cmake/detray-config.cmake.in); that makes sense because an installed project doesn't need to know how to build its own tests and so forth. The FetchContent method, on the other hand, doesn't use the install interface at all. Instead, it simply includes the whole CMake build system of the dependent project, including all the complex CMake setup that it might have.

The problem is that we need to support both interfaces; so we need to provide that install file, but the main CMake build system also has to be robust enough that if we use it through FetchContent, we don't collide with the parent project. Because of course when you FetchContent, you mix up all the variables between the two build systems.

So now imagine if Acts uses ActSVG, and Acts also depends on Detray which uses ActSVG; if that were to happen, the build system would need to download Detray twice which would probably crash the build, or otherwise cause some problems. The way we solve this problem is by having the DETRAY_SETUP_ACTSVG flag. So the Acts projects sets up ActSVG and then sets up Detray with the DETRAY_SETUP_ACTSVG flag, so Detray doesn't try to do the same and cause a collision.

The PROJECT_IS_TOP_LEVEL flag is useful here because it allows you to tell if you are building Detray as the "top level project" or if Detray is a dependency of something else.

[beomki-yeo]

Thanks for the explanation. It makes more sense now.

So now imagine if Acts uses ActSVG, and Acts also depends on Detray which uses ActSVG; if that were to happen, the build system would need to download Detray twice which would probably crash the build, or otherwise cause some problems.

Yes or no. In general, when detray is included in ACTS, detray will turn off its ACTSVG build, because we will not want to build detray TESTS, BENCHMARKS and TUTORIALS. Thus, what you worry about is not supposed to happen

A problem that you mentioned will happen when we want to turn on DETRAY_BUILD_TEST_UTILS independently in the downstream projects, because it will require ACTSVG library. This is actually what is happening in the traccc as you can see in extern/detray/CMakeLists.txt:

set( DETRAY_BUILD_UNITTESTS FALSE CACHE BOOL
   "Turn off the build of the Detray unit tests" )
set( DETRAY_BUILD_INTEGRATIONTESTS FALSE CACHE BOOL
   "Turn off the build of the Detray integration tests" )
set( DETRAY_BUILD_BENCHMARKS FALSE CACHE BOOL
   "Turn off the build of the Detray benchmarks" )
set( DETRAY_BUILD_TUTORIALS FALSE CACHE BOOL
   "Turn off the build of the Detray tutorials" )
set( DETRAY_EIGEN_PLUGIN TRUE CACHE BOOL
   "Turn on the build of the Detray Eigen code" )
set( DETRAY_VC_PLUGIN TRUE CACHE BOOL
   "Turn on the build of the Detray Vc code" )

# Needed for 'performance', 'simulation', 'examples', 'tests' and 'benchmarks'
if( TRACCC_BUILD_IO OR TRACCC_BUILD_EXAMPLES OR (BUILD_TESTING AND TRACCC_BUILD_TESTING) OR TRACCC_BUILD_BENCHMARKS )
   set( DETRAY_BUILD_TEST_UTILS TRUE CACHE BOOL
      "Turn on the build of the Detray tests utilities if needed" )
else()
   set( DETRAY_BUILD_TEST_UTILS FALSE CACHE BOOL
      "Turn off the build of the Detray tests utilities" )
endif()

But do we ever want to do this in ACTS? if so, isn't it better to set DETRAY_BUILD_TEST_UTILS and DETRAY_SETUP_ACTSVG ON by default instead of making them dependent on DETRAY_BUILD_TESTING, DETRAY_BUILD_BENCHMARKS and DETRAY_BUILD_TUTORIALS?

Therefore, I suggest building TEST_UTILS Library by default and turn on DETRAY_SETUP_ACTSVG by default. Of course we will change the name of TEST_UTILS to UTILS or so :thinking:

# Set up ACTSVG.
option( DETRAY_SETUP_ACTSVG
   "Set up the rocThrust target(s) explicitly" TRUE )

By doing this, we can freely build TEST_UTILS library in ACTS without duplicating the same library

[beomki-yeo]

The PROJECT_IS_TOP_LEVEL flag is useful here because it allows you to tell if you are building Detray as the "top level project" or if Detray is a dependency of something else.

Using this would be good as a last resort. But It feels like that CMake setup is a bit twisted in my opinion.

[niermann999]

But do we ever want to do this in ACTS?

Yes, that was the main motivation for the test_utils library actually. I would like to see if I can reuse the detay validation utils in the ACTS examples where the validation of the detray plugin has started. For example, the data-recording/consistency checking during the detray propagation already exists in our test code and might not have to be duplicated in ACTS. Possibly, I can also reuse the of comparison of truth helix traces with the detray navigation, but this time to compare ACTS/Geant4 traces with detray. Once the navigation of either project does not find a surface, an svg will be dropped automatically which shows both traces in relation to the detector geometry. This was very helpful in debugging the detray standalone navigation already...

[beomki-yeo]

If so, doesn't it make sense to you promote test_utils to mandatory library?

[niermann999]

If so, doesn't it make sense to you promote test_utils to mandatory library?

I wasn't so sure, if client projects who do not want to depend on the detray test code or actsvg at all should be mandated to build it anyway?

[beomki-yeo]

Is it a big problem if the client does not need test_utils or svg? If we really hate this then we need to consider using the PROJECT_IS_TOP_LEVEL flag. (And I think utils was a mandatory library in the past.)

It is not directly related to this discussion, the current setup is alrady weird. Look at the Stephen's suggestion: $ cmake ../detray -DDETRAY_BUILD_BENCHMARKS=ON -DDETRAY_SETUP_ACTSVG=ON I have to ask myself why I have to turn on SVG when I only want to build BENCHMARK which does not require SVG at all.

[niermann999]

I have to ask myself why I have to turn on SVG when I only want to build BENCHMARK which does not require SVG at all.

Yes, that's true. test_utils is probably a bit too big and clunky. We could split it into (several) smaller targets. But please don't call one of them just utils and put it in the top level directory again, that was a bit confusing with the things included as "detray/utils/...", which of course live in core/utils :)

[beomki-yeo]

No worries