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

OpenTelemetry-cpp superbuild #1384

Closed
esigo opened this issue May 8, 2022 · 4 comments
Closed

OpenTelemetry-cpp superbuild #1384

esigo opened this issue May 8, 2022 · 4 comments
Assignees

Comments

@esigo
Copy link
Member

esigo commented May 8, 2022

Before opening a feature request against this repo, consider whether the feature should/could be implemented in the other OpenTelemetry client libraries. If so, please open an issue on opentelemetry-specification first.

Is your feature request related to a problem?
If so, provide a concise description of the problem.
I have a piece of cmake that builds OpenTelemetry-cpp:

ExternalProject_Add(
  opentelemetry
  DEPENDS grpc
  GIT_REPOSITORY https:/open-telemetry/opentelemetry-cpp.git
  GIT_TAG v1.3.0
  GIT_SHALLOW 1
  UPDATE_COMMAND ""
  CMAKE_ARGS -DCMAKE_INSTALL_PREFIX=${STAGED_INSTALL_PREFIX}
             -DCMAKE_BUILD_TYPE=Release
             -DCMAKE_POSITION_INDEPENDENT_CODE=TRUE
             -DWITH_ZIPKIN=OFF
             -DWITH_JAEGER=OFF
             -DWITH_EXAMPLES=OFF
             -DCMAKE_INSTALL_PREFIX=/opt/third_party/install
             -DBUILD_TESTING=OFF
             -DWITH_OTLP=ON
             -DBUILD_SHARED_LIBS=ON
             -DProtobuf_DIR=${STAGED_INSTALL_PREFIX}/lib/cmake/protobuf
             -Drotobuf_DIR=${STAGED_INSTALL_PREFIX}/lib/cmake/protobuf
             -DgRPC_DIR=${grpc_DIR}
             -Dabsl_DIR=${STAGED_INSTALL_PREFIX}/lib/cmake/absl
  CMAKE_CACHE_ARGS -DCMAKE_CXX_FLAGS:STRING=${CMAKE_CXX_FLAGS}
  TEST_AFTER_INSTALL 0
  DOWNLOAD_NO_PROGRESS 1
  LOG_CONFIGURE 1
  LOG_BUILD 0
  LOG_INSTALL 1)

Where grpc is another external project (similar to #1382). The build fails using this cmake because we use find_package in module mode which doesn't find grpc/protobuf because of name mismatch. This can be solved if we use config search mode first.

failure:

  Found package configuration file:

    build/stage/lib/cmake/grpc/gRPCConfig.cmake

  but it set gRPC_FOUND to FALSE so package "gRPC" is considered to be NOT
  FOUND.  Reason given by package:

  The following imported targets are referenced, but are missing:
  protobuf::libprotobuf protobuf::libprotoc

Describe the solution you'd like
What do you want to happen instead? What is the expected behavior?
We can use this:

  find_package(Protobuf QUIET CONFIG)
  if((NOT Protobuf_FOUND AND NOT PROTOBUF_FOUND))
    find_package(Protobuf)
  endif()

Describe alternatives you've considered
Which alternative solutions or features have you considered?

Additional context
Add any other context about the feature request here.

@esigo esigo self-assigned this May 8, 2022
@owent
Copy link
Member

owent commented May 9, 2022

 find_package(Protobuf QUIET CONFIG)
 if((NOT Protobuf_FOUND AND NOT PROTOBUF_FOUND))
  find_package(Protobuf)
 endif()

I think we can search CONFIG packages first by setting CMAKE_FIND_PACKAGE_PREFER_CONFIG to ON or TRUE, not calling find_package twice.

@owent
Copy link
Member

owent commented May 9, 2022

protobuf::libprotobuf and protobuf::libprotoc are missing only when we use legacy FindProtobuf.cmake in cmake, when using prebuilt protobuf cmake scripts with protobuf_MODULE_COMPATIBLE=ON , protobuf::libprotobuf and protobuf::libprotoc should always be available. I think we can patch the legacy FindProtobuf.cmake by adding these scripts below after find_package(Protobuf)

# For old cmake versions
foreach(NEW_VAR_NAME
  Protobuf_INCLUDE_DIR
  Protobuf_LIBRARY
  Protobuf_PROTOC_LIBRARIES
  Protobuf_LIBRARY_DEBUG
  Protobuf_PROTOC_LIBRARY_DEBUG
)
  string(TOUPPER OLD_VAR_NAME "${NEW_VAR_NAME}")
  if (NOT ${NEW_VAR_NAME} AND ${OLD_VAR_NAME})
    set(${NEW_VAR_NAME} "${${OLD_VAR_NAME}}")
  endif()
endforeach()

if(NOT TARGET protobuf::libprotobuf)
  add_library(protobuf::libprotobuf IMPORTED)

  set_target_properties(protobuf::libprotobuf PROPERTIES
    INTERFACE_INCLUDE_DIRECTORIES "${Protobuf_INCLUDE_DIR}"
    IMPORTED_LINK_INTERFACE_LANGUAGES_RELEASE "CXX;RC"
    IMPORTED_LOCATION "${Protobuf_LIBRARY$<<CONFIG:Debug>:_DEBUG}"
  )
endif()

if(NOT TARGET protobuf::libprotoc)
  add_library(protobuf::libprotoc IMPORTED)

  set_target_properties(protobuf::libprotoc PROPERTIES
    INTERFACE_INCLUDE_DIRECTORIES "${Protobuf_INCLUDE_DIR}"
    INTERFACE_LINK_LIBRARIES "protobuf::libprotobuf"
    IMPORTED_LINK_INTERFACE_LANGUAGES_RELEASE "CXX;RC"
    IMPORTED_LOCATION "${Protobuf_PROTOC_LIBRARIES$<<CONFIG:Debug>:_DEBUG}}"
  )
endif()

@esigo esigo changed the title OpenTelemetry-cpp super build OpenTelemetry-cpp superbuild May 10, 2022
@esigo
Copy link
Member Author

esigo commented May 16, 2022

Thanks @owent, I fixed this by using another superbuild for protobuf as a dependency.
https:/esigo/opentelemetry-cpp/blob/superbuild/docker/CMakeLists.txt

@esigo esigo closed this as completed May 16, 2022
@compoundradius
Copy link

compoundradius commented Oct 21, 2022

Following the recommendations at https://grpc.io/docs/languages/cpp/quickstart/

We strongly encourage you to install gRPC locally — using an appropriately set CMAKE_INSTALL_PREFIX — because there is no easy way to uninstall gRPC after you’ve installed it globally.

I have installed gRPC 1.50.0 to $HOME/.local.
When compiling opentelemetry-cpp with cmake -DWITH_OTLP=ON ../. I also had to patch your CMakeLists.txt to use the above recommended workaround:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 14db63d1..f7be3c54 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -270,6 +270,10 @@ if(WITH_OTLP)
   find_package(Protobuf)
   if(WITH_OTLP_GRPC OR (NOT DEFINED WITH_OTLP_GRPC AND NOT DEFINED
                                                        CACHE{WITH_OTLP_GRPC}))
+    find_package(Protobuf QUIET CONFIG)
+    if((NOT Protobuf_FOUND AND NOT PROTOBUF_FOUND))
+        find_package(Protobuf)
+    endif()
     find_package(gRPC)
   endif()
   if((NOT Protobuf_FOUND AND NOT PROTOBUF_FOUND) OR (NOT gRPC_FOUND))

This worked, but is there a better way?

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

Successfully merging a pull request may close this issue.

3 participants