-
Notifications
You must be signed in to change notification settings - Fork 440
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] Bring your own dependency: opentelemetry-proto #1730
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1730 +/- ##
==========================================
+ Coverage 85.73% 85.79% +0.06%
==========================================
Files 171 171
Lines 5212 5212
==========================================
+ Hits 4468 4471 +3
+ Misses 744 741 -3
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Is it better to set OTELCPP_PROTO_PATH
when using internal proto by ExternalProject_Add
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks.
Improved usability: poor configuration detected at cmake time, not compile time.
Thanks @owent I don't understand the suggestion itself. Do you mean to set OTELCPP_PROTO_PATH as well after:
If so, this would mean the makefile alter choices made by the user, making it unclear if this variable is an input to the makefile, or an output from the makefile (when using submodules and/or a github download), of both, causing confusion in my opinion. Please clarify. |
@owent Do you have any further comments, or can this be merged. thanks. |
Fixes #1719
Changes
Add option OTELCPP_PROTO_PATH in CMake.
CHANGELOG.md
updated for non-trivial changes