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

Building otlp exporter from the release tarball #1056

Conversation

esigo
Copy link
Member

@esigo esigo commented Nov 6, 2021

Fixes #1051 (issue)

Changes

Please provide a brief description of the changes here.
download opentelemtery-proto if it's not stored.
Read opentelemtery-proto tag from file if not available use main.
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

@esigo esigo requested a review from a team November 6, 2021 09:42
@codecov
Copy link

codecov bot commented Nov 6, 2021

Codecov Report

Merging #1056 (109c93b) into main (55aa7ea) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1056   +/-   ##
=======================================
  Coverage   94.85%   94.85%           
=======================================
  Files         151      151           
  Lines        5971     5971           
=======================================
  Hits         5663     5663           
  Misses        308      308           

set(${third_party_name} "${third_party_tag}")
endforeach()
endif()

Copy link
Member

@lalitb lalitb Nov 8, 2021

Choose a reason for hiding this comment

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

Trying to understand the logic here. We iterate the file and extract the name and version from each entry. In the end, third_party_name and third_party_tag contain name and version for last entry of file. What are we going to do with the last entry?

Copy link
Member Author

Choose a reason for hiding this comment

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

No we are storing all the versions not just the last one. e.g. ${nlohmann-json} has v3.9.1, or ${opentelemetry-proto} has, v0.11.0.
In this PR I use ${opentelemetry-proto} to download it from github.
We can use the same technique for all the other dependencies when they are not available in the third_party folder.

Copy link
Member

Choose a reason for hiding this comment

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

Ah got it, I missed the set command.

set(PROTO_PATH "${CMAKE_CURRENT_SOURCE_DIR}/third_party/opentelemetry-proto")
set(needs_proto_download FALSE)
else()
if("${opentelemetry-proto}" STREQUAL "")
Copy link
Member

Choose a reason for hiding this comment

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

how would this be defined to point to a specific version/branch?

set(${third_party_name} "${third_party_tag}")
endforeach()
endif()

Copy link
Member

Choose a reason for hiding this comment

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

Ah got it, I missed the set command.

@ThomsonTan ThomsonTan merged commit 435e377 into open-telemetry:main Nov 10, 2021
@esigo esigo deleted the Building-OTLP-exporter-from-the-release-tarball branch November 10, 2021 06:27
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.

Building OTLP exporter from the release tarball fails for opentelemetry-proto dependency
3 participants