Skip to content

Fix opentelemetry-proto file exists check #1824

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.

Sign up for GitHub

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

Merged
lalitb merged 3 commits into from
Dec 1, 2022
Merged

Fix opentelemetry-proto file exists check #1824

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4c1611e
Fix opentelemetry-proto file exists check
eguzki Nov 30, 2022
cd12159
add CHANGELOG entry
eguzki Nov 30, 2022
9c29437
Merge branch 'main' into fix-opentelemetry-proto-file-exists-check
lalitb Nov 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ Increment the:

## [Unreleased]

* [BUILD] Fix opentelemetry-proto file exists check [#1824](https://github.com/open-telemetry/opentelemetry-cpp/pull/1824)

## [1.8.0] 2022-11-27

* [DOC] Update Metrics status in README.md [#1722](https://github.com/open-telemetry/opentelemetry-cpp/pull/1722)
Expand Down
2 changes: 1 addition & 1 deletion cmake/opentelemetry-proto.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
#

if(OTELCPP_PROTO_PATH)
if(NOT EXISTS(${OTELCPP_PROTO_PATH}/opentelemetry/proto/common/v1/common.proto))
Copy link
Copy Markdown
Contributor

@ThomsonTan ThomsonTan Nov 30, 2022

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?

Copy link
Copy Markdown
Member

@marcalff marcalff Dec 1, 2022

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.

Copy link
Copy Markdown
Member

@lalitb lalitb Dec 1, 2022

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.

if(NOT EXISTS "${OTELCPP_PROTO_PATH}/opentelemetry/proto/common/v1/common.proto")
message(FATAL_ERROR "OTELCPP_PROTO_PATH does not point to a opentelemetry-proto repository")
endif()
message(STATUS "opentelemetry-proto dependency satisfied by: external path")
Expand Down