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 fail if enable DBUILD_OPTEL but disable DBUILD_OPTEL_OTLP_BENCHMARKS #3213

Closed
1 task
chuandew opened this issue Dec 9, 2024 · 3 comments
Closed
1 task
Labels
build-problem problems with building this sdk closed-for-staleness

Comments

@chuandew
Copy link

chuandew commented Dec 9, 2024

Describe the bug

when I set BUILD_OPTEL=ON and BUILD_OPTEL_OTLP_BENCHMARKS=OFF, cmake configure fail

Regression Issue

  • Select this option if this issue appears to be a regression.

Expected Behavior

when I set BUILD_OPTEL=ON and BUILD_OPTEL_OTLP_BENCHMARKS=OFF, cmake configure should success.

Current Behavior

cmake -DCMAKE_PREFIX_PATH=/home/chuande/workspace/dingofs/third-party/installed -DBUILD_OPTEL=ON -DBUILD_SHARED_LIBS=OFF -DENABLE_TESTING=OFF -DAUTORUN_UNIT_TESTS=OFF -DBUILD_ONLY=s3 -DBUILD_OPTEL=ON -DBUILD_OPTEL_OTLP_BENCHMARKS=OFF ..

CMake Error at /home/chuande/workspace/dingofs/third-party/installed/lib/cmake/opentelemetry-cpp/opentelemetry-cpp-target.cmake:142 (set_target_properties):
  The link interface of target "opentelemetry-cpp::otlp_http_client"
  contains:

    nlohmann_json::nlohmann_json

  but the target was not found.  Possible reasons include:

    * There is a typo in the target name.
    * A find_package call is missing for an IMPORTED target.
    * An ALIAS target is missing.

Call Stack (most recent call first):
  /home/chuande/workspace/dingofs/third-party/installed/lib/cmake/opentelemetry-cpp/opentelemetry-cpp-config.cmake:131 (include)
  cmake/external_dependencies.cmake:114 (find_package)
  CMakeLists.txt:199 (include)

cmake -DCMAKE_PREFIX_PATH=/home/chuande/workspace/dingofs/third-party/installed -DBUILD_OPTEL=ON -DBUILD_SHARED_LIBS=OFF -DENABLE_TESTING=OFF -DAUTORUN_UNIT_TESTS=OFF -DBUILD_ONLY=s3 -DBUILD_OPTEL=ON -DBUILD_OPTEL_OTLP_BENCHMARKS=ON ..

-- SKIPPING:Threads::Threads
-- SKIPPING:Threads::Threads
-- SKIPPING:opentelemetry-cpp::api
-- SKIPPING:opentelemetry-cpp::api
-- SKIPPING:opentelemetry-cpp::sdk
-- SKIPPING:opentelemetry-cpp::sdk
-- SKIPPING:opentelemetry-cpp::ext
-- SKIPPING:opentelemetry-cpp::ext
-- Configuring done (8.8s)
-- Generating done (0.3s)
-- Build files have been written to: /home/chuande/workspace/aws-sdk-cpp/build

-

Reproduction Steps

In build dir run

cmake -DCMAKE_PREFIX_PATH=/home/chuande/workspace/dingofs/third-party/installed -DBUILD_OPTEL=ON  -DBUILD_SHARED_LIBS=OFF  -DENABLE_TESTING=OFF  -DAUTORUN_UNIT_TESTS=OFF  -DBUILD_ONLY=s3 -DBUILD_OPTEL=ON -DBUILD_OPTEL_OTLP_BENCHMARKS=OFF ..

Possible Solution

In external_dependencies.cmake,if BUILD_OPTEL enable, call find_package(nlohmann_json REQUIRED)

# Open telemetry client
if (BUILD_OPTEL)
    find_package(nlohmann_json REQUIRED)
    find_package(opentelemetry-cpp CONFIG REQUIRED)
endif ()

Additional Information/Context

No response

AWS CPP SDK version used

1.11.399

Compiler and Version used

gcc (GCC) 13.1.1 20230614 (Red Hat 13.1.1-4) Copyright (C) 2023 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE

Operating System and version

Rocky Linux release 8.9 (Green Obsidian)

@sbiscigl
Copy link
Contributor

sbiscigl commented Dec 9, 2024

I've opened a PR for this, but im considering closing it now that I'm thinking about it more. The SDK interface does not require the json dependency. The sample only requires json because it uses the HTTP exporter. i.e. from open telemetry dependency page json is required for only the otlp/http exporter. When we build the SDK with -DBUILD_OPTEL=ON we dont depend on any exporter implementation, only the interface.

If you are using the HTTP/OTLP exporter in your client code you should call find_pacakge on json, as you have the direct dependency on it, we do not.

heres an example dockerfile with it working as expected

# Use the latest AL2022 image
FROM public.ecr.aws/amazonlinux/amazonlinux:2023

# Install dev tools
RUN yum groupinstall -y 'Development Tools' && \
    yum install -y ninja-build cmake3 curl-devel openssl-devel

# Install otel
RUN git clone -b v1.18.0 https://github.com/open-telemetry/opentelemetry-cpp.git && \
    cd opentelemetry-cpp && \
    mkdir build && \
    cd build && \
    cmake3 -DCMAKE_CXX_STANDARD=14 -DBUILD_TESTING=OFF -DBUILD_SHARED_LIBS=ON -DCMAKE_POSITION_INDEPENDENT_CODE=ON .. && \
    cmake3 --build . && \
    cmake3 --install .

# Build SDK
RUN git clone --recurse-submodules https://github.com/aws/aws-sdk-cpp && \
    cd aws-sdk-cpp && \
    mkdir build && \
    cd build && \
    cmake3 -DCMAKE_CXX_STANDARD=14 -DBUILD_OPTEL=ON -DBUILD_OPTEL_OTLP_BENCHMARKS=OFF -DBUILD_ONLY="s3" -DAUTORUN_UNIT_TESTS=OFF .. && \
    cmake3 --build . && \
    cmake3 --install .

Since we dont actually require the dependency to build the SDK, im against putting it as a dependency.

@sbiscigl sbiscigl added build-problem problems with building this sdk response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 10 days. and removed bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Dec 9, 2024
@chuandew
Copy link
Author

chuandew commented Dec 11, 2024

When awssdk/opentelemetry-cpp/nlohmann_json all as extrenal project like this https://github.com/chuandew/dingofs/blob/add_opentele/third-party/CMakeLists.txt
We have no chance to call find_pacakge and cmake config is fail.

@sbiscigl
Copy link
Contributor

sbiscigl commented Dec 11, 2024

we are only dependent on the SDK and API, they do not have a nlohmann_json dependency. Only the otlp and http exporter have a json dependency. The SDK does not use those, only the sample code.

If you plan on using dependencies that require nlohmann_json you need to find those dependencies in your build.

The cmake error you post is a trace from the open telemetry projects cmake, not the SDK's. we are not responsible for finding a dependency that we are not dependent on. please remove us as being dependent nlohmann_json.

We only happen to find it for our example, whoever is actually dependent it is responsible for calling find_pacakge

@jmklix jmklix added closing-soon This issue will automatically close in 4 days unless further comments are made. and removed response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 10 days. labels Dec 11, 2024
@github-actions github-actions bot added closed-for-staleness and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build-problem problems with building this sdk closed-for-staleness
Projects
None yet
Development

No branches or pull requests

3 participants