db-console: bazel build for protobufjs clients by koorosh · Pull Request #64065 · cockroachdb/cockroach

koorosh · 2021-04-22T13:06:45Z

This PR includes two parts (bazel definitions and changes in JS packages) which are better to review per commit.

Related to #59329

You can build these packages with following commands:

bazel build //pkg/ui/src/js:db-console_oss_js_proto
bazel build //pkg/ui/ccl/src/js:db-console_ccl_js_proto

Note: this change doesn't integrate new build targets into the main bazel build for entire project.

Previously, protobufjs cli build relied on dependencies from pkg/ui package
and required additional installation steps to "fix" known installation issue
for "protobufjs" package (see pkg/ui/bin/gen-protobuf-cli-deps.js).

Now, these dependencies are defined in pkg/ui/src/js directory with additional
dependencies required for "protobufjs" package. It resolves the problem with
additional installation steps and makes build process straightforward.

This change introduces new bazel build targets:

pkg/ui/src/js
pkg/ui/ccl/ui/src/js
These targets are intended to build protobufjs client libs for
farther usage by db-console (pkg/ui) and as source for
crdb-protobuf-client package which is published to NPM registry.

In addition to new targets, pkg/server/serverpb BUILD targets
are refactored as well - single serverpb_proto target is removed
in favor of separate protobuf_library targets per each .proto file.
It allows to reuse them in different targets.

Release note: None

cockroach-teamcity · 2021-04-22T13:06:53Z

This change is

koorosh · 2021-04-23T10:12:22Z

@rickystewart , @jlinder , I just started working on bazel build process for db-console (crdb ui part) and current PR is intended to build JS packages for protobuf js client code which is currently performed with following Make commands:

cockroach/Makefile

Lines 1324 to 1334 in afaea24

    
           .SECONDARY: $(UI_JS_CCL) 
        
           $(UI_JS_CCL): $(GW_PROTOS) $(GO_PROTOS) $(JS_PROTOS_CCL) pkg/ui/yarn.installed | bin/.submodules-initialized 
        
           	# Add comment recognized by reviewable. 
        
           	echo '// GENERATED FILE DO NOT EDIT' > $@ 
        
           	$(PBJS) -t static-module -w es6 --strict-long --keep-case --path pkg --path ./vendor/github.com --path $(GOGO_PROTOBUF_PATH) --path $(ERRORS_PATH) --path $(COREOS_PATH) --path $(GRPC_GATEWAY_GOOGLEAPIS_PATH) $(filter %.proto,$(GW_PROTOS) $(JS_PROTOS_CCL)) >> $@ 
        
           .SECONDARY: $(UI_JS_OSS) 
        
           $(UI_JS_OSS): $(GW_PROTOS) $(GO_PROTOS) pkg/ui/yarn.installed | bin/.submodules-initialized 
        
           	# Add comment recognized by reviewable. 
        
           	echo '// GENERATED FILE DO NOT EDIT' > $@ 
        
           	$(PBJS) -t static-module -w es6 --strict-long --keep-case --path pkg --path ./vendor/github.com --path $(GOGO_PROTOBUF_PATH) --path $(ERRORS_PATH) --path $(COREOS_PATH) --path $(GRPC_GATEWAY_GOOGLEAPIS_PATH) $(filter %.proto,$(GW_PROTOS)) >> $@

These changes don't affect current build process with Makefile and aren't included into main bazel build for entire project.
It's still in a draft but will appreciate any comments/feedback on this change! thanks!

rickystewart · 2021-04-26T22:02:04Z

@@ -0,0 +1,108 @@
+"implementation is borrowed from https://github.com/bazelbuild/rules_nodejs/blob/stable/examples/protobufjs/defs.bzl"


I think this docstring pattern isn't really used anywhere else in tree? You can just replace this with a comment.

rickystewart · 2021-04-26T22:10:30Z

+    name = "db-console_oss_js_proto",
+    out_name = "protos",
+    protos = [
+      "//pkg/server/serverpb:admin_proto",


I understand you probably separated these protos out into individual targets so we could compile only the protos that HAVE to be in this JS library. Does it make a huge difference to just compile //pkg/server/serverpb:serverpb_proto, though? What's the impact? Code size in the generated file? Can/does that get squashed later in minification?

Good point! The size of generated file was the primary concern for doing this but you're right, it could be reduced with minification and tree shaking of required code in generated file!

rickystewart · 2021-04-26T22:16:29Z

+
+# Define proto_library per each .proto file according to recommended code organization:
+# https://docs.bazel.build/versions/master/be/protocol-buffer.html#proto_library
 proto_library(


So, this stuff is all auto-generated (if you check out the failing lint job, you'll see that it's yelling at you to run make bazel-generate -- but if you do, that'll trample all the changes you've made here).

Gazelle auto-generates the structure of the proto rules so that we don't have to manually update these dependencies ourselves. Any solution we arrive at for this will have to honor all the existing Gazelle-based machinery here. A couple high-level options (sorry, these are all hand-wavy, it's a complicated problem):

Exclude the .proto files in pkg/server/serverpb from being managed by Gazelle. This would mean that any change to the dependencies of any of the .proto files in here would have to be manually implemented in the BUILD files. As far as I'm concerned, this is a worst-case scenario -- I think we would have lots of angry engineers if we forced that on them.

Maybe you can adjust the gazelle:proto_group directive in this directory to achieve the same thing, where we have one proto_library per .proto file? Worth playing with and seeing what that gets you.

Gazelle is supposed to support extensions to accomplish this sort of thing. I am not sure how much work it would be to hack something together that accomplishes the same thing as this PR, but it is worth at least considering because it would be really nice to have one tool that's responsible for all our BUILD files in every language. (Edit: To be clear, I suspect this is probably more work than it's worth, but nothing's stopping us from looking.)

@rickystewart , thanks for detailed suggestions! I would prefer to avoid manual changes in .proto files as well.
Taking into account that generated JS protobuf client can be minified to reduce file size, it might be okay just rely on generated serverpb_proto proto.

rickystewart · 2021-04-26T22:17:13Z

+    attrs = {"protos": attr.label_list(providers = [ProtoInfo])},
+)
+
+def protobufjs_library(name, out_name, protos, **kwargs):


I didn't thoroughly review this, but it looks as I would expect.

Previously, protobufjs cli build relied on dependencies from `pkg/ui` package and required additional installation steps to "fix" known installation issue for "protobufjs" package. Now, these dependencies are defined in `pkg/ui/src/js` directory with additional dependencies required for "protobufjs" package. It resolves the problem with additional installation steps and makes build process straightforward. Release note: None

This change introduces new bazel build targets: - pkg/ui/src/js - pkg/ui/ccl/ui/src/js These targets are intended to build protobufjs clint libs for farther usage by db-console (`pkg/ui`) and as source for `crdb-protobuf-client` package which is published to NPM registry. In addition to new targets, `pkg/server/serverpb` BUILD targets are refactored as well - single `serverpb_proto` target is removed in favor of separate `protobuf_library` targets per each .proto file. It allows to reuse them in different targets. Release note: None

koorosh · 2021-11-17T07:18:37Z

Not actual anymore

koorosh force-pushed the cosole-db-protobufjs-bazel-build branch from f1d60ae to 2da0367 Compare April 22, 2021 13:09

koorosh marked this pull request as draft April 22, 2021 13:52

rickystewart reviewed Apr 26, 2021

View reviewed changes

koorosh force-pushed the cosole-db-protobufjs-bazel-build branch from 2da0367 to 1e0112f Compare May 27, 2021 18:43

koorosh added 3 commits June 10, 2021 12:00

wip

df1d089

koorosh force-pushed the cosole-db-protobufjs-bazel-build branch from 1e0112f to df1d089 Compare June 10, 2021 10:26

koorosh closed this Nov 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

db-console: bazel build for protobufjs clients#64065

db-console: bazel build for protobufjs clients#64065
koorosh wants to merge 3 commits intocockroachdb:masterfrom
koorosh:cosole-db-protobufjs-bazel-build

koorosh commented Apr 22, 2021 •

edited

Loading

Uh oh!

cockroach-teamcity commented Apr 22, 2021

Uh oh!

koorosh commented Apr 23, 2021

Uh oh!

rickystewart Apr 26, 2021

Uh oh!

rickystewart Apr 26, 2021

Uh oh!

koorosh May 31, 2021

Uh oh!

rickystewart Apr 26, 2021 •

edited

Loading

Uh oh!

koorosh Jun 1, 2021

Uh oh!

rickystewart Apr 26, 2021

Uh oh!

koorosh commented Nov 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		@@ -0,0 +1,108 @@
		"implementation is borrowed from https://github.com/bazelbuild/rules_nodejs/blob/stable/examples/protobufjs/defs.bzl"

Conversation

koorosh commented Apr 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Apr 22, 2021

Uh oh!

koorosh commented Apr 23, 2021

Uh oh!

rickystewart Apr 26, 2021

Choose a reason for hiding this comment

Uh oh!

rickystewart Apr 26, 2021

Choose a reason for hiding this comment

Uh oh!

koorosh May 31, 2021

Choose a reason for hiding this comment

Uh oh!

rickystewart Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

koorosh Jun 1, 2021

Choose a reason for hiding this comment

Uh oh!

rickystewart Apr 26, 2021

Choose a reason for hiding this comment

Uh oh!

koorosh commented Nov 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

koorosh commented Apr 22, 2021 •

edited

Loading

rickystewart Apr 26, 2021 •

edited

Loading