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

external nlohmann/json.hpp is not forwarded as dependency or should be linked PRIVATE? #6784

Closed
1 task done
andresailer opened this issue Nov 10, 2020 · 7 comments
Closed
1 task done

Comments

@andresailer
Copy link
Contributor

  • Checked for duplicates

Describe the bug

If the external nhlohman json is used ROOT an error occurs if ROOTEve is used

CMake Error at cmake/DD4hepBuild.cmake:625 (add_library):
  Target "DDEvePlugins" links to target "nlohmann_json::nlohmann_json" but
  the target was not found.  Perhaps a find_package() call is missing for an
  IMPORTED target, or an ALIAS target is missing?
Call Stack (most recent call first):
  DDEve/CMakeLists.txt:43 (dd4hep_add_plugin)

(original discovery via lcg nightlies in DD4hep package build on top of root master

Expected behavior

ROOT targets resolve all their dependencies so that other CMake projects can use them without error or change to their cmake
Find_dependency added to e.g., ROOTConfig.cmake
or
this needs to be PRIVATE ?
https://github.com/linev/root/blob/88bdfd736f0b859b40973c457b75df9c73b0bbaf/graf3d/eve7/CMakeLists.txt#L139

To Reproduce

have a project with a library linking against ROOTEve
Assuming cmake is in the PATH (compiler and OS shouldn't matter)

wget http://lcgpackages.web.cern.ch/lcgpackages/tarFiles/latest/ROOT-HEAD_db894-x86_64-centos7-gcc8-opt.tgz
tar xzf ROOT-HEAD_db894-x86_64-centos7-gcc8-opt.tgz
mkdir test
cd test
cat > CMakeLists.txt <<EOF
CMAKE_MINIMUM_REQUIRED(VERSION 3.12 FATAL_ERROR)
PROJECT(myeve)
find_package(ROOT REQUIRED)
add_library(myeve SHARED myeve.cpp)
TARGET_LINK_LIBRARIES(myeve ROOT::ROOTEve)
EOF

cat > myeve.cpp <<EOF
int main ()
{
  return 1;
}
EOF
mkdir build
cd build

# assuming cmake is in the path
cmake -D ROOT_DIR=${PWD}/../../ROOT/HEAD/x86_64-centos7-gcc8-opt/cmake ..

gives

CMake Error at CMakeLists.txt:4 (add_library):
  Target "myeve" links to target "nlohmann_json::nlohmann_json" but the
  target was not found.  Perhaps a find_package() call is missing for an
  IMPORTED target, or an ALIAS target is missing?

Setup

  1. ROOT master, cmake ... -Dbuiltin_nlohmannjson=OFF ....
  2. centos7
  3. build with LCGCmake
@github-actions github-actions bot added this to Needs triage in Triage Nov 10, 2020
@linev linev self-assigned this Nov 10, 2020
@linev
Copy link
Member

linev commented Nov 10, 2020

Yes, I can confirm a problem.
export for fictional nlohmann_json library is missing.
I will try to fix it

@linev
Copy link
Member

linev commented Nov 10, 2020

@oshadura, according to this cmake docu, one have to add something like find_dependency(nlohmann_json) into ROOTConfig.cmake

I did not found any example of usage of this macro in ROOT.
I tried to use your code around builtin_glew, but it does not work.
nlohmann_json does not appear in exported targets.
Also direct call like

export(TARGETS nlohmann_json::nlohmann_json NAMESPACE nlohmann_json:: APPEND FILE ${PROJECT_BINARY_DIR}/ROOTConfig-targets.cmake)

fail while nlohmann_json::nlohmann_json is not build and cmake does not allow to export it.

Should I try with find_dependency(nlohmann_json)?

linev added a commit to linev/root that referenced this issue Nov 10, 2020


In such case one requires external nlohmann_json::nlohmann_json
to get proper include paths
@oshadura
Copy link
Contributor

I agree that it is an exceptional case, I just not very happy that we will add only nlohmann_json in ROOTConfig.cmake

@linev
Copy link
Member

linev commented Nov 10, 2020

I just not very happy that we will add only nlohmann_json in ROOTConfig.cmake

May be it can be good starting point!

For many components (like RMySQL library) we provide our own FindMySQL.cmake macro,
which automatically exports system include paths. Therefore problem not appears.

If we would use standard find_package(MySQL), we probably also need to provide find_dependency(MySQL).

@oshadura
Copy link
Contributor

@linev totally agree with you, we will definitely need to review other builtins/dependencies...

@jblomer jblomer removed this from Needs triage in Triage Nov 10, 2020
linev added a commit that referenced this issue Nov 10, 2020
In such case one requires external nlohmann_json::nlohmann_json
to get proper include paths
@linev linev added this to Issues in Fixed in 6.24/00 via automation Nov 10, 2020
@linev
Copy link
Member

linev commented Nov 10, 2020

I hope, now it should work. PR #6788 has to fix a problem

@linev linev closed this as completed Nov 10, 2020
@andresailer
Copy link
Contributor Author

Thanks for quickly taking care of this!

vgvassilev pushed a commit to vgvassilev/root that referenced this issue Nov 12, 2020


In such case one requires external nlohmann_json::nlohmann_json
to get proper include paths
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

No branches or pull requests

4 participants