-
Notifications
You must be signed in to change notification settings - Fork 417
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
#574: add instructions about how to build with Bazel #747
Conversation
Thanks @iblancasa for the PR. We can get some feedback from @eyjohn and @jsuereth also here. As @eyjohn had some plans to reorganize the build/install document for cmake and bazel. |
Codecov Report
@@ Coverage Diff @@
## main #747 +/- ##
==========================================
+ Coverage 96.00% 96.01% +0.01%
==========================================
Files 176 176
Lines 7183 7183
==========================================
+ Hits 6896 6897 +1
+ Misses 287 286 -1
|
|
||
```console | ||
$ bazel build //... | ||
bazel build -- //... -//exporters/otlp/... -//exporters/prometheus/... |
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.
Technically the exclusion here -//exporters/otlp/...
- isn't right. It all builds in its entirety right now, including otlp
- which is going to be the main thing. Helper script for windows:
bazel.exe build %* //... |
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.
How about just invoke tools/buildbazel.cmd
here which will keep both consistent?
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.
Maybe we need to add a script for Linux then, like tools/build-bazel.sh
. But I actually like this document too.
I think you should be good to re-add |
|
||
```console | ||
$ bazel build //... | ||
bazel build -- //... -//exporters/otlp/... -//exporters/prometheus/... |
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.
bazel build -- //... -//exporters/otlp/... -//exporters/prometheus/... | |
bazel build //... |
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.
OTLP-gRPC takes forever to build though 😄 (293s total on my docker container with WSL2)
Please run the linter locally (you can see how it's running in yaml action), fix whatever issues, and submit the resulting cleaned-up markdown. |
To install Git, consult the [Set up Git](https://help.github.com/articles/set-up-git/) | ||
guide on GitHub. | ||
- [Bazel](https://www.bazel.build/) for building opentelemetry-cpp API, | ||
SDK with their unittests. We use 3.7.2 in our build system. |
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.
Can we somehow omit 3.7.2.
here. It's 10 seconds to follow the guidelines, even if we don't mention the version explicitly. Maybe suggesting to check what version is being used indirectly, by lookup up the file contents here:
opentelemetry-cpp/.bazelversion
Line 1 in 414a731
3.7.2 |
Something like: Please install the exact version of bazel mentioned in .bazelversion
?
mgolovanov@xps:/mnt/c/work/opentelemetry-cpp.main$ bazel build //...
ERROR: The project you're trying to build requires Bazel 3.7.2 (specified in /mnt/c/work/opentelemetry-cpp.main/.bazelversion), but it wasn't found in /usr/bin.
You can install the required Bazel version via apt:
sudo apt update && sudo apt install bazel-3.7.2
Even if someone messes it up like I did, it takes 10 seconds to correct it - it even tells how to correct / install the right version. I think we might be upgrading the Bazel soon anyways, so probably not worth it anchoring to specific version in the how-to doc.
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.
Yeah, I think we should likely encourage folks to install/use Bazelisk, as it can auto-correct versions on the fly and leads to a much smoother developer experience when you move back/forth between different bazel projects.
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 this contribution, it covers the process well.
I still have (slow) plans to restructure the install/usage docs focused on using otel-cpp in projects (which may/not include installing it) but I think this will build on top of these base instructions.
@iblancasa - This is good to merge once the CI check for markdown lint is fixed. CI uses markdownlint-cli to validate the document using this configuration. |
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.
One meta question on instructions, otherwise, thanks much for taking a crack at this!
|
||
To install Bazel, consult the [Installing Bazel](https://docs.bazel.build/versions/3.7.0/install.html) guide. | ||
|
||
### Building as Standalone Bazel Project |
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.
Isn't this more useful for contributors, less so installation?
E.g. I'd expect most bazel users to want to do something like pulling in OTel CPP via WORKSPACE
definition using http_archive
or some other mechanism.
Not that this isn't useful, just not how I'd expect folks to consume.
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.
e.g. https://github.com/GoogleCloudPlatform/opentelemetry-operations-cpp/blob/main/WORKSPACE#L46
Or you can see:
https://github.com/googleapis/google-cloud-cpp/pull/1230/files for an example usage of OpenCensus C++
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.
This is a good point. We can have a separate section (say) "Incorporating Into An Existing Project", which includes the above-mentioned steps, similar to how we document for CMake
.
@iblancasa - we would like to pull these changes in. But the markdown-lint check is failing for now. Once you fix that, we can merge the changes. |
@iblancasa - I have fixed the lint error in your branch, and will be merging now. Hope this is fine :) |
Fixes #574
Changes
Add some instructions about how to build with Bazel.