root-project
[cmake] always check if `json_fwd.hpp` header provided
If external version of nlohmann/json.hpp is provided,
always test if json_fwd.hpp is there. If not, one only
can use versions 3.9 .. 3.10, for which forward declaration
match to included into REveElement.hxx.
After LLVM upgrade one can fully skip usage of json_fwd.hpp header
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
It removes the requirement for version 3.9.
Technically code works with all 3.x versions of nlohmann/json.hpp. I add requirement for 3.9 recently just because we were not using and not testing for json_fwd.hpp. Now version restriction is not necessary.
It worsens the error message in case json_fwd.h when it is required for recent versions. They were written based on feedback from upstream, so I would like to keep the detailed messages.
json_fwd.hpp now always required - not only for 3.11. Just as workaround for time been we allow to use [3.10 .. 3.11] versions without it.
After LLVM upgrade we could completely skip usage of json_fwd.h. I hope it will happen before 6.28 release.
Technically code works with all 3.x versions of
nlohmann/json.hpp. I add requirement for 3.9 recently just because we were not using and not testing forjson_fwd.hpp. Now version restriction is not necessary.
My point is: why would we want to go back? AFAICT we already released ROOT with the requirement for 3.9. Did anybody complain?
json_fwd.hppnow always required - not only for 3.11. Just as workaround for time been we allow to use [3.10 .. 3.11] versions without it.
For me, this isn't an argument for changing the error message for the current version and make it worse.
After LLVM upgrade we could completely skip usage of
json_fwd.h.
I still don't understand this. All discussions we had so far ended with agreeing that the LLVM upgrade will change nothing wrt the JSON library.
I still don't understand this. All discussions we had so far ended with agreeing that the LLVM upgrade will change nothing wrt the JSON library.
After LLVM upgrade we can use json.hpp directly in header files.
Thus one can simply avoid complication with json_fwd.hpp
Build failed on mac1015/cxx17. Running on macitois21.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build See console output.
Failing tests:
- projectroot.gui.webdisplay.test.test_webgui_ping
- projectroot.roottest.python.distrdf.dask.roottest_python_distrdf_dask_test_all
- projectroot.roottest.python.distrdf.spark.roottest_python_distrdf_spark_test_all
That about this PR?
It is necessary to correctly detect situation when json_fwd.hpp is really required.
Main problem if system-wide installation of newer 3.11.x version of nlohmann/json.hpp does not includes json_fwd.hpp.
That's why this PR.
Older (lower than 3.9 are) rare now, but can work without problems - if provides json_fwd.hpp.
@linev as I said in https://github.com/root-project/root/pull/11268#pullrequestreview-1088542373 two weeks ago, I approve the fix for fail-on-missing. I'm not happy about the other changes (in particular, I don't understand why we would want to allow older versions than we did in the past), and I disagree with the references to the "LLVM upgrade" solving something with nlohmann/json.hpp - this never stood up to a factual check.
and I disagree with the references to the "LLVM upgrade" solving something with nlohmann/json.hpp - this never stood up to a factual check.
I did check with llvm 13 branch - it was working.
and I disagree with the references to the "LLVM upgrade" solving something with nlohmann/json.hpp - this never stood up to a factual check.
I did check with llvm 13 branch - it was working.
What did you check, and what worked? Did you build a module? For me, LLVM 9 and LLVM 13 give the same result: including json.hpp into a header in one module is fine, merging from multiple modules doesn't work in either version.
I don't understand why we would want to allow older versions than we did in the past
The reason why we restrict version - we were not testing for json_fwd.hpp.
With 3.11.x version such simple restriction no longer working.
But now we testing json_fwd.hpp and can use it, and restriction making no sense.
What did you check, and what worked? Did you build a module? For me, LLVM 9 and LLVM 13 give the same result: including json.hpp into a header in one module is fine, merging from multiple modules doesn't work in either version.
I compile with llvm13 and directly include json.hpp in REve classes and can load and use it. But I did not try to use json.hpp from several libraries - may be this makes problem.
What did you check, and what worked? Did you build a module? For me, LLVM 9 and LLVM 13 give the same result: including json.hpp into a header in one module is fine, merging from multiple modules doesn't work in either version.
I compile with llvm13 and directly include json.hpp in REve classes and can load and use it.
This already works with LLVM 9 for me. Last time we discussed, the issues you mentioned were due to GCC12.
But I did not try to use json.hpp from several libraries - may be this makes problem.
Yes, this will be a problem as discussed at CERN a while back.