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

[cmake] Only add the Core dependency to dictionaries when Core is a target#23618
bellenot merged 3 commits into
root-project:masterfrom
jmcarcell:fix-core-dict-dependency

Conversation

@jmcarcell

@jmcarcell jmcarcell commented Oct 6, 2026 •
edited
Loading

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 •
edited
Loading

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.

pcanal
pcanal reviewed Oct 6, 2026
View reviewed changes
Comment thread cmake/modules/RootMacros.cmake Outdated
jmcarcell and others added 2 commits October 6, 2026 10:54
@jmcarcell @pcanal
Add always the Core or ROOT::Core dependency
931d14d 
Co-authored-by: Philippe Canal <pcanal@fnal.gov>
@jmcarcell
[cmake] Check ROOT::Core before Core
449f28b 
Prefer the namespaced Core target when both target names are available.

Assisted-by: pi:gpt-6.1-sol
pcanal
pcanal approved these changes Oct 6, 2026
View reviewed changes

@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

jmcarcell commented Oct 6, 2026

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

@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

@andresailer

andresailer commented Oct 7, 2026

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

andresailer commented Oct 7, 2026

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
ferdymercury pushed a commit to ferdymercury/root that referenced this pull request Oct 16, 2026
@jmcarcell @pcanal
[cmake] Only add the Core dependency to dictionaries when Core is a t…
dba8f4a 
…arget (root-project#23618)

* [cmake] Only add the Core dependency to dictionaries when Core is a target

* Add always the Core or ROOT::Core dependency

Co-authored-by: Philippe Canal <pcanal@fnal.gov>

* [cmake] Check ROOT::Core before Core

Prefer the namespaced Core target when both target names are available.

Assisted-by: pi:gpt-6.1-sol

---------

Co-authored-by: Juan Miguel Carceller <jmcarcell@users.noreply.github.com>
Co-authored-by: Philippe Canal <pcanal@fnal.gov>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andresailer andresailer andresailer left review comments
@pcanal pcanal pcanal approved these changes
@bellenot bellenot Awaiting requested review from bellenot bellenot is a code owner
+1 more reviewer
@ferdymercury ferdymercury ferdymercury approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@bellenot bellenot

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jmcarcell @pcanal @andresailer @ferdymercury @bellenot