Restricting package_group in java_package_configuration doesn't have the desired effect #6010

davido · 2018-08-28T06:31:21Z

On most recent Bazel@HEAD (2894b30) trying to restrict custom toolchain declaration on specific packages, doesn't actually work. Even though we provide:

package_group(
    name = "error_prone_packages",
    packages = [
        "//java/...",
        "//javatests/...",
    ],
)

Error prone warnings are still reported from antlr3/com/google/gerrit/index/query/QueryLexer.java as well. These are generated sources and we don't want to see anything reported for them. Is there any other means to silence EP reports from generated sources?

$ bazel build --java_toolchain //tools:error_prone_warnings_toolchain //java/com/google/gerrit/server:server
[...]
INFO: From Building java/com/google/gerrit/index/libquery_parser.jar (1 source jar):
/antlr3/com/google/gerrit/index/query/QueryParser.java:6: warning: [WildcardImport] Wildcard imports, static or otherwise, should not be used
import org.antlr.runtime.*;
^
    (see https://google.github.io/styleguide/javaguide.html#s3.3.1-wildcard-imports)
/antlr3/com/google/gerrit/index/query/QueryLexer.java:6: warning: [WildcardImport] Wildcard imports, static or otherwise, should not be used
import org.antlr.runtime.*;
^
    (see https://google.github.io/styleguide/javaguide.html#s3.3.1-wildcard-imports)
/antlr3/com/google/gerrit/index/query/QueryLexer.java:620: warning: [ClassCanBeStatic] Inner class is non-static but does not reference enclosing class
	protected class DFA6 extends DFA {
	          ^
    (see https://errorprone.info/bugpattern/ClassCanBeStatic)
  Did you mean 'protected static class DFA6 extends DFA {'?
Target //java/com/google/gerrit/server:server up-to-date:
  bazel-bin/java/com/google/gerrit/server/libserver.jar
INFO: Elapsed time: 9.888s, Critical Path: 2.58s
INFO: 128 processes: 126 remote cache hit, 1 linux-sandbox, 1 worker.
INFO: Build completed successfully, 129 total actions

Reproducer: https://gerrit-review.googlesource.com/c/gerrit/+/194045

//CC @cushon

The text was updated successfully, but these errors were encountered:

cushon · 2018-08-28T14:18:51Z

package_group.packages refers to Bazel packages (e.g. //java/com/google/gerrit/index) not Java packages. It looks like antlr3/com/google/gerrit/index/query/QueryLexer.java is part of a Bazel package under //java/....

davido · 2018-08-28T15:03:10Z

So, the relevant rule in java/com/google/gerrit/index/BUILD is: https://github.com/GerritCodeReview/gerrit/blob/master/java/com/google/gerrit/index/BUILD#L14-L25:

java_library(
    name = "query_parser",
    srcs = ["//antlr3:query"],
    visibility = [
        "//javatests/com/google/gerrit/index:__pkg__",
        "//plugins:__pkg__",
    ],
    deps = [
        ":query_exception",
        "//lib/antlr:java-runtime",
    ],
)

And //antlr3:query is: https://github.com/GerritCodeReview/gerrit/blob/master/antlr3/BUILD#L3-L17:

genrule2(
    name = "query",
    srcs = ["com/google/gerrit/index/query/Query.g"],
    outs = ["query_antlr.srcjar"],
    cmd = " && ".join([
        "$(location //lib/antlr:antlr-tool) -o $$TMP $<",
        "cd $$TMP",
        "find . -exec touch -t 198001010000 '{}' ';'",
        "zip -q $$ROOT/$@ $$(find . -type f)",
    ]),
    tools = [
        "//lib/antlr:antlr-tool",
    ],
    visibility = ["//visibility:public"],
)

So, I think you are right, the fact that grammar file was built under different Bazel package (//antlr3) doesn't affect the compilation rule java/com/google/gerrit/index:query_parser that compiles generated sources.

So I guess we cannot do anything here.

davido · 2018-08-31T04:18:04Z

OK, I figured out how to fix that: java_library rule should be moved to //antlr3 Bazel package, where the grammar sources are located and Java sources are generated.

In I75d6bc4d7a antlr3 grammar sources were moved from gerrit-antlr package, but the java_library rule query_parser were missed to also be moved to antlr3 package. This is very unusual in Bazel toolchain to keep the sources in one package and build them in another. This leads to a problem: restricting package_group in java_package_configuration doesn't lead to the desired effect, as discussed in this issue: [1]. Moving the query_parser rule to the antlr3 package harmonize the location of the java_library rule to the same package where the sources are located and it also fixes the problem, that error prone warnings are not reported any more for the antlr3 grammar source in new toolchain, that doesn't list antlr3 in the package_group. ntlr3 grammar code is generated code and we would rather avoid reporting any issues in that code. [1] bazelbuild/bazel#6010 Change-Id: Iff32b6133ff7bf3f045a20fcb635d690d76e3b89

davido · 2018-09-04T20:03:14Z

@cushon Has something changed on Bazel@HEAD (8f0d73a)?

Trying to replicate the EP warnings, with my custom toolchain from https://gerrit-review.googlesource.com/c/gerrit/+/194045 that was merged in gerrit, i'm first getting this error:

$ b9 build --java_toolchain //tools:error_prone_warnings_toolchain //java/...
DEBUG: /home/davido/.cache/bazel/_bazel_davido/5c01f4f713b675540b8b424c5c647f63/external/bazel_skylib/lib/versions.bzl:98:7: 
Current Bazel is not a release version, cannot check for compatibility.
DEBUG: /home/davido/.cache/bazel/_bazel_davido/5c01f4f713b675540b8b424c5c647f63/external/bazel_skylib/lib/versions.bzl:99:7: Make sure that you are running at least Bazel 0.14.0.
ERROR: /home/davido/projects/gerrit2/tools/BUILD:45:1: no such target '@bazel_tools//tools/jdk:platformclasspath.jar': target 'platformclasspath.jar' not declared in package 'tools/jdk' (did you mean 'platformclasspath8.jar'?) defined by /home/davido/.cache/bazel/_bazel_davido/5c01f4f713b675540b8b424c5c647f63/external/bazel_tools/tools/jdk/BUILD and referenced by '//tools:error_prone_warnings_toolchain'
ERROR: Analysis of target '//java/com/google/gerrit/server:doc' failed; build aborted: Analysis failed
INFO: Elapsed time: 0.458s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (1 packages loaded)

And after fixing it as suggested:

diff --git a/tools/BUILD b/tools/BUILD
index 53f441a37b..cd5e77d43f 100644
--- a/tools/BUILD
+++ b/tools/BUILD
@@ -44,7 +44,7 @@ default_java_toolchain(
 
 default_java_toolchain(
     name = "error_prone_warnings_toolchain",
-    bootclasspath = ["@bazel_tools//tools/jdk:platformclasspath.jar"],
+    bootclasspath = ["@bazel_tools//tools/jdk:platformclasspath8.jar"],
     package_configuration = [
         ":error_prone",
     ],

And running build with custom toolchain, I only see a couple of EP warning, with all warnings activated, while previously I've seen dozen or even hundreds of warnings.

davido · 2018-09-04T20:12:28Z

I can confirm, that when building gerrit master with Bazel 0.16.1 release, I'm getting dozen of EP warnings: [1]:

  $ bazel build --java_toolchain //tools:error_prone_warnings_toolchain_bazel_0.16  //java/...

And when building from Bazel@HEAD: I only getting very few EP warnings on exactly the same gerrit code (with the amended bootclasspath to @bazel_tools//tools/jdk:platformclasspath8.jar as mentioned above):

  $ bazel build --java_toolchain //tools:error_prone_warnings_toolchain  //java/...

Any clue what is going on here?

[1] http://paste.openstack.org/show/729447

davido · 2018-09-04T20:38:31Z

I think it's related to revert of JDK10 javac in 12dcd35. If I revert this change on bazel@HEAD, I see all warnings as in 0.16.1.

cushon · 2018-09-05T03:30:54Z

I'll respond in #6077, thanks for raising this issue.

davido · 2018-09-18T18:36:40Z

@cushon Somehow our custom error prone --java_toolchain option stopped working.

Even with Bazel 0.17.1 with this diff on gerrit master:

diff --git a/tools/BUILD b/tools/BUILD
index 73ecfb92b5..19b950758d 100644
--- a/tools/BUILD
+++ b/tools/BUILD
@@ -44,7 +44,7 @@ default_java_toolchain(
 
 default_java_toolchain(
     name = "error_prone_warnings_toolchain",
-    bootclasspath = ["@bazel_tools//tools/jdk:platformclasspath.jar"],
+    bootclasspath = ["@bazel_tools//tools/jdk:platformclasspath9.jar"],
     package_configuration = [
         ":error_prone",
     ],

I'm getting this:

$ bazel build --java_toolchain //tools:error_prone_warnings_toolchain //javatests/...
INFO: Analysed 170 targets (183 packages loaded).
INFO: Found 170 targets...
ERROR: /home/davido/projects/gerrit2/java/org/eclipse/jgit/BUILD:37:1: Building java/org/eclipse/jgit/libserver.jar (2 source files) failed: Worker process returned an unparseable WorkResponse!

Did you try to print something to stdout? Workers aren't allowed to do this, as it breaks the protocol between Bazel and the worker process.

---8<---8<--- Exception details ---8<---8<---
com.google.protobuf.InvalidProtocolBufferException$InvalidWireTypeException: Protocol message tag had invalid wire type.
	at com.google.protobuf.InvalidProtocolBufferException.invalidWireType(InvalidProtocolBufferException.java:115)
	at com.google.protobuf.UnknownFieldSet$Builder.mergeFieldFrom(UnknownFieldSet.java:551)
	at com.google.protobuf.GeneratedMessageV3.parseUnknownFieldProto3(GeneratedMessageV3.java:305)
	at com.google.devtools.build.lib.worker.WorkerProtocol$WorkResponse.<init>(WorkerProtocol.java:1936)
	at com.google.devtools.build.lib.worker.WorkerProtocol$WorkResponse.<init>(WorkerProtocol.java:1886)
	at com.google.devtools.build.lib.worker.WorkerProtocol$WorkResponse$1.parsePartialFrom(WorkerProtocol.java:2503)
	at com.google.devtools.build.lib.worker.WorkerProtocol$WorkResponse$1.parsePartialFrom(WorkerProtocol.java:2497)
	at com.google.protobuf.AbstractParser.parsePartialFrom(AbstractParser.java:221)
	at com.google.protobuf.AbstractParser.parsePartialDelimitedFrom(AbstractParser.java:262)
	at com.google.protobuf.AbstractParser.parseDelimitedFrom(AbstractParser.java:275)
	at com.google.protobuf.AbstractParser.parseDelimitedFrom(AbstractParser.java:280)
	at com.google.protobuf.AbstractParser.parseDelimitedFrom(AbstractParser.java:49)
	at com.google.protobuf.GeneratedMessageV3.parseDelimitedWithIOException(GeneratedMessageV3.java:347)
	at com.google.devtools.build.lib.worker.WorkerProtocol$WorkResponse.parseDelimitedFrom(WorkerProtocol.java:2145)
	at com.google.devtools.build.lib.worker.WorkerSpawnRunner.execInWorker(WorkerSpawnRunner.java:329)
	at com.google.devtools.build.lib.worker.WorkerSpawnRunner.actuallyExec(WorkerSpawnRunner.java:159)
	at com.google.devtools.build.lib.worker.WorkerSpawnRunner.exec(WorkerSpawnRunner.java:115)
	at com.google.devtools.build.lib.exec.AbstractSpawnStrategy.exec(AbstractSpawnStrategy.java:107)
	at com.google.devtools.build.lib.exec.AbstractSpawnStrategy.exec(AbstractSpawnStrategy.java:75)
	at com.google.devtools.build.lib.exec.SpawnActionContextMaps$ProxySpawnActionContext.exec(SpawnActionContextMaps.java:362)
	at com.google.devtools.build.lib.analysis.actions.SpawnAction.internalExecute(SpawnAction.java:288)
	at com.google.devtools.build.lib.analysis.actions.SpawnAction.execute(SpawnAction.java:295)
	at com.google.devtools.build.lib.skyframe.SkyframeActionExecutor.executeActionTask(SkyframeActionExecutor.java:971)
	at com.google.devtools.build.lib.skyframe.SkyframeActionExecutor.prepareScheduleExecuteAndCompleteAction(SkyframeActionExecutor.java:900)
	at com.google.devtools.build.lib.skyframe.SkyframeActionExecutor.access$800(SkyframeActionExecutor.java:118)
	at com.google.devtools.build.lib.skyframe.SkyframeActionExecutor$ActionRunner.call(SkyframeActionExecutor.java:744)
	at com.google.devtools.build.lib.skyframe.SkyframeActionExecutor$ActionRunner.call(SkyframeActionExecutor.java:699)
	at java.base/java.util.concurrent.FutureTask.run(Unknown Source)
	at com.google.devtools.build.lib.skyframe.SkyframeActionExecutor.executeAction(SkyframeActionExecutor.java:459)
	at com.google.devtools.build.lib.skyframe.ActionExecutionFunction.checkCacheAndExecuteIfNeeded(ActionExecutionFunction.java:505)
	at com.google.devtools.build.lib.skyframe.ActionExecutionFunction.compute(ActionExecutionFunction.java:215)
	at com.google.devtools.build.skyframe.AbstractParallelEvaluator$Evaluate.run(AbstractParallelEvaluator.java:420)
	at com.google.devtools.build.lib.concurrent.AbstractQueueVisitor$WrappedRunnable.run(AbstractQueueVisitor.java:368)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source)
	at java.base/java.lang.Thread.run(Unknown Source)
---8<---8<--- End of exception details ---8<---8<---

---8<---8<--- Start of log ---8<---8<---
-Xbootclasspath/p is no longer a supported option.
ion.
---8<---8<--- End of log ---8<---8<---
INFO: Elapsed time: 6.579s, Critical Path: 0.66s
INFO: 75 processes: 20 remote cache hit, 55 linux-sandbox.
FAILED: Build did NOT complete successfully

And when trying the same on Bazel@HEAD (fdfe0f1):

RROR: /home/davido/projects/gerrit2/java/com/google/gerrit/util/ssl/BUILD:1:1: Compiling Java headers java/com/google/gerrit/util/ssl/libssl-hjar.jar (2 files) failed (Exit 1) java failed: error executing command external/embedded_jdk/bin/java -Xverify:none -Xbootclasspath/p:external/bazel_tools/third_party/java/jdk/langtools/javac-9+181-r4173-1.jar -jar external/bazel_tools/tools/jdk/turbine_deploy.jar ... (remaining 78 argument(s) skipped)

Use --sandbox_debug to see verbose messages from the sandbox
Error: Could not create the Java Virtual Machine.
Error: A fatal exception has occurred. Program will exit.
-Xbootclasspath/p is no longer a supported option.
INFO: Elapsed time: 12.859s, Critical Path: 1.47s
INFO: 146 processes: 143 remote cache hit, 3 linux-sandbox.
FAILED: Build did NOT complete successfully

Any clue?

davido · 2018-09-18T18:50:53Z

I think I figured it out: jvm_opts were missing: jvm_opts = JDK9_JVM_OPTS,:

default_java_toolchain(
    name = "error_prone_warnings_toolchain",
    bootclasspath = ["@bazel_tools//tools/jdk:platformclasspath9.jar"],
    jvm_opts = JDK9_JVM_OPTS,
    package_configuration = [
        ":error_prone",
    ],
    visibility = ["//visibility:public"],
)

But why are we getting such a scare error message?

cushon · 2018-09-18T20:37:09Z

The JDK 8 JVM flags include -Xbootclasspath/p:, which isn't supported on JDK 9, so we have to use --patch-module instead. That error is reported before the worker starts up, and the error handling when workers fail to start is kind of verbose. (cc @philwo FYI)

davido · 2018-09-19T04:26:44Z

Thanks for clarifying. I think what is not really obvious is that jvm_opts defaults to JDK8_JVM_OPTS in default_java_toolchain(). Because if I don't pass any jvm_opts it fails.

cushon · 2018-09-19T04:33:46Z

I agree: 808ec9f#diff-a430a0f3acbf77bcbe5488892ad884c7R74

This will get cleaned up once 7eb9ea1 is rolled forward.

cushon added team-Rules-Java Issues for Java rules untriaged labels Aug 28, 2018

davido closed this as completed Aug 28, 2018

cushon reopened this Sep 4, 2018

cushon assigned cushon and lberki Sep 4, 2018

cushon closed this as completed Sep 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restricting package_group in java_package_configuration doesn't have the desired effect #6010

Restricting package_group in java_package_configuration doesn't have the desired effect #6010

davido commented Aug 28, 2018 •

edited

Loading

cushon commented Aug 28, 2018

davido commented Aug 28, 2018 •

edited

Loading

davido commented Aug 31, 2018 •

edited

Loading

davido commented Sep 4, 2018

davido commented Sep 4, 2018

davido commented Sep 4, 2018

cushon commented Sep 5, 2018

davido commented Sep 18, 2018 •

edited

Loading

davido commented Sep 18, 2018

cushon commented Sep 18, 2018

davido commented Sep 19, 2018 •

edited

Loading

cushon commented Sep 19, 2018

Restricting package_group in java_package_configuration doesn't have the desired effect #6010

Restricting package_group in java_package_configuration doesn't have the desired effect #6010

Comments

davido commented Aug 28, 2018 • edited Loading

cushon commented Aug 28, 2018

davido commented Aug 28, 2018 • edited Loading

davido commented Aug 31, 2018 • edited Loading

davido commented Sep 4, 2018

davido commented Sep 4, 2018

davido commented Sep 4, 2018

cushon commented Sep 5, 2018

davido commented Sep 18, 2018 • edited Loading

davido commented Sep 18, 2018

cushon commented Sep 18, 2018

davido commented Sep 19, 2018 • edited Loading

cushon commented Sep 19, 2018

davido commented Aug 28, 2018 •

edited

Loading

davido commented Aug 28, 2018 •

edited

Loading

davido commented Aug 31, 2018 •

edited

Loading

davido commented Sep 18, 2018 •

edited

Loading

davido commented Sep 19, 2018 •

edited

Loading