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

tools can only be built with appropriate nlohmann_json version #181

Closed
tmadlener opened this issue Oct 18, 2022 · 7 comments · Fixed by #189
Closed

tools can only be built with appropriate nlohmann_json version #181

tmadlener opened this issue Oct 18, 2022 · 7 comments · Fixed by #189

Comments

@tmadlener
Copy link
Contributor

#163 introduced a CI failure, because it tries to build a target that links to nlohmann_json, which is not explicitly discovered via find_package before (or at least not with an appropriate version).

This should be rather easy to fix by putting the find_package that is currently in tests into a CMakeLists.txt further up the directory tree and then dealing with it appropriately.

@vvolkl
Copy link
Collaborator

vvolkl commented Oct 18, 2022

I'd prefer to add a cmake option and always build the nlohmann_json dependent parts except if this is explicitly turned off.

@tmadlener
Copy link
Contributor Author

Yeah, probably the solution that is easier to maintain.

@zaborowska
Copy link

On that topic... EDM4hep that I build locally works well with nlohmann_json 3.11.2, but it fails when 3.10.2 was used. Are you sure that 3.10 is the minimal requirement? That's the error I get:

[ 29%] Building CXX object edm4hep/CMakeFiles/edm4hep.dir/src/ParticleID.cc.o
/builds/azaborow/OpenDataDetector/EDM4hep/edm4hep/src/ParticleID.cc: In function 'void edm4hep::to_json(nlohmann::json&, const edm4hep::ParticleID&)':
/builds/azaborow/OpenDataDetector/EDM4hep/edm4hep/src/ParticleID.cc:138:3: error: no matching function for call to 'nlohmann::basic_json<>::basic_json(<brace-enclosed initializer list>)'
  138 |   };
      |   ^
In file included from /builds/azaborow/OpenDataDetector/EDM4hep/edm4hep/edm4hep/ParticleID.h:15,
                 from /builds/azaborow/OpenDataDetector/EDM4hep/edm4hep/src/ParticleID.cc:4:
/usr/local/include/nlohmann/json.hpp:19687:5: note: candidate: 'nlohmann::basic_json<ObjectType, ArrayType, StringType, BooleanType, NumberIntegerType, NumberUnsignedType, NumberFloatType, AllocatorType, JSONSerializer, BinaryType>::basic_json(nlohmann::basic_json<ObjectType, ArrayType, StringType, BooleanType, NumberIntegerType, NumberUnsignedType, NumberFloatType, AllocatorType, JSONSerializer, BinaryType>&&) [with ObjectType = std::map; ArrayType = std::vector; StringType = std::__cxx11::basic_string<char>; BooleanType = bool; NumberIntegerType = long int; NumberUnsignedType = long unsigned int; NumberFloatType = double; AllocatorType = std::allocator; JSONSerializer = nlohmann::adl_serializer; BinaryType = std::vector<unsigned char>]'
19687 |     basic_json(basic_json&& other) noexcept
      |     ^~~~~~~~~~
/usr/local/include/nlohmann/json.hpp:19687:5: note:   candidate expects 1 argument, 5 provided
[...]

Moreover, I have trouble updating nlohmann_json version because it seems to be always using the version ROOT was compiled with (but maybe that compatibility is needed).

@tmadlener
Copy link
Contributor Author

I am not sure I have ever tried with 3.10.2, but it looks like it is not sufficient. The lowest I have tried is 3.10.5.

I am not sure which version of nlohmman_json takes precedence if root was compiled with a different one (also probably depending on whether root was built with builtin_nlohmannjson or whether root also uses an external installation.

Which stack have you tried to build against? I will have another look and see whether we have to tighten that requirement.

On a side note: I think the main problem this issue tried to keep track of has been solved with #185

@zaborowska
Copy link

I was using this docker image, it's used in the CI of the Open Data Detector.

ROOT uses external installation (v 3.10.2).

@tmadlener
Copy link
Contributor Author

Thanks for testing. Do you need the JSON output capabilities, or would it be enough to simply make the requirements higher, so that no JSON output is built?

@zaborowska
Copy link

I think it would make sense to have the requirement changed to 3.10.5, since it's clearly not sufficient to have 3.10.

I do not need JSON output, so it was just an obstacle in the installation for me. But I just upgraded the docker image and we use now 3.10.5, so I do not have any problem anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants