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

[BUILD] Support to use different cmake package CONFIG of dependencies. #2263

Conversation

owent
Copy link
Member

@owent owent commented Aug 15, 2023

Fixes #2251

Changes

  • Patch imported targets to support to use different cmake package CONFIG of dependencies.

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

@owent owent requested a review from a team August 15, 2023 07:47
@codecov
Copy link

codecov bot commented Aug 15, 2023

Codecov Report

Merging #2263 (c88c04b) into main (3ff4b4c) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2263   +/-   ##
=======================================
  Coverage   87.41%   87.41%           
=======================================
  Files         199      199           
  Lines        6018     6018           
=======================================
  Hits         5260     5260           
  Misses        758      758           

Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

Thanks for the change.

Please add some comment to describe what feature it actually provides (or bug it fixes),
with an example of when it is useful, and how to use it.

Please assume the reader has no or little knowledge of CMake, and make the example as explicit as possible.

cmake/tools.cmake Show resolved Hide resolved
cmake/tools.cmake Outdated Show resolved Hide resolved
cmake/tools.cmake Outdated Show resolved Hide resolved
Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

Approved, thanks for the comments.

See two suggestions.

@owent
Copy link
Member Author

owent commented Sep 27, 2023

Approved, thanks for the comments.

See two suggestions.

Thanks, the spell errors are fixed.

@marcalff marcalff added the pr:please-review This PR is ready for review label Oct 3, 2023
@marcalff
Copy link
Member

marcalff commented Oct 3, 2023

Added the please-review label.

This PR looks ok to me, but given this affects makefiles with CMake in general, maybe @lalitb @ThomsonTan or @esigo can look at it also.

Copy link
Member

@esigo esigo left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks for the PR :)

cmake/tools.cmake Outdated Show resolved Hide resolved
@marcalff marcalff changed the title Support to use different cmake package CONFIG of dependencies. [BUILD] Support to use different cmake package CONFIG of dependencies. Oct 16, 2023
@marcalff marcalff merged commit cbee4de into open-telemetry:main Oct 16, 2023
44 checks passed
@owent owent deleted the support_using_different_package_config_of_dependencies_with_cmake branch October 31, 2023 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:please-review This PR is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support to use different cmake package CONFIG of dependencies.
4 participants