Skip to content

[roottest] use less ROOTSYS#22032

Merged
guitargeek merged 10 commits intoroot-project:masterfrom
ferdymercury:patch-22
Apr 24, 2026
Merged

[roottest] use less ROOTSYS#22032
guitargeek merged 10 commits intoroot-project:masterfrom
ferdymercury:patch-22

Conversation

@ferdymercury
Copy link
Copy Markdown
Collaborator

No description provided.

@pcanal pcanal self-requested a review April 23, 2026 16:14
@ferdymercury ferdymercury requested a review from guitargeek April 23, 2026 16:46
@ferdymercury ferdymercury marked this pull request as ready for review April 23, 2026 16:53
@ferdymercury ferdymercury requested a review from bellenot as a code owner April 23, 2026 16:53
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 23, 2026

Test Results

    22 files      22 suites   3d 7h 4m 18s ⏱️
 3 850 tests  3 848 ✅  1 💤 1 ❌
76 007 runs  75 988 ✅ 18 💤 1 ❌

For more details on these failures, see this check.

Results for commit eaf1e1b.

♻️ This comment has been updated with latest results.

@dpiparo
Copy link
Copy Markdown
Member

dpiparo commented Apr 24, 2026

This is the right direction to go. Maybe @guitargeek wants to chime in with his packaging expertise, given that in these cases, the coverage guaranteed by the CI is not always enough.

Comment thread roottest/CMakeLists.txt Outdated
Copy link
Copy Markdown
Contributor

@guitargeek guitargeek left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Avoiding to use ROOTSYS as a CMake variable is definitely good to avoid confusion with the environment variable of the same name which I try to get rid of 👍

Can you change the PR though to not introduce new cached variables? You do it here:

set(ROOT_LIBRARY_DIR ${CMAKE_BINARY_DIR}/lib CACHE INTERNAL "")
set(ROOT_INCLUDE_DIRS ${CMAKE_BINARY_DIR}/include CACHE INTERNAL "")

I think the target-based configuration that you suggest yourself would be a way to achieve this.

Comment thread roottest/CMakeLists.txt Outdated
Comment thread cmake/modules/RootConfiguration.cmake Outdated
@ferdymercury ferdymercury requested a review from dpiparo as a code owner April 24, 2026 11:33
Comment thread roottest/CMakeLists.txt
Copy link
Copy Markdown
Contributor

@guitargeek guitargeek left a comment

Choose a reason for hiding this comment

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

Thank you very much! Also for considering the review comments.

@guitargeek guitargeek merged commit dcc3ef1 into root-project:master Apr 24, 2026
31 of 33 checks passed
@guitargeek
Copy link
Copy Markdown
Contributor

guitargeek commented Apr 24, 2026

Ah damn I'm sorry, sometimes the muscle memory skips the squash on merge 🙁 Occasionally that happens, pardon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants