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

Bazel Build alterations #416

Merged
merged 6 commits into from
Dec 7, 2020

Conversation

jsuereth
Copy link
Contributor

@jsuereth jsuereth commented Dec 2, 2020

This PR attempts to do the following:

  • Abide by nested-workspace conventions in BAZEL community (see Recursive WORKSPACE file loading bazelbuild/bazel#1943)
    Note: This follows OpenCensusCpp implementation pretty closely
  • Update BAZEL to also use git submodules, keeping the build as close to CMAKE as possible.
    Note: These submodules are NOT locked to version/tags, but really should be. Also, lack of SHA verification has me concerned. I'm not sure what the git submodule equivalent of this is.
  • Update documentation to make sure developers know to init the submodules to build.

TL;DR: Consumers of the bazel build can define their own dependencies prior to importing OTel and we will attempt to use their build/versions rather than our own.
@jsuereth jsuereth requested a review from a team December 2, 2020 21:38
@codecov
Copy link

codecov bot commented Dec 2, 2020

Codecov Report

Merging #416 (ca476bc) into master (11cc6cd) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #416   +/-   ##
=======================================
  Coverage   94.58%   94.58%           
=======================================
  Files         179      179           
  Lines        7677     7677           
=======================================
  Hits         7261     7261           
  Misses        416      416           
Impacted Files Coverage Δ
sdk/test/common/circular_buffer_test.cc 98.97% <0.00%> (-1.03%) ⬇️
sdk/test/metrics/counter_aggregator_test.cc 100.00% <0.00%> (+1.78%) ⬆️

Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

LGTM. Would like some more eye to review too due to my limited bazel knowledge : )

WORKSPACE Outdated Show resolved Hide resolved
],
)
# Load our direct dependencies.
opentelemetry_cpp_deps()
Copy link
Member

Choose a reason for hiding this comment

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

+1 on these changes. The root WORKSPACE looks much cleaner now : )

native.local_repository,
name = "com_google_googletest",
path = "third_party/googletest",
)
Copy link
Member

Choose a reason for hiding this comment

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

Nice. We are using google-test and benchmark from submodules directory now : )

CONTRIBUTING.md Outdated Show resolved Hide resolved

# OTLP Protocol definition
maybe(
native.new_local_repository,
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems coming from the WORKSPACE file, but still wondering why OTLP protocol definition is native.new_local_repository but others are native.local_repository?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the difference between something that HAS a bazel-build already, or one where we're adapting it to be a BAZEL workspace.

OTLP does NOT have a bazel build, so we're provding our own build file.

# Google Benchmark library.
# Only needed for benchmarks, not to build the OpenTelemetry library.
maybe(
native.local_repository,
Copy link
Contributor

Choose a reason for hiding this comment

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

Are consumers of otel-cpp meant to load e.g. benchmark before calling opentelemetry_cpp_deps()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah. I need to write the "INSTALLING" section for bazel, but basic gist is if you want to configure your own version/build that you can do so if you load BEFORE calling our macro, but you'll be on your own ensuring the version is compatible with us.

@lalitb lalitb merged commit a4a88dd into open-telemetry:master Dec 7, 2020
@jsuereth jsuereth deleted the wip-use-submodules-in-bazel branch December 7, 2020 20:40
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.

4 participants