Use ROOT_MAIN_{SOURCE,BINARY}_DIR instead of CMAKE_{SOURCE, BINARY}_DIR. NFC #18130

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

Jump to bottom

Open

vgvassilev wants to merge 1 commit into root-project:master

from vgvassilev:prepare-for-superbuilds

Open

Use ROOT_MAIN_{SOURCE,BINARY}_DIR instead of CMAKE_{SOURCE, BINARY}_DIR. NFC #18130

vgvassilev wants to merge 1 commit into root-project:master from vgvassilev:prepare-for-superbuilds

Conversation

vgvassilev

Copy link

Member

@vgvassilev vgvassilev commented Mar 25, 2025 •

edited

Loading

This patch renames CMAKE_SOURCE_DIR to ROOT_MAIN_SOURCE_DIR and CMAKE_BINARY_DIR to ROOT_MAIN_BINARY_DIR.

The change unblocks progress on PR #16751 and #16211 by allowing them to disentangle the build step of building a user-specified set of components after another which was already built at an earlier stage.

Supersedes: #17746.

@vgvassilev vgvassilev requested review from bellenot and dpiparo as code owners

March 25, 2025 14:05

@vgvassilev vgvassilev requested review from bellenot, dpiparo and guitargeek and removed request for bellenot and dpiparo

March 25, 2025 14:06

@vgvassilev vgvassilev force-pushed the prepare-for-superbuilds branch from f1d6c1d to 1f217a4 Compare

March 25, 2025 14:06

@github-actions GitHub Actions

Copy link

github-actions bot commented Mar 25, 2025 •

edited

Loading

Test Results

18 files 18 suites 4d 5h 42m 9s ⏱️
2 737 tests 2 737 ✅ 0 💤 0 ❌
47 518 runs 47 518 ✅ 0 💤 0 ❌

Results for commit a092ad5.

♻️ This comment has been updated with latest results.

@vgvassilev vgvassilev force-pushed the prepare-for-superbuilds branch from 1f217a4 to fda5934 Compare

March 25, 2025 17:35

@hahnjo

Copy link

Member

hahnjo commented Mar 26, 2025

In many places, can we instead use CMAKE_CURRENT_{SOURCE,BINARY}_DIR which I believe is the standard CMake way of approaching this issue? Also you cannot just change interpreter/CppInterOp in the ROOT source directory, the change needs to be committed upstream first and then synchronized via a pinned commit hash.

@vgvassilev vgvassilev force-pushed the prepare-for-superbuilds branch from fda5934 to f575ca6 Compare

March 26, 2025 09:20

@vgvassilev


 Use ROOT_MAIN_{SOURCE,BINARY}_DIR instead of CMAKE_{SOURCE, BINARY}_D...

a092ad5

...IR. NFC
This patch renames `CMAKE_SOURCE_DIR` to `ROOT_MAIN_SOURCE_DIR` and
`CMAKE_BINARY_DIR` to `ROOT_MAIN_BINARY_DIR`.
The change unblocks progress on PR root-project#16751 and root-project#16211 by allowing them to
disentangle the build step of building a user-specified set of components after
another which was already built at an earlier stage.
Supersedes: root-project#17746.

@vgvassilev vgvassilev force-pushed the prepare-for-superbuilds branch from f575ca6 to a092ad5 Compare

March 26, 2025 10:58

@vgvassilev

Copy link

Member Author

vgvassilev commented Mar 26, 2025

In many places, can we instead use CMAKE_CURRENT_{SOURCE,BINARY}_DIR which I believe is the standard CMake way of approaching this issue?

I intended this for a quick nfc. If you can comment on where you think this should be done I can give it a shot (perhaps in a separate pr)?

Also you cannot just change interpreter/CppInterOp in the ROOT source directory, the change needs to be committed upstream first and then synchronized via a pinned commit hash.

Fixed, probably we don't need that change there.

@devajithvs devajithvs assigned vgvassilev

Apr 3, 2025

hahnjo

hahnjo reviewed

Apr 4, 2025

View reviewed changes

CMakeLists.txt

Comment on lines +53 to +55

set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${ROOT_MAIN_BINARY_DIR}/lib)

set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${ROOT_MAIN_BINARY_DIR}/lib)

set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${ROOT_MAIN_BINARY_DIR}/bin)

Copy link

Member

@hahnjo hahnjo Apr 4, 2025

Choose a reason for hiding this comment

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

In the top-level CMakeLists.txt I think we should use CMAKE_PROJECT_* after project(ROOT).

Suggested change

set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${ROOT_MAIN_BINARY_DIR}/lib)

set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${ROOT_MAIN_BINARY_DIR}/lib)

set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${ROOT_MAIN_BINARY_DIR}/bin)

set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_PROJECT_BINARY_DIR}/lib)

set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_PROJECT_BINARY_DIR}/lib)

set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_PROJECT_BINARY_DIR}/bin)

Copy link

Member Author

@vgvassilev vgvassilev Apr 4, 2025

Choose a reason for hiding this comment

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

Ok, even longer. Assuming everybody agrees with that suggestion I will do the mass rename.

bindings/pyroot/cppyy/cppyy-backend/cling/python/cppyy_backend/cmake/FindCppyy.cmake

# ReadMe file

if("${ARG_README_FILE}" STREQUAL "")

set(ARG_README_FILE ${CMAKE_SOURCE_DIR}/README.rst)

set(ARG_README_FILE ${ROOT_MAIN_SOURCE_DIR}/README.rst)

Copy link

Member

@hahnjo hahnjo Apr 4, 2025

Choose a reason for hiding this comment

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

This path is likely wrong, it should probably be something inside bindings/pyroot/cppyy/...

builtins/nlohmann/CMakeLists.txt

Comment on lines +16 to +17

COMMAND ${CMAKE_COMMAND} -E copy ${ROOT_MAIN_SOURCE_DIR}/builtins/nlohmann/json.hpp ${ROOT_MAIN_BINARY_DIR}/include/nlohmann/json.hpp

DEPENDS ${ROOT_MAIN_SOURCE_DIR}/builtins/nlohmann/json.hpp)

Copy link

Member

@hahnjo hahnjo Apr 4, 2025

Choose a reason for hiding this comment

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

Suggested change

COMMAND ${CMAKE_COMMAND} -E copy ${ROOT_MAIN_SOURCE_DIR}/builtins/nlohmann/json.hpp ${ROOT_MAIN_BINARY_DIR}/include/nlohmann/json.hpp

DEPENDS ${ROOT_MAIN_SOURCE_DIR}/builtins/nlohmann/json.hpp)

COMMAND ${CMAKE_COMMAND} -E copy ${CMAKE_CURRENT_SOURCE_DIR}/json.hpp ${ROOT_MAIN_BINARY_DIR}/include/nlohmann/json.hpp

DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/json.hpp)

Labels

None yet

2 participants

@vgvassilev @hahnjo

Use ROOT_MAIN_{SOURCE,BINARY}_DIR instead of CMAKE_{SOURCE, BINARY}_DIR. NFC #18130

Are you sure you want to change the base?

Use ROOT_MAIN_{SOURCE,BINARY}_DIR instead of CMAKE_{SOURCE, BINARY}_DIR. NFC #18130

Uh oh!

Conversation

@vgvassilev vgvassilev commented Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

hahnjo commented Mar 26, 2025

Uh oh!

vgvassilev commented Mar 26, 2025

Uh oh!

@hahnjo hahnjo Apr 4, 2025

Choose a reason for hiding this comment

Uh oh!

@vgvassilev vgvassilev Apr 4, 2025

Choose a reason for hiding this comment

Uh oh!

@hahnjo hahnjo Apr 4, 2025

Choose a reason for hiding this comment

Uh oh!

@hahnjo hahnjo Apr 4, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

@vgvassilev vgvassilev commented Mar 25, 2025 •

edited

Loading

github-actions bot commented Mar 25, 2025 •

edited

Loading