Fix and disallow Java classpath collisions in Bazel

[fmeum]

This relies on the recently added oneversion support in rules_java.

[fmeum]

I had to disable oneversion checks for tests as there are far too many violations.

[meteorcloudy]

@hvadehra is definitely a better reviewer for this one ;)

[fmeum]

There is one remaining failure in JavaBuilder caused by duplication between io_github_eisop_dataflow_errorprone and org_checkerframework_checker_qual. The former bundles parts of the latter. I tried updating them to the same version, but they still differ (e.g. constant pool order is different).

Do you see a way to get rid of this duplication?

[hvadehra]

Hmm, if io_github_eisop_dataflow_errorprone is a fat jar, that's going to always be problematic. The usual way to get around this would be add the JavaBuilder target to the one version allowlist specified on the java_toolchain. But that's could end up being a fair bit of work, since AFAICT Bazel is using the default toolchains from rules_java.

fwiw, this ugly hack gets JavaBuilder to build:

$ git diff
diff --git a/third_party/BUILD b/third_party/BUILD
index fba8cd0655..ec53dd497d 100644
--- a/third_party/BUILD
+++ b/third_party/BUILD
@@ -351,12 +351,19 @@ distrib_jar_filegroup(
     enable_distributions = ["debian"],
 )
 
+java_import(
+    name = "error_prone-jars",
+    jars = [
+        "@maven//:com_google_errorprone_error_prone_core_file",
+        "@maven//:com_google_errorprone_error_prone_check_api_file",
+    ],
+)
+
 java_library(
     name = "error_prone",
     exports = [
+        ":error_prone-jars",
         ":error_prone_annotations",
-        "@maven//:com_google_errorprone_error_prone_check_api",
-        "@maven//:com_google_errorprone_error_prone_core",
     ],
 )

But after this, $ bazel build //src/java_tools/buildjar/... runs into:

ERROR: 
..../src/java_tools/buildjar/java/com/google/devtools/build/java/turbine/BUILD:23:12: Checking for one-version violations in //src/java_tools/buildjar/java/com/google/devtools/build/java/turbine:turbine_direct_binary failed: (Exit 1): one_version_main failed: error executing JavaOneVersion command (from target //src/java_tools/buildjar/java/com/google/devtools/build/java/turbine:turbine_direct_binary) external/rules_java++toolchains+remote_java_tools_linux/java_tools/src/tools/one_version/one_version_main ... (remaining 1 argument skipped)

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
Found one definition violations on the runtime classpath:
  com/google/common/escape/package-info has incompatible definitions in:
    crc32=219812193
      @@rules_jvm_external++maven+maven//:com_google_turbine_turbine [new]
      via bazel-out/k8-fastbuild/bin/external/rules_jvm_external++maven+maven/com/google/turbine/turbine/0.7.0/processed_turbine-0.7.0.jar
    crc32=291638515
      @@rules_jvm_external++maven+maven//:com_google_guava_guava [new]
      via bazel-out/k8-fastbuild/bin/external/rules_jvm_external++maven+maven/com/google/guava/guava/33.0.0-jre/processed_guava-33.0.0-jre.jar

[hvadehra]

cc @cushon

[cushon]

I'll look in the remaining violations. I think we can fix them, but it'll take time to get the fixes released.

Note that the internal version of this has a finer grained allowlist mechanism to deal with this kind of thing, to allow suppressing specific violations to make it easier to roll out in code bases with existing violations: #1071 (comment). We should probably spin off a FR for that if it doesn't happen as part of #1071, something like that may be useful for other Bazel users trying to roll this out.

The turbine violation is because it deliberately forks some code from Guava, and then declares a package-info, and that package info clashes: https://github.com/google/turbine/blob/e1ea3c5130b465ea8079d03e7cc68ebb44af32ae/java/com/google/common/escape/SourceCodeEscapers.java#L17. I can look at using a different package for turbine's copy of that package.

io_github_eisop_dataflow_errorprone repackages some classes in under a new namespace for Error Prone (ironically to avoid one version issues on the annotation processor path with other processors that depend on that dataflow implementation). It's a processed jar, but it isn't supposed to be an uber-jar, including the copies of those annotations may be an oversight.

I'm not sure #23592 (comment) is the right fix, does the JavaBuilder actually work at runtime with that change? Is it just excluding transitive deps of the Error Prone jars?

[hvadehra]

I'm not sure #23592 (comment) is the right fix, does the JavaBuilder actually work at runtime with that change?

I didn't try that nor run any tests. To be clear, I was not suggesting we proceed that way, just that there are potentially more violations to worry about than those caught by CI :)

Is it just excluding transitive deps of the Error Prone jars?

That was the idea, yes.

[cushon]

I have a fix in progress for turbine, and filed typetools/checker-framework#6795 for the dataflow issue.

I also wrote up the FR for the allowlist as #23598, I think that's probably a good next step instead of trying to chase down all of the one version issues in Bazel and waiting for them to be fixed upstream.