Skip to content

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vgvassilev
Copy link
Member

@vgvassilev vgvassilev commented Mar 25, 2025

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 guitargeek, dpiparo and bellenot 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
Copy link

github-actions bot commented Mar 25, 2025

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
…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

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.

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

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

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.

endif()
set(LICENSE_FILE ${ARG_LICENSE_FILE})

# 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

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/...

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

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)

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.

2 participants