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

[eve7] Include json_fwd.hpp if available #11205

Merged
merged 3 commits into from
Aug 19, 2022
Merged

Conversation

hahnjo
Copy link
Member

@hahnjo hahnjo commented 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 #11130

@hahnjo hahnjo requested a review from Axel-Naumann August 18, 2022 08:11
@hahnjo hahnjo self-assigned this Aug 18, 2022
@phsft-bot
Copy link
Collaborator

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14
How to customize builds

@linev
Copy link
Member

linev commented Aug 18, 2022

json_fwd.hpp exists since a while and included in many early distributions.
My OpenSUSE has nlohmann/json.hpp version 3.10.5 with such forward declaration.

Problem is ROOT builds where nlohmann/json.hpp can be found only with extra settings for shell variables.
In such cases none of its includes can be used in public ROOT interfaces.

Copy link

@falbrechtskirchinger falbrechtskirchinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some notes:

  • Avoid v3.11.0 and v3.11.1 if possible, i.e., advise users to upgrade.
  • We did not split the inline namespace in v3.11.2 (it's still just one).
  • json_fwd.hpp is only available from v3.11.0 if built with -DJSON_MultipleHeaders=ON.
  • If not for the inline namespace, this would have broken once nlohmann/json#3110 is merged.

cmake/modules/SearchInstalledSoftware.cmake Outdated Show resolved Hide resolved
graf3d/eve7/inc/ROOT/REveElement.hxx Outdated Show resolved Hide resolved
hahnjo added 2 commits August 19, 2022 09:49
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
Our forward declaration in REveElement.hxx breaks with the versioned
namespaces in 3.11, so we require the json_fwd.hpp header.
@phsft-bot
Copy link
Collaborator

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14
How to customize builds

@hahnjo
Copy link
Member Author

hahnjo commented Aug 19, 2022

json_fwd.hpp exists since a while and included in many early distributions. My OpenSUSE has nlohmann/json.hpp version 3.10.5 with such forward declaration.

Problem is ROOT builds where nlohmann/json.hpp can be found only with extra settings for shell variables. In such cases none of its includes can be used in public ROOT interfaces.

I know, but we have to do something, and as agreed on Wednesday, we actually want to ensure that users have the same version of nlohmann/json.

Copy link
Member

@linev linev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eve7 examples working - tested with opensuse and system-wide install json.hpp and json_fwd.hpp
One can merge it!

@hahnjo hahnjo merged commit ee52a8b into root-project:master Aug 19, 2022
@hahnjo hahnjo deleted the json-fwd branch August 19, 2022 11:52
@wdconinc
Copy link
Contributor

Does this merit a backport into 6.26 branch since it prevents compilation? Just wondering from a packager's point of view keeping an eye on build issues.

@hahnjo
Copy link
Member Author

hahnjo commented Aug 19, 2022

@wdconinc yes, I already posted #11225 (currently waiting for Jenkins to finish before merging). Will be released as 6.26/08 (no date planned yet AFAIK), or you can pick the middle commit yourself.

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 this pull request may close these issues.

ROOT doesn't compile with new nlohmann-json version 3.11.0
5 participants