Skip to content

[CMake] CMake / include fixes for upcoming target-based CMake #19176

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jun 26, 2025

Conversation

hageboeck
Copy link
Member

These are commits cherry-picked from #8709. These should be able to stand on their own, but they will be required for the upcoming changes in #8709.

Concretely, we have here:

  • Moving a few include directives to where they are used.
  • Move setting of OSX_SYSROOT before the project() call as required in the CMake documentation
  • Make an implicit dependency of sofie explicit

The GemmDerivative test was including TInterpreter without linking to
Core. This will start to fail once automatic includes of everything
will be removed in ROOT's CMake.
The test was including nlohmann/json, but RDF's dependency on nlohmann
will be made private in subsequent commits.
Consequently, this include might fail, but it's not used anyway.
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.
According to the documentation, any OSX_SYSROOT needs to be set before
the first project call.
https://cmake.org/cmake/help/latest/variable/CMAKE_OSX_SYSROOT.html

Since the setting came too late, homebrew compilers were missing the
include list for homebrew compilers was generated wrongly.
@hageboeck hageboeck force-pushed the target-basedCMake_part1 branch from bd0cff0 to 6c2aa67 Compare June 25, 2025 13:25
Copy link
Member

@vepadulano vepadulano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@guitargeek
Copy link
Contributor

Good that the failure on alma8 happened, it's only the second time I see it!

So new sporadic confirmed:
#19125

Copy link

github-actions bot commented Jun 26, 2025

Test Results

    20 files      20 suites   3d 8h 44m 32s ⏱️
 3 038 tests  3 038 ✅ 0 💤 0 ❌
59 166 runs  59 166 ✅ 0 💤 0 ❌

Results for commit 6c2aa67.

♻️ This comment has been updated with latest results.

@hageboeck hageboeck merged commit ebc38bd into root-project:master Jun 26, 2025
45 of 46 checks passed
@hageboeck hageboeck deleted the target-basedCMake_part1 branch June 26, 2025 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants