Skip to content
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

ROOT doesn't compile with new nlohmann-json version 3.11.0 #11130

Closed
kgizdov opened this issue Aug 6, 2022 · 28 comments · Fixed by #11205
Closed

ROOT doesn't compile with new nlohmann-json version 3.11.0 #11130

kgizdov opened this issue Aug 6, 2022 · 28 comments · Fixed by #11205

Comments

@kgizdov
Copy link
Contributor

kgizdov commented Aug 6, 2022

from #11130 (comment)

[ 91%] Building CXX object roofit/roofitcore/CMakeFiles/RooFitCore.dir/src/TestStatistics/RooSumL.cxx.o
In file included from /build/root/src/root-6.26.06/graf3d/eve7/src/REveBoxSet.cxx:20:
/usr/include/nlohmann/json.hpp:5085:8: error: reference to ‘json’ is ambiguous
 5085 | inline nlohmann::json operator "" _json(const char* s, std::size_t n)
      |        ^~~~~~~~
In file included from /usr/include/nlohmann/detail/meta/type_traits.hpp:22,
                 from /usr/include/nlohmann/detail/exceptions.hpp:22,
                 from /usr/include/nlohmann/detail/conversions/from_json.hpp:23,
                 from /usr/include/nlohmann/adl_serializer.hpp:14,
                 from /usr/include/nlohmann/json.hpp:35:
/usr/include/nlohmann/json_fwd.hpp:61:7: note: candidates are: ‘using json = class nlohmann::json_v3_11_1::basic_json<>’
   61 | using json = basic_json<>;
      |       ^~~~
.....

Original bug report regarding glibc 2.36

Describe the bug

ArchLinux recently upgraded to glibc 2.36, and that resulted in ROOT not compiling with either GCC 11.3.0 or GCC 12.1.1

Expected behaviour

ROOT should compile with glibc 2.36.

To Reproduce

  1. Install glibc 2.36 and relevant compilers
  2. Try to build ROOT
  3. Error during build

Setup

  1. ROOT version: 6.26/06 (or any other version)
  2. Operating system: Arch Linux x86
  3. Build from source with GCC 11.3.0 / 12.1.1

Additional context

An interesting caveat is that it seems ROOT also requires 32-bit specific headers (gnu/stubs-32.h) as well.
Error log:

...
[ 73%] Built target Thread
[ 73%] Generating G__RIO.cxx, ../../lib/libRIO.rootmap
In file included from <built-in>:400:
<command line>:1:9: error: macro name must be an identifier
#define -compilerI/usr/include/c++/12.1.1 1
^
In file included from input_line_3:1:
In file included from /build/root/src/build/etc/cling/lib/clang/9.0.1/include/assert.h:8:
In file included from /usr/include/assert.h:35:
In file included from /usr/include/features.h:514:
/usr/include/gnu/stubs.h:7:11: fatal error: 'gnu/stubs-32.h' file not found
# include <gnu/stubs-32.h>
^~~~~~~~~~~~~~~~
Error: Error loading the default rootcling header files.   
@kgizdov kgizdov added the bug label Aug 6, 2022
@guitargeek
Copy link
Contributor

Hi @kgizdov!

I had the same problem as you after upgrading Arch Linux last week. This PR fixed the problem for me:

#11111

I also backported it to the 6.26 branch, such that the next ROOT 6.26 patch release won't have the problem.

Can you check if after this fix things work again for you? Thanks!

@kgizdov
Copy link
Contributor Author

kgizdov commented Aug 7, 2022

Thanks, I'll try right away.

@kgizdov
Copy link
Contributor Author

kgizdov commented Aug 7, 2022

Almost, now I have a different problem having to do with json:

[ 91%] Building CXX object roofit/roofitcore/CMakeFiles/RooFitCore.dir/src/TestStatistics/RooSumL.cxx.o
In file included from /build/root/src/root-6.26.06/graf3d/eve7/src/REveBoxSet.cxx:20:
/usr/include/nlohmann/json.hpp:5085:8: error: reference to ‘json’ is ambiguous
 5085 | inline nlohmann::json operator "" _json(const char* s, std::size_t n)
      |        ^~~~~~~~
In file included from /usr/include/nlohmann/detail/meta/type_traits.hpp:22,
                 from /usr/include/nlohmann/detail/exceptions.hpp:22,
                 from /usr/include/nlohmann/detail/conversions/from_json.hpp:23,
                 from /usr/include/nlohmann/adl_serializer.hpp:14,
                 from /usr/include/nlohmann/json.hpp:35:
/usr/include/nlohmann/json_fwd.hpp:61:7: note: candidates are: ‘using json = class nlohmann::json_v3_11_1::basic_json<>’
   61 | using json = basic_json<>;
      |       ^~~~
.....

Any remedy for that?

@guitargeek guitargeek assigned linev and Axel-Naumann and unassigned guitargeek and linev Aug 8, 2022
@guitargeek
Copy link
Contributor

Ah, I see that error too now, just needed to enable ROOT 7 in the cmake config.

So the issue is related to the latest nlohmann json release, and comes from these two PRs:

This is unfortunate, and we can't just update the forward declarations because they will change with each nlohmann json version now.

I assigned this issue to @Axel-Naumann, because probably he remembers best why the manual forward declaration was introduced. Optimally, we'll find a solution where the official json_fwd header can be used again.

@guitargeek
Copy link
Contributor

Also marking this as critical, because we don't want to make any new release with broken compilation I guess

@guitargeek guitargeek changed the title GLIBC 2.36 breaks compilation from source ROOT doesn't compile with new nlohmann-json version 3.11.0 Aug 8, 2022
@linev
Copy link
Member

linev commented Aug 9, 2022

I not yet test latest nlohmann/json.hpp, but seems to be it is incompatible with previous versions.
The simple solution for now would be ignoring incompatible versions of nlohmann/json.hpp and use builtin one.

@linev
Copy link
Member

linev commented Aug 9, 2022

I assigned this issue to @Axel-Naumann, because probably he remembers best why the manual forward declaration was introduced. Optimally, we'll find a solution where the official json_fwd header can be used again.

Main problem is cling. If nlohmann/json.hpp included in header files and parsed during dictionary generation, next time when such file "seen" by cling - it crashes.

Official json_fwd cannot be used while some distributions does not include it - making impossible to use external version of nlohmann/json.hpp.

@hahnjo
Copy link
Member

hahnjo commented Aug 16, 2022

I updated the issue description to talk about nlohmann/json instead of the (already solved) issue with glibc 2.36. For the forward declaration, I have a (hopefully clever) idea how we can solve this across versions, let me play with this today.

@linev
Copy link
Member

linev commented Aug 16, 2022

llvm13 branch works with plain nlohmann/json.hpp.
Once it is merged, one can easily resolve this issue

@hahnjo
Copy link
Member

hahnjo commented Aug 16, 2022

Ok, then I still don't understand the issue. Yesterday I understood that it's not related to LLVM, but that ROOT's forward declaration is simply "wrong" for newer versions of nlohmann/json because it changed some template parameters...

@linev
Copy link
Member

linev commented Aug 16, 2022

Original issue related to the fact, that each new version of nlohmann/json.hpp requires own forward declaration like it used in REveElement.hxx:

https://github.com/root-project/root/blob/master/graf3d/eve7/inc/ROOT/REveElement.hxx#L24-L37

Without llvm13 one cannot directly include nlohmann/json.hpp in header file.

@hahnjo
Copy link
Member

hahnjo commented Aug 16, 2022

My understanding is the following, please correct me: As soon as we include nlohmann/json.hpp into a header, all the classes become part of the corresponding module. So if headers from different modules include the header, like

diff --git a/graf3d/eve/inc/TEveCalo.h b/graf3d/eve/inc/TEveCalo.h
index 9be7925a6b..3ca258105f 100644
--- a/graf3d/eve/inc/TEveCalo.h
+++ b/graf3d/eve/inc/TEveCalo.h
@@ -21,6 +21,8 @@
 #include "TEveCaloData.h"
 #include <vector>
 
+#include <nlohmann/json.hpp>
+
 class TClass;
 class TEveRGBAPalette;
 
diff --git a/graf3d/eve7/inc/ROOT/REveElement.hxx b/graf3d/eve7/inc/ROOT/REveElement.hxx
index 2a127888a2..fb237fdfb6 100644
--- a/graf3d/eve7/inc/ROOT/REveElement.hxx
+++ b/graf3d/eve7/inc/ROOT/REveElement.hxx
@@ -16,26 +16,13 @@
 #include <ROOT/REveVector.hxx>
 #include <ROOT/REveProjectionBases.hxx>
 
+#include <nlohmann/json.hpp>
+
 #include <map>
 #include <memory>
 
 class TGeoMatrix;
 
-namespace nlohmann {
-template<typename T, typename SFINAE>
-struct adl_serializer;
-
-template <template <typename U, typename V, typename... Args> class ObjectType,
-          template <typename U, typename... Args> class ArrayType, class StringType, class BooleanType,
-          class NumberIntegerType, class NumberUnsignedType, class NumberFloatType,
-          template <typename U> class AllocatorType, template <typename T, typename SFINAE = void> class JSONSerializer,
-          class BinaryType>
-class basic_json;
-
-using json = basic_json<std::map, std::vector, std::string, bool, std::int64_t, std::uint64_t, double, std::allocator,
-                        adl_serializer, std::vector<std::uint8_t>>;
-} // namespace nlohmann
-
 namespace ROOT {
 namespace Experimental {
 

we get errors because the symbols are provided by two modules. The only way out would be to have a decoupled json module, but that is bad for other reasons.

Without llvm13 one cannot directly include nlohmann/json.hpp in header file.

Not sure what's supposed to change with llvm13, but the diff I posted above fails with identical error message. And in my opinion, rightfully so.

hahnjo added a commit to hahnjo/root that referenced this issue Aug 16, 2022
Since nlohmann/json#3590, the basic_json class
and the json using-declaration are located in a "versioned, ABI-tagged
inline namespace". This makes it impossible to forward declare the type
in REveElement.hxx.
Instead introduce a new struct REveJsonWrapper that just wraps a json
object (after including the full nlohmann/json.hpp). As the struct is
under our control, we can easily forward declare the type and use it
for method arguments.

Fixes root-project#11130
@linev
Copy link
Member

linev commented Aug 16, 2022

This should be tested - I compile llvm13 branch, replace forward declaration in eve7 and add TJSONFile with nlohmann/json in header. It works, but makes lot of warnings.

Why one cannot include same header file from different modules?

@hahnjo
Copy link
Member

hahnjo commented Aug 17, 2022

This should be tested - I compile llvm13 branch, replace forward declaration in eve7 and add TJSONFile with nlohmann/json in header. It works, but makes lot of warnings.

Indeed, placing #include <nlohmann/json.hpp> into one of the existing io/ headers seems to work; but already with master / LLVM9. Maybe it's because RIO is "special" and loaded during generation of other modules? Anyway, adding the include to tree/dataframe/inc/ROOT/RDataFrame.hxx leads to the same errors during generation of lib/modules.idx (which btw is not recognized by the build system as an error...)

@linev
Copy link
Member

linev commented Aug 17, 2022

Indeed, placing #include <nlohmann/json.hpp> into one of the existing io/ headers seems to work

Not really. One can generate dictionary and compile code.
But if one tries to load such include in ROOT session, it terminates with error.

hahnjo added a commit to hahnjo/root that referenced this issue Aug 17, 2022
Now that we have a module, this should work.

Fixes root-project#11130
@kgizdov
Copy link
Contributor Author

kgizdov commented Aug 17, 2022

@linev @hahnjo I wonder if there is something I could do in the meantime to quick patch it and compile it as is. I would like at least a working install for Arch users while this is fixed. Unfortunately, as you know on Arch, it's not trivial to switch to an older version of nlohnmann-json.

@hahnjo
Copy link
Member

hahnjo commented Aug 17, 2022

@kgizdov I suppose you're asking for the Arch Linux package in particular? (thanks btw, I'm a user myself) As the nlohmann-json package ships with the json_fwd.hpp header, you can (temporarily) apply the following:

diff --git a/graf3d/eve7/inc/ROOT/REveElement.hxx b/graf3d/eve7/inc/ROOT/REveElement.hxx
index 2a127888a2..9deb3af147 100644
--- a/graf3d/eve7/inc/ROOT/REveElement.hxx
+++ b/graf3d/eve7/inc/ROOT/REveElement.hxx
@@ -16,26 +16,13 @@
 #include <ROOT/REveVector.hxx>
 #include <ROOT/REveProjectionBases.hxx>
 
+#include <nlohmann/json_fwd.hpp>
+
 #include <map>
 #include <memory>
 
 class TGeoMatrix;
 
-namespace nlohmann {
-template<typename T, typename SFINAE>
-struct adl_serializer;
-
-template <template <typename U, typename V, typename... Args> class ObjectType,
-          template <typename U, typename... Args> class ArrayType, class StringType, class BooleanType,
-          class NumberIntegerType, class NumberUnsignedType, class NumberFloatType,
-          template <typename U> class AllocatorType, template <typename T, typename SFINAE = void> class JSONSerializer,
-          class BinaryType>
-class basic_json;
-
-using json = basic_json<std::map, std::vector, std::string, bool, std::int64_t, std::uint64_t, double, std::allocator,
-                        adl_serializer, std::vector<std::uint8_t>>;
-} // namespace nlohmann
-
 namespace ROOT {
 namespace Experimental {
 

(tested on master, but should also apply for 6.26)

@hahnjo
Copy link
Member

hahnjo commented Aug 17, 2022

uh, @Axel-Naumann @linev since nlohmann/json#3679 (included in 3.11.2), nlohmann-json also installs json_fwd.hpp in case of JSON_MultipleHeaders=OFF (which btw changed its default to ON in nlohmann/json#3532 for 3.11.0). What this means is that we can check for the header and use it if present. If it's not there, we can assume we have an older version (pre 3.11) and continue to use the forward declaration as it's written now. This would leave a small range of unsupported versions (3.11.0 and 3.11.1 built with JSON_MultipleHeaders=OFF), but I think that would be fine. What do you think?

@kgizdov
Copy link
Contributor Author

kgizdov commented Aug 17, 2022

@hahnjo CMake can natively check if a specific bit of code compiles - I think if you have a check on the forward declaration snippet and flip a flag based on successful compilation or not, it would be better and will not suffer unsupported versions, no?

@hahnjo
Copy link
Member

hahnjo commented Aug 17, 2022

@kgizdov the problem is the two cases are non-exhaustive: For versions before 3.11.0, our own forward declaration will always work. Starting with 3.11.2, json_fwd.hpp is always available. But in between, for 3.11.0 and 3.11.1, we can hit an "external" version of the library without json_fwd.hpp where our own forward declaration doesn't work (because of nlohmann/json#3590). It's not clear to me how to make this case work, but I also don't think it's really that important.

@linev
Copy link
Member

linev commented Aug 17, 2022

Another "simple" approach to compile ROOT is specify -Droot7=OFF. This will disable eve7 compilation which has such problem with forward declarations.

@kgizdov
Copy link
Contributor Author

kgizdov commented Aug 17, 2022

@hahnjo, your CMake test will either compile or not. It can't both compile and not compile at the same time. If including the forward declaration in a CMake test compiles, you set a variable that enables it in the code; otherwise, the forward declaration is switched off in the code. What am I missing?

hahnjo added a commit to hahnjo/root that referenced this issue Aug 18, 2022
Version 3.11 of nlohmann/json introduced "versioned, ABI-tagged inline
namespace"s, which breaks our forward declaration. Fortunately, we can
assume the json_fwd.hpp header to be present starting from that same
version because the JSON_MultipleHeaders option now defaults to ON and
even if not, json_fwd.hpp is installed since patch version 3.11.2. For
earlier versions, both methods work but json_fwd.hpp isn't guaranteed
to be installed. Still use it if available.

Fixes root-project#11130
@hahnjo
Copy link
Member

hahnjo commented Aug 18, 2022

Third time's a charm: #11205

@kgizdov

If including the forward declaration in a CMake test compiles, you set a variable that enables it in the code; otherwise, the forward declaration is switched off in the code. What am I missing?

The case where the forward declaration doesn't work and the json_fwd.hpp header is not provided. In the PR linked above, I propose to just error out in this case.

@kgizdov
Copy link
Contributor Author

kgizdov commented Aug 18, 2022

well, that's easily fixed as well:

try_compile(FWD_DECLARE MYBINDIR forward_declare.cpp)
try_compile(FWD_HEADER MYBINDIR forward_header.cpp)
if(FWD_DECLARE)
  message("Using forward declaration for nlohmann_json")
elseif(FWD_HEADER)
  message("Using forward header from nlohmann_json")
else()
  set(DO_WHATEVER_NLOHMANM_3.11.0_NEEDS ON)
  message("using the special code that nlohmann 3.11.0 needs")
endif()

But it's up to you. Arch Linux package builds now, so I am satisfied. Thanks for the help and quick response!

@hahnjo
Copy link
Member

hahnjo commented Aug 19, 2022

The problem is I don't know what DO_WHATEVER_NLOHMANM_3.11.0_NEEDS could be; we cannot forward declare and we cannot include json_fwd.hpp nor the full json.hpp. The last PR just errors in this case.

hahnjo added a commit to hahnjo/root that referenced this issue Aug 19, 2022
Version 3.11 of nlohmann/json introduced "versioned, ABI-tagged inline
namespace"s, which breaks our forward declaration. Fortunately, we can
assume the json_fwd.hpp header to be present starting from that same
version because the JSON_MultipleHeaders option now defaults to ON and
even if not, json_fwd.hpp is installed since patch version 3.11.2. For
earlier versions, both methods work but json_fwd.hpp isn't guaranteed
to be installed. Still use it if available.

Fixes root-project#11130
hahnjo added a commit that referenced this issue Aug 19, 2022
Version 3.11 of nlohmann/json introduced "versioned, ABI-tagged inline
namespace"s, which breaks our forward declaration. Fortunately, we can
assume the json_fwd.hpp header to be present starting from that same
version because the JSON_MultipleHeaders option now defaults to ON and
even if not, json_fwd.hpp is installed since patch version 3.11.2. For
earlier versions, both methods work but json_fwd.hpp isn't guaranteed
to be installed. Still use it if available.

Fixes #11130
hahnjo added a commit to hahnjo/root that referenced this issue Aug 19, 2022
Version 3.11 of nlohmann/json introduced "versioned, ABI-tagged inline
namespace"s, which breaks our forward declaration. Fortunately, we can
assume the json_fwd.hpp header to be present starting from that same
version because the JSON_MultipleHeaders option now defaults to ON and
even if not, json_fwd.hpp is installed since patch version 3.11.2. For
earlier versions, both methods work but json_fwd.hpp isn't guaranteed
to be installed. Still use it if available.

Fixes root-project#11130

(cherry picked from commit ed56a35)
@hahnjo hahnjo assigned hahnjo and unassigned Axel-Naumann Aug 19, 2022
@kgizdov
Copy link
Contributor Author

kgizdov commented Aug 19, 2022

For DO_WHATEVER_NLOHMANM_3.11.0_NEEDS just display a message:

try_compile(FWD_DECLARE MYBINDIR forward_declare.cpp)
try_compile(FWD_HEADER MYBINDIR forward_header.cpp)
if(FWD_DECLARE)
  message("Using forward declaration for nlohmann_json")
elseif(FWD_HEADER)
  message("Using forward header from nlohmann_json")
else()
  set(DO_WHATEVER_NLOHMANM_3.11.0_NEEDS ON)
  message(FATAL_ERROR "You are probably running nlohmann_json version 3.11.0 which is deemed unusable by its creator (https://github.com/nlohmann/json/releases/tag/v3.11.0). Please install a working version of nlohmann_json. If you think you're seeing this message in error and you have a different version of nlohmann_json installed, please report it as a bug.")
endif()

but the advantage of this way is that it's version independent and will work when it works, even if the default behaviour changes in the future again.

@hahnjo
Copy link
Member

hahnjo commented Aug 19, 2022

But that's what I'm saying all the time and what the (already merged) PR does... Except that we don't need it so convoluted, we know when our forward declaration breaks (and will be broken forever).

@kgizdov
Copy link
Contributor Author

kgizdov commented Aug 19, 2022

The point was not to depend on the version if the behaviour changes in the future. Again, it's a finer point; probably, it doesn't matter. Thanks for fixing it so quickly!

hahnjo added a commit that referenced this issue Aug 19, 2022
Version 3.11 of nlohmann/json introduced "versioned, ABI-tagged inline
namespace"s, which breaks our forward declaration. Fortunately, we can
assume the json_fwd.hpp header to be present starting from that same
version because the JSON_MultipleHeaders option now defaults to ON and
even if not, json_fwd.hpp is installed since patch version 3.11.2. For
earlier versions, both methods work but json_fwd.hpp isn't guaranteed
to be installed. Still use it if available.

Fixes #11130

(cherry picked from commit ed56a35)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants