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.

Sign up for GitHub

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

Jump to bottom

refactor!: bazel build #442

Merged
merged 5 commits into from
May 17, 2020
Merged

refactor!: bazel build #442

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7f5b7cb
refactor!: bazel build
alexander-fenster Apr 17, 2020
1e8ba2f
fix: PR feedback
alexander-fenster May 8, 2020
3097fce
fix: PR feedback
alexander-fenster May 17, 2020
d3e1812
Merge branch 'master' into we-need-bazel
alexander-fenster May 17, 2020
a614838
test: fix circleci
alexander-fenster May 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 50 additions & 41 deletions .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,72 +2,83 @@ version: 2.1
jobs:
testGenerator:
docker:
- image: circleci/node:10-browsers
- image: l.gcr.io/google/bazel
working_directory: /home/circleci/project/gapic-generator-typescript
steps:
- checkout
- run:
name: Install protoc
command: |
wget https://github.com/protocolbuffers/protobuf/releases/download/v3.10.0/protoc-3.10.0-linux-x86_64.zip
cd / && sudo unzip /tmp/protoc-3.10.0-linux-x86_64.zip
protoc --version
working_directory: /tmp
- run:
name: Run npm install
command: |
npm install
- run:
name: Run generator unit tests
command: |
npm link
npm test
environment:
NPM_CONFIG_PREFIX: /tmp/.npm-global
- run:
name: Run linting
bazel test //:unit_tests
- run:
name: Unzip baseline tests output
command: |
npm run lint
mkdir /home/circleci/project/baselines
unzip bazel-testlogs/unit_tests/test.outputs/outputs.zip -d /home/circleci/project/baselines
- run:
name: copy protos to generated client library & move .test-out-* to project/out/
name: Copy protos to baselines
command: |
for dir in .test-out-* ; do
for dir in /home/circleci/project/baselines/.test-out-* ; do
cp -r test-fixtures/protos $dir
done
- run:
name: move test out folders to a separate folder
- run:
name: Copy compiled test applications
# bazel-bin/ is a symlink to /root/.cache/bazel/... and we won't have it
# available in Node container. Let's just save what we need.
command: |
mkdir /home/circleci/project/baselines
cp -r .test-out-* /home/circleci/project/baselines
mkdir test-application-runners
cp bazel-bin/test/test-application/test-*.js test-application-runners/
cp bazel-testlogs/unit_tests/test.outputs/outputs.zip test-application-runners/
- persist_to_workspace:
root: /home/circleci/project
paths:
- gapic-generator-typescript
- baselines
lint:
docker:
- image: circleci/node:10-browsers
JustinBeckwith marked this conversation as resolved.
Show resolved Hide resolved
steps:
- attach_workspace:
at: /home/circleci/workspace
- run:
name: Run lint
working_directory: /home/circleci/workspace/gapic-generator-typescript
command: |
rm -f bazel-* # we don't need these broken symlinks here
# we have some extra stuff here in Circle that we don't want to lint
echo test-application-runners >> .eslintignore
echo test-application-runners >> .prettierignore
# and now finally...
npm run lint
showcaseTestApplications:
docker:
- image: circleci/node:10-browsers
steps:
- checkout
steps:
- attach_workspace:
at: /home/circleci/workspace
- run:
name: Prepare test applications
working_directory: /home/circleci/workspace/gapic-generator-typescript
command: |
rm -f bazel-testlogs
mkdir -p bazel-testlogs/unit_tests/test.outputs
cp test-application-runners/outputs.zip bazel-testlogs/unit_tests/test.outputs/
- run:
name: Run showcase test for Typescript users
working_directory: /home/circleci/workspace/gapic-generator-typescript
command: npm run ts-test-application
command: npx mocha test-application-runners/test-ts.js
environment:
NPM_CONFIG_PREFIX: /tmp/.npm-global
- run:
name: Run showcase test for JavaScript users
working_directory: /home/circleci/workspace/gapic-generator-typescript
command: npm run js-test-application
command: npx mocha test-application-runners/test-js.js
environment:
NPM_CONFIG_PREFIX: /tmp/.npm-global
showcaseLibTest:
docker:
- image: circleci/node:10-browsers
steps:
- checkout
steps:
- attach_workspace:
at: /home/circleci/workspace
- run:
Expand All @@ -84,8 +95,7 @@ jobs:
kmsLibTest:
docker:
- image: circleci/node:10-browsers
steps:
- checkout
steps:
- attach_workspace:
at: /home/circleci/workspace
- run:
Expand All @@ -95,8 +105,7 @@ jobs:
translateLibTest:
docker:
- image: circleci/node:10-browsers
steps:
- checkout
steps:
- attach_workspace:
at: /home/circleci/workspace
- run:
Expand All @@ -106,8 +115,7 @@ jobs:
monitoringLibTest:
docker:
- image: circleci/node:10-browsers
steps:
- checkout
steps:
- attach_workspace:
at: /home/circleci/workspace
- run:
Expand All @@ -117,8 +125,7 @@ jobs:
dlpLibTest:
docker:
- image: circleci/node:10-browsers
steps:
- checkout
steps:
- attach_workspace:
at: /home/circleci/workspace
- run:
Expand All @@ -128,8 +135,7 @@ jobs:
ttsLibTest:
docker:
- image: circleci/node:10-browsers
steps:
- checkout
steps:
- attach_workspace:
at: /home/circleci/workspace
- run:
Expand All @@ -141,6 +147,9 @@ workflows:
tests:
jobs:
- testGenerator
- lint:
requires:
- testGenerator
- showcaseTestApplications:
requires:
- testGenerator
Expand Down
3 changes: 2 additions & 1 deletion .eslintignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,5 @@ build/
docs/
protos/
test-fixtures/
pbjs-genfiles/
bazel-*/
coverage/
2 changes: 1 addition & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ config.yaml
ignored
build
.vscode
package-lock.json
JustinBeckwith marked this conversation as resolved.
Show resolved Hide resolved
pbjs-genfiles/
.DS_Store
.idea
Expand All @@ -26,3 +25,4 @@ docker/gitlog.txt
.showcase-typescript
.test-application-js
.test-application-ts
bazel-*
1 change: 1 addition & 0 deletions .prettierignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@
build/
docs/
protos/
bazel-*/
164 changes: 164 additions & 0 deletions BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,164 @@
# Allow any ts_library rules in this workspace to reference the config
exports_files(["tsconfig.json"], visibility = ["//:__subpackages__"])

package(default_visibility = ["//visibility:public"])

load("@build_bazel_rules_nodejs//:index.bzl", "nodejs_binary", "nodejs_test")
load("@npm_bazel_typescript//:index.bzl", "ts_project", "ts_config")
load("@npm_bazel_labs//:index.bzl", "protobufjs_ts_library")

proto_library(
name = "google_protobuf_protos",
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

srcs = glob(["node_modules/google-gax/build/protos/google/protobuf/*.proto"]) +
[
"node_modules/google-gax/build/protos/google/protobuf/compiler/plugin.proto",
],
strip_import_prefix = "node_modules/google-gax/build/protos",
)

proto_library(
name = "google_rpc_protos",
deps = [
":google_protobuf_protos",
],
srcs = glob(["node_modules/google-gax/build/protos/google/rpc/*.proto"]),
strip_import_prefix = "node_modules/google-gax/build/protos/",
)

proto_library(
name = "google_api_protos",
deps = [
":google_protobuf_protos",
],
srcs = glob(["node_modules/google-gax/build/protos/google/api/*.proto"]),
strip_import_prefix = "node_modules/google-gax/build/protos",
)

proto_library(
name = "google_longrunning_protos",
deps = [
":google_protobuf_protos",
":google_api_protos",
":google_rpc_protos",
],
srcs = ["node_modules/google-gax/build/protos/google/longrunning/operations.proto"],
strip_import_prefix = "node_modules/google-gax/build/protos",
)

proto_library(
name = "service_config_proto",
deps = [
":google_protobuf_protos",
":google_rpc_protos",
],
srcs = ["protos/service_config.proto"],
strip_import_prefix = "protos/",
)

protobufjs_ts_library(
name = "protos",
deps = [
":google_api_protos",
":google_longrunning_protos",
":service_config_proto",
]
)

filegroup(
name = "templates",
srcs = glob(["templates/**/*.*", "templates/**/.*"]),
)

filegroup(
name = "test_fixtures",
srcs = glob(["test-fixtures/**/*.*"]),
)

filegroup(
name = "baselines",
srcs = glob(["baselines/**/*.*", "baselines/**/.*"]),
)

npm_runtime_dependencies = [
"@npm//file-system",
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@alexander-fenster alexander-fenster May 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

"@npm//fs-extra",
"@npm//get-stdin",
"@npm//google-gax",
"@npm//js-yaml",
"@npm//module-alias",
Copy link
Contributor

Choose a reason for hiding this comment

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

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

"@npm//nunjucks",
"@npm//object-hash",
"@npm//protobufjs",
"@npm//yargs",
]

npm_test_dependencies = npm_runtime_dependencies + [
"@npm//assert-rejects",
JustinBeckwith marked this conversation as resolved.
Show resolved Hide resolved
"@npm//espower-typescript",
"@npm//sinon",
"@npm//mocha",
]

npm_compile_dependencies = npm_test_dependencies + [
"@npm//@types/js-yaml",
"@npm//@types/fs-extra",
"@npm//@types/get-stdin",
"@npm//@types/mocha",
"@npm//@types/module-alias",
"@npm//@types/node",
"@npm//@types/nunjucks",
"@npm//@types/object-hash",
"@npm//@types/sinon",
"@npm//@types/yargs",
"@npm//gts",
]

ts_project(
name = "compile",
srcs = glob([
"src/**/*.ts",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this effectively copying what's happening in tsconfig.json? Do we need to coordinate changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TypeScript support is really wobbly in bazel :) They use tsconfig.json to get some compiler options from there, but the tasks must still specifically define their inputs so that bazel could've built a dependency tree (think make on steroids).

"test/**/*.ts",
"tools/**/*.ts",
"index.d.ts"
]),
tsconfig = "//:tsconfig.json",
extends = ["@npm//:node_modules/gts/tsconfig-google.json"],
source_map = True,
declaration = True,
deps = [
":protos",
] + npm_compile_dependencies,
)

nodejs_binary(
name = "protoc_plugin",
entry_point = "//:src/protoc-plugin.js",
data = [
":compile",
":templates",
":protos",
] + npm_runtime_dependencies,
)

nodejs_binary(
name = "gapic_generator_typescript",
entry_point = "//:src/gapic-generator-typescript.js",
data = [
":protoc_plugin",
":compile",
":templates",
":protos",
"@com_google_protobuf//:protoc",
] + npm_runtime_dependencies,
)

nodejs_test(
name = "unit_tests",
entry_point = "@npm//:node_modules/mocha/bin/mocha",
args = ["test/unit/"],
data = [
":gapic_generator_typescript",
":test_fixtures",
":baselines",
] + npm_test_dependencies,
)
Loading