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

#574: add instructions about how to build with Bazel #747

Merged
merged 6 commits into from
May 26, 2021

Conversation

iblancasa
Copy link
Contributor

Fixes #574

Changes

Add some instructions about how to build with Bazel.

@iblancasa iblancasa requested a review from a team May 13, 2021 15:51
@lalitb
Copy link
Member

lalitb commented May 13, 2021

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
Copy link

codecov bot commented May 13, 2021

Codecov Report

Merging #747 (1d2d9bf) into main (a6bd648) will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
sdk/src/logs/batch_log_processor.cc 95.00% <0.00%> (+1.25%) ⬆️


```console
$ bazel build //...
bazel build -- //... -//exporters/otlp/... -//exporters/prometheus/...
Copy link
Contributor

@maxgolov maxgolov May 13, 2021

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 %* //...

Copy link
Contributor

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?

Copy link
Contributor

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.

@maxgolov
Copy link
Contributor

I think you should be good to re-add otlp exporter. If you fix that, it looks good then. Approved with suggestion.


```console
$ bazel build //...
bazel build -- //... -//exporters/otlp/... -//exporters/prometheus/...
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
bazel build -- //... -//exporters/otlp/... -//exporters/prometheus/...
bazel build //...

Copy link
Contributor

@maxgolov maxgolov May 13, 2021

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)

@maxgolov
Copy link
Contributor

maxgolov commented May 13, 2021

CI / markdown-lint (pull_request) Failing after 7s — markdown-lint

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.
Copy link
Contributor

@maxgolov maxgolov May 13, 2021

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:

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.

@jsuereth

Copy link
Contributor

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.

Copy link
Contributor

@eyjohn eyjohn left a 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.

@lalitb
Copy link
Member

lalitb commented May 14, 2021

@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.

Copy link
Contributor

@jsuereth jsuereth left a 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
Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

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.

@lalitb
Copy link
Member

lalitb commented May 20, 2021

@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.

@lalitb
Copy link
Member

lalitb commented May 26, 2021

@iblancasa - I have fixed the lint error in your branch, and will be merging now. Hope this is fine :)

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.

Add Installation instructions for Bazel.
6 participants