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

Building from the release tarball fails if nlohmann-json not installed on the system #981

Closed
westonpace opened this issue Sep 17, 2021 · 1 comment · Fixed by #1074
Closed
Labels
bug Something isn't working build and test

Comments

@westonpace
Copy link

Describe your environment

Ubuntu 20.04
Linux pace-desktop 5.11.0-34-generic #36~20.04.1-Ubuntu SMP Fri Aug 27 08:06:32 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
nlohmann-json3-dev package is NOT installed system wide

Steps to reproduce

  • In reality I am trying to add nlohmann-json as a dependency into my own cmake module using ExternalProject and listing the release tarball as the desired version. The below steps however, are simpler, and just pretend I wanted to download and install the release into my system.
  • Download release tarball
  • Extract release tarball
  • cd <extracted_dir>
  • mkdir build
  • cd build
  • cmake ..

What is the expected behavior?
Ideally, cmake would download nlohmann-json using ExternalProject or some other mechanism (or at least have a flag to enable this). Or, nlohmann-json's headers would be included as part of the release tarball. However, that does not seem to be implemented yet, which is fine.

Given this, I would expect to see the standard CMake find_package failure message:

CMake Error at CMakeLists.txt:245 (find_package):
  By not providing "Findnlohmann_json.cmake" in CMAKE_MODULE_PATH this
  project has asked CMake to find a package configuration file provided by
  "nlohmann_json", but CMake did not find one.

  Could not find a package configuration file provided by "nlohmann_json"
  with any of the following names:

    nlohmann_jsonConfig.cmake
    nlohmann_json-config.cmake

  Add the installation prefix of "nlohmann_json" to CMAKE_PREFIX_PATH or set
  "nlohmann_json_DIR" to a directory containing one of the above files.  If
  "nlohmann_json" provides a separate development package or SDK, be sure it
  has been installed.

What is the actual behavior?
What did you see instead?

CMake Error at CMakeLists.txt:256 (add_subdirectory):
  The source directory

    /home/pace/Downloads/opentelemetry-cpp-1.0.0-rc4/third_party/nlohmann-json

  does not contain a CMakeLists.txt file.

Additional context

The problem is that the release tarball contains the nlohmann-json directory but it is empty (likely because it is a git submodule and the CI/build is not including this as part of the tarball).

If I delete that directory then I still don't get the error message I would expect. Instead I get:

  nlohmann_json package was not found.  Please either provide it manually or
  clone with submodules.  To initialize, fetch and checkout any nested
  submodules, you can use the following command:

  git submodule update --init --recursive

This is a little better. I imagine it is a helpful message added for new developers. However, since I am downloading the release tarball, it is more or less meaningless (I didn't clone any git repository, there is no action I can take here). If I mark the package as required then I get the error message I would expect (at which point I clearly realize that I need to provide a system install of the nlohmann-json module and I can resolve that).

This might be opinionated, and I really do appreciate the work that you have done here, but it is rather odd behavior from a CMake module and any deviation from the CMake norm seems to send me down an unfortunate trail of debugging.

@westonpace westonpace added the bug Something isn't working label Sep 17, 2021
@lalitb
Copy link
Member

lalitb commented Sep 17, 2021

Valid concern. We should improve on the error.

Also, default build of API + SDK + OTLP/gRPC doesn't use JSON, and cmake shouldn't check for this library for default build.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working build and test
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants