nlohmann_json fails to install to /usr/local during build

[jpeach]

trafficstars

Describe your environment

ProductName: macOS ProductVersion: 12.4 BuildVersion: 21F79

Steps to reproduce

$ git rev-parse HEAD
2f0a3d69cb5fdf3486fd79502bc681f017ee04f3

$ cmake -GNinja -DBUILD_TESTING=OFF -DWITH_OTLP=ON ..
...
$ ninja
...
[103/115] Performing download step (git clone) for 'nlohmann_json_download'
Cloning into 'nlohmann_json_download'...
HEAD is now at 4f8fba140 Merge branch 'release/3.10.5'
[108/115] Performing install step for 'nlohmann_json_download'
FAILED: third_party/src/nlohmann_json_download-stamp/nlohmann_json_download-install /Users/jpeach/upstream/opentelemetry/opentelemetry-cpp/build/third_party/src/nlohmann_json_download-stamp/nlohmann_json_download-install
cd /Users/jpeach/upstream/opentelemetry/opentelemetry-cpp/build/third_party/src/nlohmann_json_download-build && /opt/homebrew/Cellar/cmake/3.23.2/bin/cmake -P /Users/jpeach/upstream/opentelemetry/opentelemetry-cpp/build/third_party/src/nlohmann_json_download-stamp/nlohmann_json_download-install-.cmake && /opt/homebrew/Cellar/cmake/3.23.2/bin/cmake -E touch /Users/jpeach/upstream/opentelemetry/opentelemetry-cpp/build/third_party/src/nlohmann_json_download-stamp/nlohmann_json_download-install
CMake Error at /Users/jpeach/upstream/opentelemetry/opentelemetry-cpp/build/third_party/src/nlohmann_json_download-stamp/nlohmann_json_download-install-.cmake:49 (message):
  Command failed: 1

   '/opt/homebrew/Cellar/cmake/3.23.2/bin/cmake' '--build' '.' '--target' 'install'

  See also

    /Users/jpeach/upstream/opentelemetry/opentelemetry-cpp/build/third_party/src/nlohmann_json_download-stamp/nlohmann_json_download-install-*.log


ninja: build stopped: subcommand failed.

What is the expected behavior? I expected the build to download and build dependencies within the build tree. I did not expect it to attempt to install build dependencies to a global system location.

What is the actual behavior?

$ cat third_party/src/nlohmann_json_download-stamp/nlohmann_json_download-install-err.log
CMake Error at cmake_install.cmake:41 (file):
  file INSTALL cannot make directory "/usr/local/include": Permission denied.

[marcalff]

After investigations, it turns out there are 3 different ways to build with nlohmann_json in the top level CMakeList.txt.

(1)

CMake finds a nlohmann_json package.

Message printed: Using external nlohmann::json

Variables visible in cmake: nlohmann_json_DIR /opt/nlohmann_json-3.10.5/lib64/cmake/nlohmann_json

make:

• builds the code
• installs nothing, as expected

make install:

• installs opentelemetry-cpp headers and libraries
• does not install nlohmann_json headers and cmake files

(2)

With:

• git submodule init
• git submodule update

When (1) fails, the makefile looks for git submodules

Message printed:

Trying to use local nlohmann::json from submodule
 Using the single-header code from
 /home/malff/CODE/OTEL/opentelemetry-cpp/third_party/nlohmann-json/single_include/

Variables visible in cmake: nlohmann_json_DIR nlohmann_json_DIR-NOTFOUND

make:

• builds the code
• installs nothing, as expected

make install:

• installs opentelemetry-cpp headers and libraries
• does install nlohmann_json headers and cmake files

(3)

When both (1) and (2) fails, the makefile downloads nlohmann_json.

Message printed: nlohmann_json package was not found. Cloning from github

Variables visible in cmake: nlohmann_json_DIR nlohmann_json_DIR-NOTFOUND

make performs these actions:

Scanning dependencies of target nlohmann_json_download
[  3%] Creating directories for 'nlohmann_json_download'
[  4%] Performing download step (git clone) for 'nlohmann_json_download'
Cloning into 'nlohmann_json_download'...
Note: switching to 'v3.10.5'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at 4f8fba140 Merge branch 'release/3.10.5'
[  4%] No patch step for 'nlohmann_json_download'
[  4%] No update step for 'nlohmann_json_download'
[  5%] Performing configure step for 'nlohmann_json_download'
-- nlohmann_json_download configure command succeeded.  See also /home/malff/CODE/OTEL/build_3/third_party/src/nlohmann_json_download-stamp/nlohmann_json_download-configure-*.log
[  5%] Performing build step for 'nlohmann_json_download'
-- nlohmann_json_download build command succeeded.  See also /home/malff/CODE/OTEL/build_3/third_party/src/nlohmann_json_download-stamp/nlohmann_json_download-build-*.log
[  5%] Performing install step for 'nlohmann_json_download'
-- nlohmann_json_download install command succeeded.  See also /home/malff/CODE/OTEL/build_3/third_party/src/nlohmann_json_download-stamp/nlohmann_json_download-install-*.log
[  6%] Completed 'nlohmann_json_download'
[  6%] Built target nlohmann_json_download

so here a make (on opentelemetry-cpp) causes a make install on nlohmann_json, which fails if CMAKE_INSTALL_PREFIX points to a global location, typically owned by root.

make install:

• installs opentelemetry-cpp headers, libraries, cmake files

In case of (3), the step [ 5%] Performing install step for 'nlohmann_json_download' during make (not make install) is out of place and is a bug.

This is the CMake build only with 3 flavors, and there is also the Bazel build.

[lalitb]

That's correct, it's a valid bug. Unfortunately, we don't have any CI test to catch this error. For Linux - the nlohmann-json dependency is already installed through apt, and macOS CI tests are only with bazel.

[marcalff]

Before changing makefiles for (3), I would like to understand if (3) is even needed at all.

Downloading code manually from github sounds redundant with git submodules, which are already in place, so maybe (3) can be removed entirely ?

@lalitb ?

[marcalff]

Also, picking an existing external nlohmann-json dependency might bring a different version, causing subtle bugs, especially considering the external package (1) takes precedence over git submodules (2)

[lalitb]

Downloading code manually from github sounds redundant with git submodules, which are already in place, so maybe (3) can be removed entirely ?

We have source releases, so it is possible for someone to download the release archive, and build it. The 3rd scenario will be used there.

[github-actions[bot]]

This issue was marked as stale due to lack of activity.

[marcalff]

Remaining header files using nlohmann_json:

elasticsearch/include/opentelemetry/exporters/elasticsearch/es_log_exporter.h:#  include "nlohmann/json.hpp"
elasticsearch/include/opentelemetry/exporters/elasticsearch/es_log_recordable.h:#  include "nlohmann/json.hpp"
etw/include/opentelemetry/exporters/etw/etw_provider.h:#  include "nlohmann/json.hpp"
zipkin/include/opentelemetry/exporters/zipkin/zipkin_exporter.h:#include "nlohmann/json.hpp"
zipkin/include/opentelemetry/exporters/zipkin/recordable.h:#include "nlohmann/json.hpp"

elasticsearch and etw are to move to opentelemetry-cpp-contrib.

zipkin header files are the zipkin exporter implementation, not to be exposed to a client (use the zipkin exporter factory instead).

As a result, there is a possibility to eliminate the need to install nlohmann_json entirely, which will simplify makefiles and dependencies.

This fix is waiting for #1423 (ETW) This fix is waiting for #1424 (ES)