-
Couldn't load subscription status.
- Fork 225
[WIP] toml11/nlohmann-json: avoid symbol clashes with different versions in upstream dependencies #5367
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
base: dev
Are you sure you want to change the base?
Conversation
|
Just ran a test, the segfault that I saw is fixed by this. |
|
I was just going to fix this for nlohmann-json, but noticed that it has already been fixed with version 3.11 nlohmann/json#3590. Internally, we still use version 3.9.1. I will remove my workaround and upgrade the single header instead. |
7436f3f to
f1007d3
Compare
|
Upgrading nlohmann_json is not entirely trivial, since #4812 apparently added the library also to targets compiled by the device compiler (it had previously been provided only to the host compiler since nlohmann_json does not support NVIDIA compilers and there had been other issues in the past). This again lets us run into this issue that nlohmann_json has with some versions of NVCC. There is now a draft to add minimal support for NVCC on nlohmann/json#4796, but this particular issue will not be fixed as it seems to be a compiler/STL implementation bug. CI currently failing due to 403 errors, will try later. |
6f1bf4f to
2ed37ef
Compare
|
@franzpoeschel ~I am wondering why there are toml changes in this PR, I thought it is about json only?~~ #5367 (comment) explain my question |
|
@franzpoeschel could you commit the toml and json update as generic user and all cmake changes within a seperate commit. |
| # https://github.com/ComputationalRadiationPhysics/picongpu/pull/5367 | ||
| # | ||
| # This script helps applying the patch (manual intervention likely needed). | ||
|
|
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.
Uhh, I was not aware that we change the thirt party code.
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.
It is a temporary patch until this fix is part of a toml11 release. I do not want to wait for a release since this behavior causes actual crashes, and this is the only way to deploy a bugfix without having to wait.
The script for nlohmann_json can be removed though.
|
why do we not use the openPMD shipped json library if a target |
openPMD does not ship the JSON library, it just uses it internally, but does not install it. With the current setup, any consumer of openPMD should not even notice in its build system that the library has been used. This PR adds fixes to restore this behavior. openPMD/openPMD-api#1757 adds an option to install the internal nlohmann-json and toml11 alongside openPMD, but we cannot rely on that in PIConGPU. |
The updated JSON tree is already on a separate commit with Niels Lohmann as an author: 59213d3 |
2a18a89008d3daac6d8f9db03ddd582173032c7a
2ed37ef to
f78a5dc
Compare
|
The current version of toml11 uses Should we
|
If success, this will be a PR to toml11
abafd39 to
9258b5e
Compare
|
@franzpoeschel You set the toml and lohmann json developer as auther. I thought about is. We should not do that. We should them set as co-auther and commit as tools users. The reason is that currently they will be shown up in the statistic as project contributors but they contributed only indirect because we use there project as dependency. So after thinking a lot about it I would say we should push the changes clearly as subtree as in #5364 to not fake the project statistic with external developers. |
Fix #5355 by upgrading dependencies toml11 and nlohmann_json to versions containing the fix:
This is a relatively simple workaround for the issue documented therein, especially as it does not require changes in openPMD and hence fixes the problem also for old versions.
Explanation:
inline namespacecreates a namespace that does not actually need to be used in including code. Instead, it only instructs the linker to emit qualified symbols, thus avoiding the problematic symbol clashes.Aside from this, I am currently experimenting with automatically installing the internally shipped versions of toml11 / nlohmann-json along with openPMD uponmake installin openPMD/openPMD-api#1757. The far goal would be to use the logic described there as a "protocol" for openPMD and PIConGPU to agree on common versions for internal header-only libraries.TODO: