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

[INSTALL] add cmake components to the package #3220

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

dbarker
Copy link
Contributor

@dbarker dbarker commented Dec 23, 2024

Fixes #3218

Overview

This PR introduces CMake COMPONENTS to the opentelemetry-cpp package. Components are essentially named groups of CMake targets and can be passed to the find_package function to more selectively include targets and dependencies.

For example, with this PR the following will only include the opentelemetry-cpp::api header only target and search for no other dependencies that may be needed by other targets of the package.

# only find and include the api target
find_package(opentelemetry-cpp CONFIG REQUIRED COMPONENTS api)

Using components like this provides developers more control over what targets and dependencies CMake includes and should help reduce the scope of dependency issues a developer may encounter when building their instrumented CMake projects against different versions/configurations of the installed opentelemetry-cpp package.

Please see the updated INSTALL.md file of this PR for the proposed components to targets mapping.

Backwards compatibility is covered by including all components and finding all dependencies when the developer does not pass any components to find_package as in the case below.

# include all targets and find all dependencies of the package
find_package(opentelemetry-cpp CONFIG REQUIRED)

Changes

  1. Breaks up the single opentelemetry-cpp-target.cmake file into target files per component by updating the CMake install commands that create export sets in the CMakeLists.txt files for each of the targets.
  2. Updates the opentelemetry-cpp-config.cmake to include a complete list of all components and all targets. This file now reacts to the opentelemetry-cpp_FIND_COMPONENTS list set by find_package when a developer adds the COMPONENTS arguments.
  3. Updates the INSTALL.md file with the proposed components to targets mapping and guidance on usage.
  4. TODO - add ci tests on the installed packages/components/targets.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

Copy link

netlify bot commented Dec 23, 2024

Deploy Preview for opentelemetry-cpp-api-docs canceled.

Name Link
🔨 Latest commit d2d2608
🔍 Latest deploy log https://app.netlify.com/sites/opentelemetry-cpp-api-docs/deploys/677ed42ceefbb500083523a1

@dbarker
Copy link
Contributor Author

dbarker commented Dec 23, 2024

Posting this as draft and seeking feedback. Interested in thoughts/concerns on the general approach and the proposed component to target mapping.

If the proposed changes look reasonable, I'd be happy to add the ci tests to verify the changes.

Copy link

codecov bot commented Dec 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.16%. Comparing base (d19eb32) to head (d2d2608).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #3220   +/-   ##
=======================================
  Coverage   88.16%   88.16%           
=======================================
  Files         198      198           
  Lines        6224     6224           
=======================================
  Hits         5487     5487           
  Misses        737      737           

@dbarker dbarker changed the title add cmake components to group targets [INSTALL] add cmake components to the package Dec 23, 2024
@marcalff
Copy link
Member

marcalff commented Jan 8, 2025

Hi @owent , could you take a look at this change ? Thanks.

@@ -12,6 +12,9 @@ target_include_directories(
PUBLIC "$<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}/include>"
"$<INSTALL_INTERFACE:include>")

set_target_properties(opentelemetry_exporter_zipkin_trace
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The EXPORT_NAME property is set for all other installed targets, so setting here for the zipkin target.


set(OPENTELEMETRY_CPP_LIBRARIES)
set(_OPENTELEMETRY_CPP_LIBRARIES_TEST_TARGETS
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This list has been renamed to _OPENTELEMETRY_CPP_TARGETS above and the following targets have been added:

  • resources - Existing target of the sdk
  • in_memory_metric_exporter - Existing target of exporters/memory
  • zipkin_trace_exporter - New EXPORT_NAME for the existing opentelemetry_exporter_zipkin_trace target

@dbarker
Copy link
Contributor Author

dbarker commented Jan 8, 2025

@owent This is still a draft but I'm interested in your feedback before I go too much further. Here is a brief summary of work remaining and some open questions.

Work remaining:

  1. Add support for transitive component dependencies (ie: find_package(opentelemetry-cpp COMPONENTS sdk) must automatically include the api component).
  2. Add the find_dependency call for nlohmann/json in the cmake.config file
  3. Ensure all components requiring the http client, curl, and json find those dependencies in the cmake config file
  4. Add install verification tests to CI
    • Check for backwards compatibility with current cmake guidance.
    • Check that the appropriate targets are present after calling find_package(... COMPONENTS ...) on each component

Some open questions:

  1. Aside from ensuring behavior of the current cmake guidance remains the same, are there any other backwards compatibility concerns?
  2. Does the proposed mapping of target names to components in the INSTALL.md make sense?
    • I had considered a 1:1 mapping for targets to components but decided to group them by dependency/function instead. This way the components are more closely associated with the cmake build options (ie: WITH_OTLP_GRPC=ON will create the exporters_otlp_grpc component).
  3. Are the proposed tests sufficient and is it reasonable to append these to the existing ci tests similar to verify_packages.sh script?

@dbarker dbarker marked this pull request as ready for review January 8, 2025 20:32
@dbarker dbarker requested a review from a team as a code owner January 8, 2025 20:32
# List all possible opentelemetry-cpp component targets. Some may not be supported by this install


set(_OPENTELEMETRY_CPP_COMPONENTS
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will likely move the components list and targets list to separate cmake files that can be included here. This along with a file that defines the component to component dependencies and component to thirdparty package dependencies will be helpful in testing.

@@ -774,17 +774,6 @@ if(OPENTELEMETRY_INSTALL)
"${CMAKE_CURRENT_BINARY_DIR}/cmake/${PROJECT_NAME}/${PROJECT_NAME}-config-version.cmake"
DESTINATION "${CMAKE_INSTALL_LIBDIR}/cmake/${PROJECT_NAME}")

# Export all components
export(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This export of the target file to the current binary directory appears unused and I've removed it. The PR only installs the target files for the components in the install directory.

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 this pull request may close these issues.

Support CMake find_package for opentelemetry-cpp components
3 participants