-
Notifications
You must be signed in to change notification settings - Fork 51
CMake: Install toml11 and nlohmann_json for inclusion in downstream code #1757
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
CMake: Install toml11 and nlohmann_json for inclusion in downstream code #1757
Conversation
0a0ef54
to
c3d0e0d
Compare
@franzpoeschel Please take care that dependency fetching from the web is on some systems not possible due to restrictions that login nodes or compute nodes are not allowed to download things from the web. |
Dependency fetching is already part of our build system, this PR does not change that, but rather adds an install step. Offline builds are fully supported (and we use them for distribution via package managers), see CMake variables:
|
install(CODE " | ||
execute_process( | ||
COMMAND ${CMAKE_COMMAND} \ | ||
--build \"${CMAKE_BINARY_DIR}\" \ | ||
--target fetchednlohmann_json-install | ||
) | ||
") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far, I have intentionally turned the install of sub-projects off. Instead of this code, we could set JSON_Install ON
:
https://github.com/nlohmann/json/blob/v3.12.0/CMakeLists.txt#L48
install(CODE " | ||
execute_process( | ||
COMMAND ${CMAKE_COMMAND} \ | ||
--build \"${CMAKE_BINARY_DIR}\" \ | ||
--target fetchedtoml11-install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TOML11_INSTALL
is the equivalent option there:
https://github.com/ToruNiina/toml11/blob/v4.4.0/CMakeLists.txt#L27-L28
install(CODE " | ||
execute_process( | ||
COMMAND ${CMAKE_COMMAND} \ | ||
--build \"${CMAKE_BINARY_DIR}\" \ | ||
--target fetchednlohmann_json-install | ||
) | ||
") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Different question: Why would you hack together a manual installation here? Isn't it conceptually closer to what you want to express to link to json/toml11 publicly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See Axel's comment above; this did not work because I was not aware about CMake options in nlohmann-json/toml11 which we set to suppress installations.
I'm only leaving this PR open rn because we might in future add nlohmann-json/toml11 functionality to our public API, in which case we would need to install these libraries somehow along with the main library. There is at the moment no concrete plan to work on this further.
Closing this for now, might reuse the code later. |
This is an attempt at finding a solution for this issue ComputationalRadiationPhysics/picongpu#5355
Problem: If a downstream code wants to use nlohmann_json / toml11 as well, then it must use the same version used also in openPMD, otherwise it might happen that the same symbol defined by toml11/nlohmann_json might exist in different implementations (different versions) in libopenPMD.so and the compiled binary of the downstream. The linker might then jump between implementations, causing undefined behavior.
Attempted solution in this PR:
Alternatives:
TODO:
inline namespace