Revert "[cxxmodules] Enable a module if json is present." #9267
Conversation
This reverts commit 0cdfa69.
|
Starting build on |
|
Build failed on mac1014/python3. |
|
Build failed on mac11.0/cxx17. Failing tests: |
This reverts commit 1c38aa0.
|
Starting build on |
|
Build failed on ROOT-ubuntu2004/soversion. Failing tests: |
|
Build failed on mac11.6/cxx17. Errors:
|
|
@osschar we really need to replace the JSON include in the eve header with a forward declaration, it's causing problems with modules. If that cannot be done, can we be more selective on the supported versions of nlohmann, to either still use fwd decls, or do be sure to have json_fwd.h? |
|
Starting build on |
| @@ -18,11 +18,7 @@ | |||
|
|
|||
| #include <memory> | |||
|
|
|||
| #ifdef NLOHMANN_JSON_PROVIDES_FWD_HPP | |||
| #include <nlohmann/json_fwd.hpp> | |||
There was a problem hiding this comment.
I believe that only #include <nlohmann/json.hpp> should be used, as nlohmann/json_fwd.hpp is not always available, and we don't have control over when it's available or not if nlohmann_json is taken from the system. Other than that, seems fine.
There was a problem hiding this comment.
Yeah, maybe the build system should detect that early on and switch to builtin_json=On...
There was a problem hiding this comment.
No, for distros you cannot enable builtins, it has to be taken from the system. Same for LCG, probably, but there we can change the installation of nlohman_json.
|
Let's see what #9278 says - that's still my preferred solution, even if it'd mean that we can only support a restricted set of nlohmann_json versions. Or have multiple, version specific fwd decls. |
That presumes the underlying template forward declaration is the same across versions and I am not sure how safe is that. We should just support json versions that have this header and that is it. |
It's not a question of versions, it's a question of packages, and some distros don't have it. And I am happy to add fwd decls for different nlohmann_json versions if needed - right now we don't know whether that's actually needed or a theoretical issue. We can also "just support json versions" that match the fwd decls ;-) |
|
See also #8345, where the check for the header was included. |
|
Yes, any solution that doesn't use the json_fwd.h header should be ok. |
|
I can close that now. |
Reverts #9251 due to failures in LCG.
In 1c38aa0 we made it possible to use installed json versions which do not have
json_fwd.hto forward declare the json classes. However in this case, that brings the entire json package in the dictionaries. In turn, then we will need to build Json.pcm to avoid content duplication. This in turn requires LCG to provide ROOT_INCLUDE_PATH=/path/to/json at runtime.Is this all worth it?