build: Add Bazel java_grpc_library rule #2975

simonhorlick · 2017-05-04T07:12:28Z

Bazel third party dependencies are specified in repositories.bzl which
gives the consumer the ability to opt-out of any dependencies they use
directly in their own project. Also note that reposititories.bzl
uses java_import_external from rules_closure instead of Bazel's built in
maven_jar. This allows us to specify multiple urls for each jar,
reducing the likelyhood of build failures due to repositories becoming
unavailable and also define dependencies between libraries.

Fixes #2756

References: bazelbuild/bazel#1952

grpc-kokoro · 2017-05-04T07:35:02Z

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure.

buchgr · 2017-05-05T12:24:06Z

ok to test

buchgr

I glanced over it and it looks good.

Two things:

Tests for java_grpc_library would be great
I think maven_jar might be better than copying java_import_external from the closure rules.

ejona86 · 2017-05-05T16:05:43Z

Tests for java_grpc_library would be great

I can add some tests post-merge.

carl-mastrangelo · 2017-05-05T17:37:55Z

BUILD

@@ -0,0 +1,126 @@
+package(default_visibility = ["//visibility:public"])
+
+cc_binary(


Consider moving these build targets into the directory that they reference. For example, this rule should probably go in a BUILD file under compiler/

carl-mastrangelo · 2017-05-05T17:39:17Z

BUILD

+java_library(
+    name = "core",
+    srcs = glob([
+        "core/src/main/java/**/*.java",


Please don't use super globs. We are trying to split up the core and core internal package, and this pulls it in. It also unnnessarily pulls in inprocess and util.

carl-mastrangelo · 2017-05-05T17:40:10Z

BUILD

+)
+
+java_library(
+    name = "protobuf-nano",


Please be consistent with underscores and dashes for name rules. I think underscores are more common.

In that case, if I move the BUILD file to protobuf-nano should I keep the target the same as the directory name, or still replace with underscores?

underscores

ejona86 · 2017-05-10T15:46:03Z

@simonhorlick I'm a bit busy at the moment, so it may be a week before I can look at this closer.

simonhorlick · 2017-05-11T08:03:10Z

It looks like my change to the gradle output directory is causing the Windows Jenkins build to fail. Presumably because it can't find the test outputs any more.

Code taken from PR: grpc/grpc-java#2975 Once this is merged into gRPC we can directly depend on gRPC. Change-Id: Ide77414276b4125e85dfa6f93725143e3149d8ec

Bazel third party dependencies are specified in repositories.bzl which gives the consumer the ability to opt-out of any dependencies they use directly in their own project. Fixes grpc#2756

This reverts commit 98bee07.

simonhorlick · 2017-06-13T07:26:40Z

In 98bee07 I renamed the output directory that gradle uses because it clashes with the BUILD files. Alternatively it seems Bazel supports naming build files BUILD.bazel, which might be a less invasive way of achieving the same thing. Let me know if you think that would be better.

buchgr · 2017-06-13T17:18:49Z

java_grpc_library.bzl

+  ctx.action(
+      inputs = [ctx.executable._java_plugin] + srcs + includes,
+      outputs = [srcdotjar],
+      executable = ctx.executable._protoc,


When hacking this into bazel [1], I found that creating the action this way let's protoc crash on Windows. Instead, I am building the command line myself.

I am quite sure this is a bazel bug, but haven't had time to investigate yet.

[1] https://github.com/bazelbuild/bazel/blob/master/third_party/grpc/build_defs.bzl

I don't want us to make the command line ourselves, as then we have to deal with more possibilities of escaping. I'd rather us figure out what's wrong with using executable on Windows. That can happen after the merging of this PR.

ejona86 · 2017-06-16T20:40:10Z

@simonhorlick, Hmm... The BUILD vs build/ is an interesting problem. We'll have a similar problem internally as we have pre-existing Blaze BUILD files and we'll probably want both of them around for a bit as we migrate to have one canonical set. I've started a conversation internally asking about Blaze vs Bazel behavior; depending on how the two differ I'll be able to provide a recommendation.

ejona86 · 2017-06-16T22:34:35Z

@simonhorlick, I'm pretty sure I want to use BUILD.bazel for now. I think it will make life easier for us as we support Blaze internally and also be convenient during migrating to a single set of BUILD files (Blaze+Bazel). In the future we may actually swap to using BUILD, but that's still TBD.

ejona86

This looks pretty good.

ejona86 · 2017-06-17T00:04:47Z

BUILD

@@ -0,0 +1,11 @@
+package(default_visibility = ["//visibility:public"])


Please don't set the default visibility to public. It makes it far too easy to leak targets. I say this as someone who has been surprised multiple times to learn a target was public and then needed to clean it up. Yes, I know it'll be annoying now because everything is actually public, but once we start adding tests and using java_libraries it is asking for trouble.

ejona86 · 2017-06-17T00:05:22Z

BUILD

+package(default_visibility = ["//visibility:public"])
+
+java_library(
+    name = "grpc_java",


What is this target used for?

I was using it just to reduce the number of deps in rules that use grpc-java. You can see an example here: https://github.com/simonhorlick/grpc-java-helloworld/blob/master/java/io/grpc/examples/helloworld/BUILD#L44

We had one of these targets internally, and it caused problems, because tons got added to it. If it is just core+stub, that seems like it could be fine. But on the other hand, one vs two targets for deps isn't that bad. @carl-mastrangelo, thoughts?

ejona86 · 2017-06-17T00:17:52Z

repositories.bzl

+    actual = "@com_google_code_gson//jar",
+  )
+
+def com_google_api_grpc_google_common_protos():


Was there a tool that created this stuff or that can help maintain it?

I made this manually, I'm not actually sure if there are tools available.

ejona86

This is very close now, in my mind. @buchgr, wanna take another look?

ejona86 · 2017-06-19T19:26:29Z

BUILD

+package(default_visibility = ["//visibility:public"])
+
+java_library(
+    name = "grpc_java",


We had one of these targets internally, and it caused problems, because tons got added to it. If it is just core+stub, that seems like it could be fine. But on the other hand, one vs two targets for deps isn't that bad. @carl-mastrangelo, thoughts?

ejona86 · 2017-06-19T19:26:50Z

BUILD

+    exports = [
+        "//core",
+        "//stub",
+        "@com_google_code_findbugs_jsr305//jar",


Remove export of jsr305 and guava.

I would strongly prefer leaving the meta target out for now. It's something we can easily add in later.

There are a couple reasons for this:

Right now auto generated build files and tools that auto add deps have a hard time figuring out what to do when a file is exposed through multiple targets. I think that making life easier for tools is a better long term strategy than making life easier for humans. (the former is a better deal for the latter).

Bazel is not currently smart enough to trim deps not actually needed. For example, IIRC core depends on the Census library. Even if users really only needed stub, they would pull in Census too (I realize that stub depends on core, but that's a much easier thing to fix). Same with the auth library, which depends on Google client libraries. (this had the sad effect of pulling in additional licences for hundreds of Android apps that are infeasible to back out, despite none of them actually needing it)

Bazel cannot figure out which deps are needed for compilation. It knows if they are missing, but it cannot decide if they happened to be unnecessary. This bloats jars. There have been at least 5 or 6 tools internally that try to find unnecessary deps, but in practice they each failed on a different set of corner cases, and have a pretty high rate of being ignored.

The temptation is strong to put everything in the grpc_java, especially for new deps. (this happened for grpc's predecessor, and led to no jar being smaller than 200MB.) Let's stay lean and mean for now, and add this target back in once there is strong evidence that it would be widely useful.

Thanks for the explanation, that makes a lot of sense.

ejona86 · 2017-06-19T19:29:17Z

core/BUILD.bazel

    name = "internal",
    srcs = glob([
        "src/main/java/io/grpc/internal/*.java",
    ]),
+    visibility = ["//visibility:public"],


It seems this should have reduced visibility. I guess everything in grpc-java can use it, though? I guess I don't know how cross-repo visibility works in Bazel...

I updated this to visibility = ["//:__subpackages__"] and then tested it by depending on this target from another repo, bazel throws a visibility error as expected.

carl-mastrangelo · 2017-06-20T04:05:45Z

okhttp/BUILD.bazel

+        "src/main/java/**/*.java",
+    ]),
+    resources = glob([
+        "src/main/resources/**/*",


/** should be sufficient no?

carl-mastrangelo · 2017-06-20T04:07:43Z

repositories.bzl

+
+def com_google_api_grpc_google_common_protos():
+  native.maven_jar(
+      name = "com_google_api_grpc_google_common_protos",


please sort args, or at least make arg order consistent across this and the the rules below. (e.g. sha1 comes after name)

Bazel requires an empty BUILD file here so users can reference //:repositories.bzl and //:java_grpc_library.bzl.

ejona86

Let's give @buchgr a day to mention whether he wants to take more of a look. If still no response, any issues can be cleaned up later. (I say this also because I spoke with him in person before my review and he sounded favorable toward the approaches that I knew nothing about.)

Of the two issues raised, I said I'd handle one later and the other seems resolved.

ejona86 · 2017-06-22T20:05:56Z

All comments appear addressed. Merging.

simonhorlick · 2017-06-23T00:19:22Z

Great, thanks for the reviews!

buchgr · 2017-07-03T10:58:11Z

Sorry, I was on a two week vacation, with mostly no internet :(.

I am excited that this go merged! Thanks @simonhorlick !

ejona86 · 2017-07-05T20:55:14Z

CC @cgrushko. It's been a couple weeks, and it seems the Bazel build is working well.

buchgr previously requested changes May 5, 2017

View reviewed changes

carl-mastrangelo reviewed May 5, 2017

View reviewed changes

buchgr mentioned this pull request May 9, 2017

Within Bazel, java_proto_library hack prevents proto-to-proto dependencies bazelbuild/bazel#2974

Closed

simonhorlick force-pushed the master branch from d41b0b8 to 85f1e06 Compare May 10, 2017 08:49

simonhorlick added 7 commits June 13, 2017 14:17

build: Add Bazel java_grpc_library rule

2ab78d1

Bazel third party dependencies are specified in repositories.bzl which gives the consumer the ability to opt-out of any dependencies they use directly in their own project. Fixes grpc#2756

build: Split up BUILD files

bc12966

build: Split out internal and util from core

b3f2aeb

build: Use built in maven_jar instead of java_import_external

468817a

build: Avoid filename clashes on case-insensitive filesystems

98bee07

build: Use underscores for target names

6d1a12e

build: Update bazel dependencies

480f1d0

simonhorlick force-pushed the master branch from 9cf4fa6 to 480f1d0 Compare June 13, 2017 06:20

simonhorlick added 3 commits June 13, 2017 14:39

build: Fix checksum for netty-tcnative

5ea8323

Revert "build: Avoid filename clashes on case-insensitive filesystems"

0638863

This reverts commit 98bee07.

build: Rename all BUILD files to BUILD.bazel

71a89bf

buchgr reviewed Jun 13, 2017

View reviewed changes

ejona86 reviewed Jun 17, 2017

View reviewed changes

simonhorlick added 3 commits June 17, 2017 11:24

build: Move visibility attributes to rules

c9da63b

build: bump netty version

2dfcd1a

build: Add missing inprocess library

b3f437c

ejona86 reviewed Jun 19, 2017

View reviewed changes

simonhorlick added 3 commits June 20, 2017 10:10

build: Remove exports of jsr305 and guava

e5a16b0

build: Restrict visibility of //core:internal to this repository

8b71e05

build: Remove grpc_java meta-target

2bf6e15

carl-mastrangelo reviewed Jun 20, 2017

View reviewed changes

simonhorlick added 3 commits June 20, 2017 12:10

build: Fix build

83e7a66

Bazel requires an empty BUILD file here so users can reference //:repositories.bzl and //:java_grpc_library.bzl.

build: Simplify glob patterns

97a5d20

build: Sort arguments in repositories.bzl

f0901ed

ejona86 approved these changes Jun 20, 2017

View reviewed changes

ejona86 merged commit 6d9e149 into grpc:master Jun 22, 2017

This was referenced Jun 22, 2017

Add CI testing for Bazel #3125

Closed

Build with Bazel #2458

Closed

Build grpc-java from a new arch---mips64el #2326

Closed

> Invalid NativePlatform: linux_aarch64 #2202

Closed

jyane mentioned this pull request Jan 5, 2018

Add dependencies checking between bazel and gradle? #3918

Open

lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019

		@@ -0,0 +1,126 @@
		package(default_visibility = ["//visibility:public"])

		cc_binary(

		@@ -0,0 +1,11 @@
		package(default_visibility = ["//visibility:public"])

build: Add Bazel java_grpc_library rule #2975

build: Add Bazel java_grpc_library rule #2975

Conversation

simonhorlick commented May 4, 2017

grpc-kokoro commented May 4, 2017

buchgr commented May 5, 2017

buchgr left a comment

Choose a reason for hiding this comment

ejona86 commented May 5, 2017

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

ejona86 commented May 10, 2017

simonhorlick commented May 11, 2017

simonhorlick commented Jun 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejona86 commented Jun 16, 2017

ejona86 commented Jun 16, 2017

ejona86 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

ejona86 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejona86 left a comment

Choose a reason for hiding this comment

ejona86 commented Jun 22, 2017

simonhorlick commented Jun 23, 2017

buchgr commented Jul 3, 2017

ejona86 commented Jul 5, 2017