-
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
Fix opentelemetry-proto file exists check #1824
Fix opentelemetry-proto file exists check #1824
Conversation
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.
Thanks for the fix.
The original bug is mine, sorry I must have been distracted when doing last minute changes.
@@ -26,7 +26,7 @@ | |||
# | |||
|
|||
if(OTELCPP_PROTO_PATH) | |||
if(NOT EXISTS(${OTELCPP_PROTO_PATH}/opentelemetry/proto/common/v1/common.proto)) |
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.
@marcalff , do you think we can have a test on this passed in env var?
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.
Tested locally with:
malff@localhost.localdomain:build-opentelemetry-cpp> pwd
/home/malff/CODE/MY_GITHUB/build-opentelemetry-cpp
ccmake \
-DOTELCPP_PROTO_PATH=/home/malff/CODE/MY_GITHUB/opentelemetry-proto \
-DWITH_OTLP=ON \
-DWITH_OTLP_HTTP=ON \
-DWITH_OTLP_GRPC=ON \
../opentelemetry-cpp \
CMake result without the fix:
CMake Error at cmake/opentelemetry-proto.cmake:30 (message):
OTELCPP_PROTO_PATH does not point to a opentelemetry-proto repository
CMake result with the fix:
opentelemetry-proto dependency satisfied by: external path
So the fix is working.
OTELCPP_PROTO_PATH also shows in the ccmake UI.
@ThomsonTan
Using environment variables will be odd, may be I don't understand what you suggest.
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.
@ThomsonTan - Hope this is ok to merge. If there are any specific test scenarios, we can address them separately.
Fixes #1823
Changes
Fix CMake
EXISTS
syntaxFor significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes