refactor!: bazel build #442

alexander-fenster · 2020-04-24T16:35:06Z

Implementing Bazel build for gapic-generator-typescript.

You can still use npm install, npm run compile, and npm test (they will work with the help of @bazel/bazelisk), but having a local bazel installation is highly recommended. To make bazelisk use the local installation, add this to your environment:

export USE_BAZEL_VERSION=`which bazel`  # path to your local Bazel

The code was rearranged and refactored a little bit. The main difference is that everywhere where we want to call something (protoc, or pass a plugin to protoc, or call the generator from tests), we call a shell script provided by Bazel. At the same time, I made sure that it still works if packed into Docker image with npm pack (with no Bazel stuff inside at all), so the transition for Yoshi should be very easy: we can keep publishing Docker images with generator updates and move libraries to Bazel build at the same time.

Please check the updated README.md (it shows how to call the generator using Bazel). Regular npm install -g . will also work, but with Bazel, you won't need to have protoc installed locally.

Please feel free to ask any questions :)

README.md

JustinBeckwith

Forgive me, I don't know the first thing about bazel

.circleci/config.yml

.gitignore

JustinBeckwith · 2020-05-08T18:29:35Z

BUILD.bazel

+)
+
+npm_runtime_dependencies = [
+    "@npm//file-system",


How does this interact with the dependencies defined in package.json? Do you need to maintain this list in two places?

Yes. https://bazelbuild.github.io/rules_nodejs/Built-ins.html

So ... what happens here when renovate comes along and updates our package.json and package-lock.json?

Nothing bad - next time bazel build is run, it will figure out that package-lock.json has changed, and will fetch new dependencies. (I assume renovate knows how to deal with package-lock.json, I think it does)

Note that here in BUILD file it's just a list of packages without versions, so version updates by renovate are not going to affect this in any way.

Ahhhh - what happens when you add a new package to package.json?

You go here, decide if it's a compile, or test, or runtime dependency, and add it manually to one of the lists. They call it "fine grained npm dependencies", you see.

How it works internally: after they do yarn install or npm install (whatever), they actually bazelify all the dependencies (auto-generate BUILD.bazel file for all of them) so that they become "targets" in bazel world (think make targets). Making bazel task depend on "just all the crap from node_modules" is not something they approve (that was deprecated long time ago), so yes, we list dependencies in two places.

BUILD.bazel

JustinBeckwith · 2020-05-08T18:30:31Z

BUILD.bazel

+npm_test_dependencies = npm_runtime_dependencies + [
+    "@npm//assert-rejects",
+    "@npm//c8",
+    "@npm//codecov",


Do you specifically call into codecov, or rely on the GitHub Action like we do in libs?

I'm not calling it, this should be removed from here. Actually, I have no idea how to call codecov and c8 from bazel but I'll figure out :)

package.json

JustinBeckwith · 2020-05-08T18:33:33Z

package.json

-    "compile": "tsc -p .",
-    "compile-protos": "pbjs -p protos -p node_modules/google-gax/protos -t static-module -o pbjs-genfiles/plugin.js google/protobuf/compiler/plugin.proto google/api/annotations.proto google/api/field_behavior.proto google/api/resource.proto google/api/client.proto google/longrunning/operations.proto service_config.proto && pbts pbjs-genfiles/plugin.js -o pbjs-genfiles/plugin.d.ts",
+    "baseline": "node bazel-bin/tools/update-baselines.js",
+    "clean": "bazel clean && rm -rf build",


I'm surprised bazel wouldn't know build is a build artifact and clean it up

The thing is, build is not a build artifact for bazel :) It puts its stuff into bazel-* folders that are actually symlinks pointing directly to hell.

$ ls -l bazel-* lrwxr-xr-x 1 fenster 5001 131 May 8 01:58 bazel-bin -> /private/var/tmp/_bazel_fenster/a716da2979244e454b0762cc74541b5d/execroot/gapic_generator_typescript/bazel-out/darwin-fastbuild/bin lrwxr-xr-x 1 fenster 5001 100 May 8 01:58 bazel-gapic-generator-typescript -> /private/var/tmp/_bazel_fenster/a716da2979244e454b0762cc74541b5d/execroot/gapic_generator_typescript lrwxr-xr-x 1 fenster 5001 110 May 8 01:58 bazel-out -> /private/var/tmp/_bazel_fenster/a716da2979244e454b0762cc74541b5d/execroot/gapic_generator_typescript/bazel-out lrwxr-xr-x 1 fenster 5001 136 May 8 01:58 bazel-testlogs -> /private/var/tmp/_bazel_fenster/a716da2979244e454b0762cc74541b5d/execroot/gapic_generator_typescript/bazel-out/darwin-fastbuild/testlogs

package.json

JustinBeckwith · 2020-05-08T18:35:03Z

package.json

@@ -45,20 +42,24 @@
    "get-stdin": "^7.0.0",
    "google-gax": "^2.3.1",
    "js-yaml": "^3.13.1",
+    "module-alias": "^2.2.2",


Adding a runtime dependency in pursuit of a build toolchain change doesn't feel right

Agreed, like 100% agreed. This is all about the protos.js that get compiled and placed into random location, which TypeScript can figure out during compile time but Node cannot find in the runtime. Let me add a huge TODO here, we must get rid of this, but as of today, I don't have a good idea how exactly to do it.

tsconfig.json

bcoe · 2020-05-08T19:46:20Z

BUILD.bazel

+    "@npm//get-stdin",
+    "@npm//google-gax",
+    "@npm//js-yaml",
+    "@npm//module-alias",


I find the fact that we need to declare our deps twice pretty ugly.

summer-ji-eng · 2020-05-08T21:05:12Z

README.md

+`-I "$GOOGLEAPIS"` means pass the `googleapis` to `protoc`
+`--output-dir /tmp/translate-v3-typescript` is where to put the result
+`--grpc-service-config "$GOOGLEAPIS/google/cloud/translate/v3/translate_grpc_service_config.json"`
+is an optional configuration file for timeouts and stuff
 Then we add all the `translate` `v3` proto file to the command line, as well as the
 proto file that defines common resources (some APIs need it, some others don't).


Thanks a lot updating the READ.md
Tested on my local, it works for me.
Congrats 👍

vam-google

LGTM, with few questions.

Do we need to check-in the package-lock.json file? It is huge and constitutes most of the new lines committed in this PR.
Also, how is it created (updated)? Why is it so big (does generator have so many dependencies in it)?

BUILD.bazel

vam-google · 2020-05-12T03:11:50Z

package-lock.json

@@ -0,0 +1,4835 @@
+{


How is this file created and how is it supposed to be updated?

It's created by npm and it's used by bazel to ensure reproducibility. See here: https://bazelbuild.github.io/rules_nodejs/#hermeticity-and-reproducibility

vam-google · 2020-05-12T03:20:35Z

BUILD.bazel

+load("@npm_bazel_labs//:index.bzl", "protobufjs_ts_library")
+
+proto_library(
+    name = "google_protobuf_protos",


The idiomatic way for bazel is to have BUIDL.bazel file in each sub-folder (which turns that folder and all its subfolders without their own BUILD.bazel files into a "package" from bazel's perspective).
For example, please consider putting proto_library targets which compile stuff under node_modules/google-gax/build/protos into a BUIDL.bazel file under same directory (i.e. node_modules/google-gax/build/protos).

Let me postpone this as a possible refactor - I see your point but for now I would prefer not to introduce any BUILD.bazel files to google-gax (the reason for that is that build/protos folder in google-gax is auto-generated and I cannot just put stuff there; also, it's a user-facing folder).

README.md

alexander-fenster requested a review from vam-google April 24, 2020 16:35

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 24, 2020

zamnuts mentioned this pull request May 3, 2020

Ensure "clean" in all repos and add clean during "precompile" googleapis/synthtool#487

Closed

alexander-fenster force-pushed the we-need-bazel branch 2 times, most recently from 04bb3c3 to fa5cd49 Compare May 7, 2020 07:33

alexander-fenster mentioned this pull request May 8, 2020

chore: release 1.0.0 #475

Closed

alexander-fenster force-pushed the we-need-bazel branch 10 times, most recently from 68e86ac to d91aae9 Compare May 8, 2020 12:27

refactor!: bazel build

7f5b7cb

alexander-fenster force-pushed the we-need-bazel branch from d91aae9 to 7f5b7cb Compare May 8, 2020 12:35

alexander-fenster marked this pull request as ready for review May 8, 2020 12:42

alexander-fenster requested review from xiaozhenliu-gg5, summer-ji-eng, JustinBeckwith and bcoe May 8, 2020 12:42

summer-ji-eng reviewed May 8, 2020

View reviewed changes

README.md Show resolved Hide resolved

JustinBeckwith reviewed May 8, 2020

View reviewed changes

alexander-fenster force-pushed the we-need-bazel branch from e010950 to e3afdda Compare May 8, 2020 19:26

fix: PR feedback

1e8ba2f

alexander-fenster force-pushed the we-need-bazel branch from e3afdda to 1e8ba2f Compare May 8, 2020 19:33

bcoe reviewed May 8, 2020

View reviewed changes

summer-ji-eng reviewed May 8, 2020

View reviewed changes

vam-google reviewed May 12, 2020

View reviewed changes

JustinBeckwith approved these changes May 12, 2020

View reviewed changes

alexander-fenster added 2 commits May 17, 2020 01:37

fix: PR feedback

3097fce

Merge branch 'master' into we-need-bazel

d3e1812

alexander-fenster force-pushed the we-need-bazel branch from 9c5ed25 to 1568c20 Compare May 17, 2020 08:54

test: fix circleci

a614838

alexander-fenster force-pushed the we-need-bazel branch from 1568c20 to a614838 Compare May 17, 2020 09:08

alexander-fenster merged commit 79a5eaf into master May 17, 2020

alexander-fenster deleted the we-need-bazel branch May 17, 2020 09:34

release-please bot mentioned this pull request May 21, 2020

chore: release 0.99.0 #515

Merged

release-please bot mentioned this pull request Jan 4, 2022

chore(main): release gapic-generator-typescript 1.0.0 #1078

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor!: bazel build #442

refactor!: bazel build #442

alexander-fenster commented Apr 24, 2020 •

edited

JustinBeckwith left a comment

JustinBeckwith May 8, 2020

alexander-fenster May 8, 2020

JustinBeckwith May 8, 2020

alexander-fenster May 8, 2020

alexander-fenster May 8, 2020

JustinBeckwith May 8, 2020

alexander-fenster May 8, 2020 •

edited

JustinBeckwith May 8, 2020

alexander-fenster May 8, 2020

JustinBeckwith May 8, 2020

alexander-fenster May 8, 2020

JustinBeckwith May 8, 2020

alexander-fenster May 8, 2020

bcoe May 8, 2020

summer-ji-eng May 8, 2020

vam-google left a comment

vam-google May 12, 2020

alexander-fenster May 12, 2020

vam-google May 12, 2020

alexander-fenster May 12, 2020

refactor!: bazel build #442

refactor!: bazel build #442

Conversation

alexander-fenster commented Apr 24, 2020 • edited

JustinBeckwith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-fenster May 8, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vam-google left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-fenster commented Apr 24, 2020 •

edited

alexander-fenster May 8, 2020 •

edited