[CMake] Don't use find_package(ROOT REQUIRED) in test/ subdirectory
#20640
Conversation
guitargeek
added
in:Build System
clean build
guitargeek
force-pushed
the
roottest_cmake
branch
from
dfdc11a to
62d7351
Compare
cmake/modules/RootNewMacros.cmake
Outdated
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I you think this is controversial, I can bring it forward in a separate PR!
There was a problem hiding this comment.
There was a problem hiding this comment.
...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.
There was a problem hiding this comment.
Oh right, sorry, I overlooked at it...
Test Results 22 files 22 suites 3d 19h 46m 25s ⏱️ Results for commit 994c94c. ♻️ This comment has been updated with latest results. |
| # This shows nicely how to compile and link applications | ||
| # using the ROOT libraries on all supported platforms. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That looks fine enough (although it 'requires' the information provide by Henry Schreiner. It is linked from our documentation but we depends on that linked page staying up.
Has been deprecated since ROOT 6.20 and can now be removed.
guitargeek
force-pushed
the
roottest_cmake
branch
from
62d7351 to
c2d7c6c
Compare
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
force-pushed
the
roottest_cmake
branch
from
c2d7c6c to
994c94c
Compare
|
@pcanal, I addressed all your inline comments |
4 participants
As Sergey said in the comment in
test/CMakeLists.txt, this testproject stopped working standalone. But, it still uses
find_package(ROOT)to find ROOT, even though it's not installed yet aswe are building ROOT itself at the same time.
Apparently that works, and sets the flags like
ROOT_opengl_FOUND, butwe should not rely on this and use the build-time configuration flags
instead.