Target based c make #219
Open
Target based c make #219
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
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.
|
please test for CMSSW_15_1_ROOT6_X |
|
A new Pull Request was created by @smuzaffar for branch cms/master/dd0680311b. @iarspider, @smuzaffar can you please review it and eventually sign? Thanks. |
|
cms-bot internal usage |
|
-1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-1860d9/44688/summary.html External BuildI found compilation warning when building: See details on the summary page. |
cmsbuild
added
tests-rejected
tests-started
and removed
tests-started
tests-rejected
labels
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.
No description provided.