C++ classes with JSON serialization/deserialization - #277
Conversation
|
I had to rebase this branch, you can use the following to update your local branch instead of |
Luthaf
left a comment
There was a problem hiding this comment.
The different fields in the new struct should have some documentation, and the new structs should be added to the sphinx documentation as well
|
As discussed offline, I did some refactoring allowing to use a default constructor while still forcing the user to initialize all fields (using AI). |
I went with downloading the archive, which is also the recommended method on their README. |
| set(JSON_BuildTests OFF CACHE INTERNAL "") | ||
| set(JSON_Install ON CACHE INTERNAL "") | ||
|
|
||
| FetchContent_MakeAvailable(nlohmann_json) |
There was a problem hiding this comment.
Trying to make sure I understand everything. This will install the library when doing make install in such a way that it can later be found by metatomic-config.cmake? In a scenario where someone first configure-build-install metatomic; and then tries to configure a package that depends on metatomic cmake and have it use the installed version?
There was a problem hiding this comment.
Yes, if nlohmann_json is downloaded and built by metatomic-core it gets installed under metatomic-core/usr/include/nlohmann and it is correctly found by find_dependency(nlohmann_json 3.11.0) in metatomic-config.cmake.
If it is found externally (provided by the system, provided by Spack, etc), then find_dependency(nlohmann_json 3.11.0) will also need to find the external one when configuring a package that depends on metatomic.
What we might add (useful for Spack), is a way to force one behavior or the other, something like METATOMIC_FORCE_EXTERNAL_JSON/METATOMIC_FORCE_INTERNAL_JSON.
There was a problem hiding this comment.
Makes sense. Maybe METATOMIC_DOWNLOAD_JSON, which default to ON if we can not find nlohmann_json? Although I remember hitting some issue with the fact that find_package(nlohmann_json) would create the corresponding targets, and if the user then forces downloading anyway the second target creation would fail
There was a problem hiding this comment.
ok, I kinda need this branch to finish the System API, so I'll merge in the current state and we can add this option as a follow up!
This PR add C++ classes matching what is available in Rust for convenience, allowing automated JSON serialization/deserialization.
Contributor (creator of pull-request) checklist
Reviewer checklist
📚 Download documentation for this pull-request
⚙️ Download Python wheels for this pull-request (you can install these with pip)