Skip to content

[cmake] Only add the Core dependency to dictionaries when Core is a target - #23618

Merged
bellenot merged 3 commits into
root-project:masterfrom
jmcarcell:fix-core-dict-dependency
Oct 7, 2026
Merged

bellenot merged 3 commits into
root-project:masterfrom
jmcarcell:fix-core-dict-dependency

Conversation

@jmcarcell

@jmcarcell jmcarcell commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

This Pull request:

Changes or fixes:

#23207 made every dictionary with a MODULE depend on Core by inserting Core into ARG_DEPENDENCIES, which then ends up in the DEPENDS of the rootcling custom command. This works inside ROOT's build, where Core is a target, but downstream projects only have the imported ROOT::Core, so the bare Core is treated as a file and the build fails. For example, when building Garfield++:

make[5]: *** No rule to make target 'Core', needed by 'GarfieldDict.cxx'.
make[5]: Target 'CMakeFiles/GarfieldDict.dir/depend' not remade because of errors.
make[4]: *** [CMakeFiles/Makefile2:232: CMakeFiles/GarfieldDict.dir/all] Error 2

This adds the Core dependency only when Core is a target, so the behaviour inside ROOT is unchanged and downstream projects work as before.

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

This PR fixes a regression introduced in #23207

@jmcarcell
jmcarcell requested a review from bellenot as a code owner October 6, 2026 05:41
@ferdymercury
ferdymercury requested a review from pcanal October 6, 2026 06:25
@github-actions

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Test Results

    24 files      24 suites   4d 1h 29m 37s ⏱️
 3 883 tests  3 880 ✅ 0 💤 3 ❌
83 112 runs  83 108 ✅ 0 💤 4 ❌

For more details on these failures, see this check.

Results for commit 449f28b.

♻️ This comment has been updated with latest results.

Comment thread cmake/modules/RootMacros.cmake Outdated
jmcarcell and others added 2 commits October 6, 2026 10:54
Prefer the namespaced Core target when both target names are available.

Assisted-by: pi:gpt-6.1-sol

@pcanal pcanal left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me.

@bellenot bellenot self-assigned this Oct 6, 2026
@jmcarcell

Copy link
Copy Markdown
Contributor Author

It would be great if this is merged today together with #23626 so that we have dev3 builds tomorrow, a few of the issues we had should have been solved.

@pcanal

pcanal commented Oct 6, 2026

Copy link
Copy Markdown
Member

@andresailer

Copy link
Copy Markdown
Contributor

@linev Could you take a look at the failures of test-stressgui-xvfb on opensuse16: https://github.com/root-project/root/pull/23618/checks?check_run_id=112499176495 and https://github.com/root-project/root/actions/runs/37449769482/job/112280590348?pr=23618

Seems unrelated to this PR: #23632

@andresailer

Copy link
Copy Markdown
Contributor

Could we get a review / merge today so we can see further tomorrow?

@bellenot
bellenot merged commit f365138 into root-project:master Oct 7, 2026
53 of 63 checks passed
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.

5 participants