[CMake] Don't use find_package(ROOT REQUIRED) in `test/` subdirectory #20640

guitargeek · 2025-12-04T09:34:05Z

As Sergey said in the comment in test/CMakeLists.txt, this test
project stopped working standalone. But, it still uses
find_package(ROOT) to find ROOT, even though it's not installed yet as
we are building ROOT itself at the same time.

Apparently that works, and sets the flags like ROOT_opengl_FOUND, but
we should not rely on this and use the build-time configuration flags
instead.

vepadulano · 2025-12-04T10:17:59Z

cmake/modules/RootNewMacros.cmake

Is it necessary to remove this file? Could it be possible that someone in the world is still using it? Should we perhaps change the deprecation message to specify a ROOT release when this will be removed?

Not strictly necessary, but it's a bit confusing to keep as it's called RootNewMacros.cmake, with "New" in the name, but deprecated and replaced by RootMacros.cmake without "New". I would like to keep confusing situations like this behind for good.

Users will have seen the deprecation message for 6 years now. I think people will understand if we remove it now.

I you think this is controversial, I can bring it forward in a separate PR!

Geant4 still uses it: https://github.com/Geant4/geant4/blob/master/examples/extended/persistency/P01/CMakeLists.txt#L39
https://github.com/Geant4/geant4/blob/master/examples/advanced/CaTS/CMakeLists.txt#L53

...only if it exists 🙂 Otherwise, they automatically get the new RootMacros.cmake via the find_package(ROOT REQUIRED) before in the file. So they actually to the right thing, to even be backwards compatible with ROOT before 6.20.

Oh right, sorry, I overlooked at it...

github-actions · 2025-12-04T19:54:04Z

Test Results

22 files 22 suites 3d 19h 46m 25s ⏱️
3 784 tests 3 784 ✅ 0 💤 0 ❌
81 227 runs 81 227 ✅ 0 💤 0 ❌

Results for commit 994c94c.

♻️ This comment has been updated with latest results.

pcanal · 2025-12-04T23:26:45Z

test/CMakeLists.txt

-# This shows nicely how to compile and link applications
-# using the ROOT libraries on all supported platforms.


Can we update the PR description and commit log on where we now document and/or demonstrate the modern way to build you application that uses ROOT?

Well, anywhere where we use ROOT_EXECUTABLE or ROOT_STANDARD_LIBRARY_PACKAGE I guess? I don't see what made this test/CMakeLists.txt a good place "to show nicely how to compile and link applications using the ROOT libraries on all supported platforms". That comment seemed pretty random to me.

Rather than random, it is historical. Once upon a time, this was a tidy smallish standalone (Makefile then CMake) example that build dictionaries, libraries and executables that use ROOT. I can see that over time we drifted away from that (seemingly breaking the standalone part and getting a bit complex). However unless (but I assume we actually do have one) we have a replacement documentation, then this would be the closest we have to a documentation. (If we don't have the documentation and nice examples, then this PR can still proceed but we need to open an issue to remind us to write that documentation).

Well, anywhere where we use ROOT_EXECUTABLE or ROOT_STANDARD_LIBRARY_PACKAGE

Humm .. that does not sound enough, it also needs at least the call to find_package(ROOT) and a good set of usage of the CMake function we recommend using. i.e. enough for a CMake beginner to get started.

Okay, I see now! The entry-point documentation for using ROOT via CMake is here now:
https://root.cern/manual/integrate_root_into_my_cmake_project/#full-example-event-project

Do you see anything missing from there? I think it's good that we advocate the custom macros like ROOT_EXECTUABLE too much, and instead show how to use standard CMake, just like the example on the website does.

pcanal · 2025-12-04T23:28:00Z

test/CMakeLists.txt

+  if(mathmore)
+    list(APPEND stress_hist_fit_extra_libs MathMore)
+  endif()


Why is this now needed?

Because of this conditional include here:
https://github.com/root-project/root/blob/master/math/mathcore/inc/Math/DistFunc.h#L39

But let me try to replace this include

That include was already there before this PR, so why don't we need the dependency before ...

Same reason as #20640 (comment)

pcanal · 2025-12-04T23:28:27Z

test/CMakeLists.txt

 ROOT_GENERATE_DICTIONARY(G__TBench ${CMAKE_CURRENT_SOURCE_DIR}/TBench.h MODULE TBench LINKDEF benchLinkDef.h DEPENDENCIES MathCore Tree)
 ROOT_LINKER_LIBRARY(TBench TBench.cxx G__TBench.cxx LIBRARIES Core MathCore RIO Tree)
-ROOT_EXECUTABLE(bench bench.cxx LIBRARIES Core TBench)
+ROOT_EXECUTABLE(bench bench.cxx LIBRARIES Core TBench RIO)


Why is RIO needed/wanted when it wasn't before?

Before, we implicitly linked all main ROOT libraries via find_package(ROOT REQUIRED). But I want to avoid calling that when building ROOT itself, as that seems pretty weird to me. So now, we have to link all required libraries explicitly.

But I want to avoid calling that when building ROOT itself

+1

we implicitly linked all main ROOT libraries via find_package(ROOT REQUIRED)

humm ... what? ... If I read that correctly (probably didn't), this would be mean that for any user of find_package(ROOT) "all" their libraries would linked against libCore and friends? i.e. Can you remind what is the side effect of find_package that make the dependency on RIO (and apparently mathMore) optional/redundant?

You are right, that sounds like it can't be true. I looked into it again, and I think I found the real reason:
https://github.com/root-project/root/blob/master/cmake/modules/RootMacros.cmake#L1420

if(NOT (PROJECT_NAME STREQUAL "ROOT")) # only for non-ROOT executable use $ROOTSYS/include include_directories(BEFORE ${CMAKE_BINARY_DIR}/include)

So it was the removal of project(test) in this PR that resulted ${CMAKE_BINARY_DIR}/include in not being appended anymore, and this is why the include directories need to be explicitly added per package via the CMake target_link_library mechanism.

Has been deprecated since ROOT 6.20 and can now be removed.

As Sergey said in the comment in `test/CMakeLists.txt`, this test project stopped working standalone. But, it still uses `find_package(ROOT)` to find ROOT, even though it's not installed yet as we are building ROOT itself at the same time. Apparently that works, and sets the flags like `ROOT_opengl_FOUND`, but we should not rely on this and use the build-time configuration flags instead.

guitargeek · 2025-12-05T22:54:18Z

@pcanal, I addressed all your inline comments

guitargeek self-assigned this Dec 4, 2025

guitargeek requested a review from bellenot as a code owner December 4, 2025 09:34

guitargeek added in:Build System clean build Ask CI to do non-incremental build on PR labels Dec 4, 2025

guitargeek force-pushed the roottest_cmake branch from dfdc11a to 62d7351 Compare December 4, 2025 10:04

vepadulano reviewed Dec 4, 2025

View reviewed changes

pcanal reviewed Dec 4, 2025

View reviewed changes

[CMake] Remove deprecated RootNewMacros.cmake

36a4e01

Has been deprecated since ROOT 6.20 and can now be removed.

guitargeek force-pushed the roottest_cmake branch from 62d7351 to c2d7c6c Compare December 4, 2025 23:52

guitargeek requested a review from dpiparo as a code owner December 4, 2025 23:52

guitargeek force-pushed the roottest_cmake branch from c2d7c6c to 994c94c Compare December 5, 2025 08:18

		# This shows nicely how to compile and link applications
		# using the ROOT libraries on all supported platforms.

[CMake] Don't use find_package(ROOT REQUIRED) in test/ subdirectory #20640

Are you sure you want to change the base?

[CMake] Don't use find_package(ROOT REQUIRED) in test/ subdirectory #20640

Conversation

guitargeek commented Dec 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pcanal Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guitargeek commented Dec 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[CMake] Don't use find_package(ROOT REQUIRED) in `test/` subdirectory #20640

[CMake] Don't use find_package(ROOT REQUIRED) in `test/` subdirectory #20640

github-actions bot commented Dec 4, 2025 •

edited

Loading

pcanal Dec 4, 2025 •

edited

Loading