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

Lazy persons's guide to building with Bazel on Windows #710

Merged
merged 4 commits into from
Apr 30, 2021

Conversation

maxgolov
Copy link
Contributor

@maxgolov maxgolov commented Apr 28, 2021

Helper script to make dev life easy.

How to build with bazel quickly

Step 1: Install bazel.exe to path. Installing Bazel on Windows.

Step 2: Then in cmd.exe - navigate to OpenTelemetry C++ SDK source tree root. Run:

tools\build-bazel.cmd

Done. Also easy to script it to run from your favorite editor. Like Visual Studio or Visual Studio Code.

Adding temporary build artifacts to .gitignore : it is annoying to see these build artifacts hanging around in case if you subsequently do commit from that same build tree.

@maxgolov maxgolov requested review from a team and jsuereth April 28, 2021 20:22
@jsuereth
Copy link
Contributor

A few questions on the command:

  • Why aren't you recommending users install bazelisk? That one makes it much easier to upgrade bazel dynamically. I think liekly we should be recommending that.
  • From a practically standpoint, CONSUMERS of Otel won't be using bazel build //... or variants at all, they'll be importing it as a workspace and defining a dependnecy on specific C++ static libs from bazel.
  • If this is meant as a dev-productivity thing, why are you explciitly calling out certain build rules vs. just using bazel test //... or bazel build //... ?

+1 to the .gitignore changes, not sure about the .cmd. I tend to leave a CMD window open and run bazel //... commands dynamically but it could be I migrated my linux workflow to windows....

@codecov
Copy link

codecov bot commented Apr 28, 2021

Codecov Report

Merging #710 (2c4cb1a) into main (fc7a325) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #710   +/-   ##
=======================================
  Coverage   94.72%   94.72%           
=======================================
  Files         216      216           
  Lines        9900     9900           
=======================================
  Hits         9378     9378           
  Misses        522      522           

@maxgolov
Copy link
Contributor Author

maxgolov commented Apr 28, 2021

  • Why aren't you recommending users install bazelisk? That one makes it much easier to upgrade bazel dynamically. I think liekly we should be recommending that.

I only mentioned the minimum necessary setup needed to build. Not the most optimal bazel setup. Since I only mentioned it in the PR notes, the actual bazel.exe might as well come with bazelisk installation. We can describe it in a markdown doc?

  • From a practically standpoint, CONSUMERS of Otel won't be using bazel build //... or variants at all, they'll be importing it as a workspace and defining a dependnecy on specific C++ static libs from bazel.

My intent was to extract the step from our harder-to-read CI script into a standalone developer-focused build script, mainly to verify that the build is successful after my changes to any bazel build files.

  • If this is meant as a dev-productivity thing, why are you explciitly calling out certain build rules vs. just using bazel test //... or bazel build //... ?

This is meant to be a dev-productivity thing. I copied these build rules from CI script.
I am not an expert in Bazel build rules, so I copied these 1-1 from here:

bazel build $BAZEL_OPTIONS -- //... //api/test/... //sdk/test/...

My assumption is we have to build all without any failures, as CI would've done. Build all is rather snappy. We can probably do both bazel build, then bazel test at once?

+1 to the .gitignore changes, not sure about the .cmd. I tend to leave a CMD window open and run bazel //... commands dynamically but it could be I migrated my linux workflow to windows....

Since I don't know Bazel by heart (usually not using it), the issue I had with it is that first - the build and plugin dirs must be created. Otherwise, I believe, it was failing. Plus I had to navigate to source tree to run it. Whereas here - the batch file handles it for me automagically: it'd change dir to root, create the missing dirs if necessary, then spin the build the same as what CI would've done. But from a minimal, succinct script. I can invoke that from IDE, for example. It's slightly shorter than invoking pwsh.exe ci\do_ci.ps1 "bazel.build". Plus powershell CI script does not support custom Bazel Options.... Whereas this build-bazel.cmd batch file forwards all options passed to it to bazel.exe as options. Maybe it's just I'm too old school and I prefer batch files for simple tasks.

@maxgolov
Copy link
Contributor Author

maxgolov commented Apr 28, 2021

@jsuereth - I am not 100% sure why our CI script was only executing this for two directories.

As I read more thru that, I tried what you are recommending - and everything works fine. Adjusted the script accordingly.

I am also unsure what was the original reason why I had to create the build and plugin directory. I removed it.

Shouldn't we adjust the CI script to do just that, build all and test all

bazel.exe build //...
bazel.exe test //...

?

Works on my Windows machine. All tests are a passing.

@jsuereth
Copy link
Contributor

I only mentioned the minimum necessary setup needed to build. Not the most optimal bazel setup

@maxgolov I'm suggesting that you should also use bazelisk to avoid pain in the future, trust me :)

Don't mistake my questions for accusatiions, sorry! I love that you're trying to improve this. I'm merely asking if you had a reason to use bazel.exe vs. recommending bazelisk.

I've been waiting to write instructions on bazel usage until I can take time to install tools on a fresh machine and make sure I've gotten all the right steps (especially for bazelisk + windows, there's a lot of things I've forgotten I had to do). However, I'll just get over it and post what I think. For your sake though, would you mind trying out the bazelisk + windows instructions and let me know what didn't work?

Bazelisk release binaries

If you install bazelisk you should have a lot less pain when we upgrade bazel (which is the next PR I plan to submit so we can clean up the windows vs. linux target mess that was old bazel)

Shouldn't we adjust the CI script to do just that, build all and test all

Yes. I think at some point it didn't work on windows, but I do ~50% of my OTel C++ on bazel(isk) + Windows these days, so i've been slowly trying to make sure everything builds as I discover more.

My intent was to extract the step from our harder-to-read CI script into a standalone developer-focused build script, mainly to verify that the build is successful after my changes to any bazel build files.

Sure. If the bash script makes this easier, don't want to get in the way. Let's make sure it run bazel test //... at a minimum and bazel build //... && bazel test //... if we can. TL;DR; I'd love if we can avoid the issue we had before were some directories were unbuildable on windows :)

Again, thanks for setting this up! Hopefully my comments are helpful.

@@ -0,0 +1,6 @@
pushd "%~dp0"
set "PATH=%CD%;%PATH%"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is setting path to CWD necessary? Is this because you downloaded bazel to the project root?

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, that's not the root workspace but the $project/tools path. And indeed, that's actually where I'm downloading the bazel.exe into, so that I don't have it in my usual standard paths (or so that I can have different versions of it elsewhere in another build tree). We could change that, but it should be rather benign, as it allows you to prefer the tools/ directory tools from your current local snapshot. Kinda additional isolation of tooling in a way.

@maxgolov
Copy link
Contributor Author

maxgolov commented Apr 30, 2021

@jsuereth - I actually like how blazing fast Bazel is. It pulls a bit more than what I would've expected as dependencies (go? java? and more?). But it is really-really snappy, especially for incremental builds. I'll also give Bazilisk a try.

For CMake builds: we can switch to CMake+ninja. It also gives nice boost in terms of compilation speed. It does not necessarily cache as much as Bazel does. But also offers reasonably good alternative. Visual Studio 2019 IDE builds with cmake now use ninja by default... For CMake, of course, there is a downside that we initially also spend a bit of time installing / building dependencies at first (like, building gRPC is the slowest of them).

@lalitb lalitb merged commit fdef43b into main Apr 30, 2021
@lalitb lalitb deleted the maxgolov/tools_bazel branch April 30, 2021 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants