picongpu icon indicating copy to clipboard operation
picongpu copied to clipboard

[WIP] toml11/nlohmann-json: avoid symbol clashes with different versions in upstream dependencies

Open franzpoeschel opened this issue 6 months ago • 9 comments

Fix #5355 by upgrading dependencies toml11 and nlohmann_json to versions containing the fix:

  • nlohmann_json: upgraded to version 3.12.0
  • toml11: upgraded to current development branch (commit 2a18a89008d3daac6d8f9db03ddd582173032c7a)

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 namespace creates 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 upon make install in https://github.com/openPMD/openPMD-api/pull/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:

  • [x] Document the scripts a bit.

franzpoeschel avatar May 15 '25 15:05 franzpoeschel

Just ran a test, the segfault that I saw is fixed by this.

franzpoeschel avatar May 15 '25 16:05 franzpoeschel

I was just going to fix this for nlohmann-json, but noticed that it has already been fixed with version 3.11 https://github.com/nlohmann/json/pull/3590. Internally, we still use version 3.9.1. I will remove my workaround and upgrade the single header instead. I supplied a similar PR to toml11 which has been merged by now: https://github.com/ToruNiina/toml11/pull/291 But it is not yet part of any release, so for the moment, I will leave that part as it is.

franzpoeschel avatar May 26 '25 08:05 franzpoeschel

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 https://github.com/nlohmann/json/pull/4796, but this particular issue will not be fixed as it seems to be a compiler/STL implementation bug. The workaround is to set #define JSON_HAS_RANGES 0 which needs to be done in CMake since even some Param files seem to include nlohmann_json by now and we have no control over those. Even in CMake, it's a bit of an adventure to catch every usage.

CI currently failing due to 403 errors, will try later.

franzpoeschel avatar Jun 02 '25 12:06 franzpoeschel

@franzpoeschel ~I am wondering why there are toml changes in this PR, I thought it is about json only?~~

https://github.com/ComputationalRadiationPhysics/picongpu/pull/5367#issuecomment-2908983679 explain my question

psychocoderHPC avatar Jun 10 '25 11:06 psychocoderHPC

@franzpoeschel could you commit the toml and json update as generic user

GIT_AUTHOR_NAME="Third Party" GIT_AUTHOR_EMAIL="[email protected]" git commit

and all cmake changes within a seperate commit.

psychocoderHPC avatar Jun 10 '25 11:06 psychocoderHPC

why do we not use the openPMD shipped json library if a target nlohmann_json::nlohmann_json is already available? Let's discuss it in our next developer meeting.

psychocoderHPC avatar Jun 10 '25 11:06 psychocoderHPC

why do we not use the openPMD shipped json library if a target nlohmann_json::nlohmann_json is already available? Let's discuss it in our next developer meeting.

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.

https://github.com/openPMD/openPMD-api/pull/1757 adds an option to install the internal nlohmann-json and toml11 alongside openPMD, but we cannot rely on that in PIConGPU.

franzpoeschel avatar Jun 10 '25 11:06 franzpoeschel

@franzpoeschel could you commit the toml and json update as generic user

GIT_AUTHOR_NAME="Third Party" GIT_AUTHOR_EMAIL="[email protected]" git commit

and all cmake changes within a seperate commit.

The updated JSON tree is already on a separate commit with Niels Lohmann as an author: https://github.com/ComputationalRadiationPhysics/picongpu/pull/5367/commits/59213d3ae1cd98edda0c4cd6d192a21affc47a03

franzpoeschel avatar Jun 10 '25 11:06 franzpoeschel

The current version of toml11 uses std::source_location which is only supported in Clang 15 (https://en.cppreference.com/w/cpp/compiler_support/20) and hence breaks our CI runs of Clang 12, 13 and 14. Patching that out of toml11 is a single line (https://github.com/ComputationalRadiationPhysics/picongpu/pull/5367/commits/0cd31f4a7b5dcd020ab470a31bd65d3b3e104a84).

Should we

  • Drop support for Clang < 15 (I've not even been able to set things up locally since those Clang versions have many other problems with C++20)
  • Keep the patch in our internal clone of toml11
  • Add some define like TOML11_DISABLE_SOURCE_LOCATION to override toml11's detection mechanics and submit that patch to the mainline

franzpoeschel avatar Jun 12 '25 10:06 franzpoeschel

@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. It could be they do not want to shown as contributors directly.

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.

psychocoderHPC avatar Jun 23 '25 07:06 psychocoderHPC