[CMake] Move towards target-based CMake and partly fix picking up headers from an installed ROOT #8709
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1 task
hageboeck
force-pushed
the
target-basedCMake
branch
from
c03017d to
b3d1626
Compare
hageboeck
force-pushed
the
target-basedCMake
branch
3 times, most recently
from
4379a7c to
a567d9e
Compare
hageboeck
referenced
this pull request
Dec 6, 2021
The way this is done was copy-pasted from `graf3d/eve7/CMakeLists.txt`.
hageboeck
force-pushed
the
target-basedCMake
branch
from
a567d9e to
d8d15cc
Compare
hageboeck
force-pushed
the
target-basedCMake
branch
from
fb6f3ec to
1d9d850
Compare
hageboeck
force-pushed
the
target-basedCMake
branch
from
1d9d850 to
54e38cb
Compare
hageboeck
force-pushed
the
target-basedCMake
branch
from
54e38cb to
3af0417
Compare
guitargeek
reviewed
Feb 10, 2023
guitargeek
reviewed
Feb 10, 2023
|
Can the roofit part be split into a dedicated PR / removed here? |
hageboeck
force-pushed
the
target-basedCMake
branch
2 times, most recently
from
15ce452 to
9dc9363
Compare
Yes, partly. I pushed the tutorial part by mistake (and removed it here). The CMake-related things in RF need to stay here. |
Axel-Naumann
approved these changes
Feb 14, 2023
hageboeck
force-pushed
the
target-basedCMake
branch
2 times, most recently
from
e023803 to
ea3c012
Compare
hageboeck
force-pushed
the
target-basedCMake
branch
3 times, most recently
from
d9e0456 to
fc3378b
Compare
hageboeck
force-pushed
the
target-basedCMake
branch
from
fc3378b to
b1cc4ba
Compare
This was
linked to
issues
Jan 27, 2025
hageboeck
force-pushed
the
target-basedCMake
branch
from
fff52af to
7e42cc5
Compare
For an unknown reason, a res/ header was included, leading to an include error. The include could be removed without problems.
gtest.h used to be parasitically included in TestSupport.hxx, which isn't using gtest at all. It's cleaner to include it where it is actually used.
TestSupport.hxx isn't using gtest, so it does not make sense to include it here.
VDT used to be configured using variables, but this creates a problem when VDT is installed in a system directory where ROOT is installed as well. The -I<VDT_LOCATION> will lead to headers from the installed ROOT being picked up during compilation. Here, VDT is configured in a target-based way. When external, it's now an IMPORTED target, and its include directory is marked as SYSTEM, so it will not interfere with other ROOT includes.
- Create the target XRootD::XrdCl for usage in ROOT Create an IMPORTED target for XRootD that we populate either with a system xrootd or with the builtin library. This also solves the problem of ROOT's build system picking up ROOT headers when it is trying to include xrootd from a system directory where both ROOT and xrootd are installed. All packages inside ROOT should use the target instead of any CMake variables. This way, an update of XRootD's CMake will only affect one single location in ROOT. - Refactor builtin XRootD. Synchronise the variables that the builtin and SearchInstalled are setting, and use those to configure the ROOT-internal target. - Add a test for XrdCl headers, since these are used in TNetXNG. If xrootd is installed in the system, the XrdCl library might be present without the corresponding headers. This would lead to a build error in ROOT, so cmake will try to find the headers at configure time to warn about a possibly missing package. - Add a CMAKE_BUILD_RPATH to all ROOT targets in order to find the libraries of a builtin XRootD.
Since ROOT already has SSL support, the logic for builtin XRootD could be simplified by requiring these options to be on.
To help find the builtin openssl location, it is now saved as a cache variable, which is passed to xrootd.
- Create the target nlohmann_json that will configure all dependent libraries. - Replace all uses of json with target_link_libraries(... nlohman_json) - Remove explicit uses of the nlohmann include directory across ROOT. - Add depedency to RooFitCore, which depends on json through exposed json interface.
- Remove the use of variables for lzma, use target_link_libraries instead. - Use the same target name as the CMake module: LibLZMA::LibLZMA
For an unknown reason, ROOT's cmake macros were reading all include directories from the targets they depend on, and doing some manual processing of those. Due to more extensive use of target-based cmake, this manual treatment should become obsolete. This makes the management of include directories simpler, and will allow for better debugging with CMake.
- Collect all dependency and include-related instructions in one place to make it more clear what's going on. - Improve documentation a bit. - Remove manual treatmant of include directories and link configs, as they should now be handled automatically by target_link_libraries.
This means that target_link_libraries has to use the PRIVATE/PUBLIC keyword.
- In case ROOT_GENERATE_DICTIONARY is invoked with a dependency that doesn't have a dictionary itself, the for loop through dependencies now just continue()s. Before, this would raise a CMake error. - The object library with the dictionary file is now linked into the main library using target_link_libraries(). - When the list of include directories for the dictionary is generated, the INTERFACE_SYSTEM_INCLUDE_DIRECTORIES of the dependencies is now honoured. Before, system includes would decay to normal includes. Unfortunately, PRIVATE includes still decay to normal -I includes. This can lead header conflicts when ROOT is built while another ROOT is installed in system include directories, but only for the dictionary files. Since ROOT include directories are very generously prepended to all targets, I wasn't able to provoke a header conflict.
Most unit tests in core/thread were running twice: - Once from ROOT_ADD_UNIT_TEST_DIR, which globs all *.cxx and compiles them into a test - Once more from explicitly registering the tests Here, we register all tests explicitly.
The argument COMPILEMACROS isn't used anywhere, so it can be removed.
- Remove own FindGTest, use the one from CMake. Starting from CMake 3.23, the GTest libraries have canonical names. - Replace uses of legacy targets like "gtest" in ROOT with canonical target names such as GTest::gtest or GTest::gtest_main. - Create ALIAS targets like in CMake 3.23 such as GTest::gtest_main. For CMake < 3.23, this will allow using the standard FindGTest with the modern names already in cmake 3.20.
Explicitly list GTest::gmock in core.
RooFit multiprocess privately depends on nlohmann_json. The res headers use it too, but since the dependency is private, the tests cannot use it unless they explicitly request nlohmann_json, too.
hageboeck
force-pushed
the
target-basedCMake
branch
from
7e42cc5 to
164600c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Partial fix of #8708.
In a setup where ROOT was installed in a system directory, ROOT was picking up headers from that directory instead of its own.
How to reproduce:
echo '#error This is the wrong header' > /my/include/directory/RooSpan.h(or a few other headers).cmake -DCMAKE_PREFIX_PATH=/my/include/directory/ <root>to create a dependency to that include directory.The problem only becomes visible when
Adepends onB and C, andBdepends on something in/system/include/, andCis installed in those system includes as well. This generates a compile command such as:In this PR:
IMPORTED SYSTEMtargets, so their includes have lowest precedence.-isystem(https://its.cern.ch/jira/browse/ROOT-10414)What remains to be done:
It is likely that more builtins (or rather
FindXXXhave to be converted toIMPORTEDtargets, so they don't provoke this error again. A broken configuration can be detected by#error-ROOT headers in there orcompile_commands.jsonfor-I/my/include/directory/FindXXXfor this dependency.