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

CMake targets broken in 8.1.0 when using external nlohmann_json #2762

Closed
dirkvdb opened this issue Jul 2, 2021 · 6 comments · Fixed by #2780
Closed

CMake targets broken in 8.1.0 when using external nlohmann_json #2762

dirkvdb opened this issue Jul 2, 2021 · 6 comments · Fixed by #2780
Assignees
Labels

Comments

@dirkvdb
Copy link

dirkvdb commented Jul 2, 2021

When cmake configure finds an external nlohmann_json it will use that.

In that case a nlohmann_json::nlohmann_json dependency is set on the cmake targets. But no find_dependency(nlohmann_json REQUIRED) is added to the proj-config.cmake.

Adding the proj target as a dependency now causes a cmake error:

Target "yourtarget" links to target "nlohmann_json::nlohmann_json" but the target was not found

Environment Information

  • PROJ version 8.1.0

Installation method

  • from source
@dirkvdb dirkvdb added the bug label Jul 2, 2021
@rouault
Copy link
Member

rouault commented Jul 2, 2021

@mwtoews opinions / guidance welcome how to fix that. I'd say that external nlohmann_json should not be exported by PROJ cmake files, as it is only an implementation detail that users of the PROJ API don't have direct access to

@mwtoews
Copy link
Member

mwtoews commented Jul 3, 2021

@dirkvdb could you provide a few more details, such as the command or options you used to configure cmake? An os and compiler would be handy too. I'm unable to trigger this error, so any guidance will help me find a resolution.

@dirkvdb
Copy link
Author

dirkvdb commented Jul 3, 2021

This happens when configuring proj with -DNLOHMANN_JSON_ORIGIN="external" on any platform.
The proj build will work fine, but external projects that do a

find_package(PROJ CONFIG REQUIRED)
target_link_libraries(mytarget PRIVATE PROJ::proj)

will get the error.

I made a pull request #2763 with a possible fix.

@mwtoews
Copy link
Member

mwtoews commented Jul 5, 2021

I'm still unable to trigger this error. Here is my method, first starting up docker in the git repo:

docker run -t -i --rm -v `pwd`:/io ubuntu:20.04 bash

then within the VM:

apt update
apt install cmake nlohmann-json3-dev sqlite3 libsqlite3-dev libtiff-dev libcurl4-openssl-dev g++
cd /io/
mkdir build && cd build
cmake ..
make -j7
make install
apt remove nlohmann-json3-dev
cd ../test
./postinstall/test_cmake.sh /usr/local

and I've modified line 7 on test/postinstall/testappprojinfo/CMakeLists.txt to try and break it.

Ideally dependant projects won't need nlohmann/json, as it's a header-only library only required to build PROJ.

@dirkvdb
Copy link
Author

dirkvdb commented Jul 5, 2021

In the testappprojinfo project you link against proj using the ${${USE_PROJ_NAME}_LIBRARIES} define.
Since the problem is in the cmake target, this will work.
I think it will break if you modify the test project to link against the cmake proj target: PROJ::proj

The cmake targets system is superior to using the _LIBRARIES and _INCLUDES variables because it contains more information than just include paths and libraries (linker flags, transitive dependencies, ...) It does require the targets to be found in the cmake config file even when it is a private dependency. This is crucial for static builds that require the transitive dependencies for linking.

@mwtoews
Copy link
Member

mwtoews commented Jul 5, 2021

Ah right, it's a static build. I now see the error with -DBUILD_SHARED_LIBS=OFF. Thanks for the PR, I'll investigate further. I've been meaning to get the postinstall suite to also check static builds, so this seems to be a good time to get that going.

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