Skip to content

[eve7] Include json_fwd.hpp if available#11205

Merged
hahnjo merged 3 commits intoroot-project:masterfrom
hahnjo:json-fwd
Aug 19, 2022
Merged

[eve7] Include json_fwd.hpp if available#11205
hahnjo merged 3 commits intoroot-project:masterfrom
hahnjo:json-fwd

Conversation

@hahnjo
Copy link
Copy Markdown
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
Copy Markdown

phsft-bot commented Aug 18, 2022

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 hahnjo mentioned this pull request Aug 18, 2022
Closed
@linev
Copy link
Copy Markdown
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.

@wdconinc wdconinc mentioned this pull request Aug 18, 2022
Merged
falbrechtskirchinger
falbrechtskirchinger reviewed Aug 18, 2022
View reviewed changes
Copy link
Copy Markdown

@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.

hahnjo added 2 commits August 19, 2022 09:49
@hahnjo
[eve7] Include json_fwd.hpp if available
395d387 
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
[cmake] Check for json_fwd.hpp in versions > 3.11
7f699ad 
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
Copy Markdown

phsft-bot commented Aug 19, 2022

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
Copy Markdown
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.

linev
linev reviewed Aug 19, 2022
View reviewed changes
linev
linev approved these changes Aug 19, 2022
View reviewed changes
Copy link
Copy Markdown
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!

@linev linev mentioned this pull request Aug 19, 2022
Merged
@hahnjo hahnjo merged commit ee52a8b into root-project:master Aug 19, 2022
@hahnjo hahnjo deleted the json-fwd branch August 19, 2022 11:52
@hahnjo hahnjo mentioned this pull request Aug 19, 2022
Merged
@wdconinc
Copy link
Copy Markdown
Contributor

wdconinc commented Aug 19, 2022

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
Copy Markdown
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

Reviewers

@linev linev linev approved these changes

@Axel-Naumann Axel-Naumann Awaiting requested review from Axel-Naumann

@bellenot bellenot Awaiting requested review from bellenot bellenot is a code owner

@osschar osschar Awaiting requested review from osschar

+1 more reviewer

@falbrechtskirchinger falbrechtskirchinger falbrechtskirchinger left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@hahnjo hahnjo

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

5 participants

@hahnjo @phsft-bot @linev @wdconinc @falbrechtskirchinger