Decouple Error Prone version from Bazel version

[dfabulich]

Bazel 0.4.5 upgraded Error Prone, and in so doing introduced new build failures that weren't there in Bazel 0.4.4. We weren't able to upgrade to Bazel 0.4.5 until we addressed all of the new Error Prone failures. The same thing happened with Bazel 0.4.4.

Those Error Prone issues are valuable, but I wish we could pin our version of Error Prone to an older version while upgrading to Bazel 0.4.5, so we can get the benefits of the new version of Bazel without fixing all of the Error Prone issues.

[davido]

-1.

Current design is not only valuable but also the way I would expect it to work. I don't want to care what EP version a specific Bazel version is using. What new check to enable in new Bazel/EP version. Also note, EP has some dependencies that must be shipped in bazel for it to work.

I expect to upgrade Bazel and implicitly upgrade EP (and all other dependent Bazel library for the matter). I'm really happy, that we get error/warning reports for granted during Bazel upgrade!

[hermione521]

@cushon what do you say?

[cushon]

@damienmg has been creating a procedure for rolling out breaking changes that we'll follow for Error Prone once it's ready. The tentative plan is to leave it enabled by default and keep enabling new errors if they meet the critera: http://errorprone.info/docs/criteria.

Also, the version of Error Prone in the next Bazel release will support a flag to disable all checks (XepDisableAllChecks), so if you want to avoid being broken by new checks and selectively enable the ones you want, you'll be to use the flag for that.

(Finally - I'm glad the issues you saw were valuable, but if you see checks that are not valuable please let us know: https://github.com/google/error-prone/issues, https://groups.google.com/forum/#!forum/error-prone-discuss.)

[dfabulich]

I see it this way. Every time Error Prone is upgraded, it's a breaking change. It's usually a desirable breaking change, but it's a break, nonetheless. This certainly can't keep going on once Bazel hits 1.0. Upgrading to Bazel 1.1 shouldn't cause your build to fail if it works in Bazel 1.0.

[cushon]

That's the motivation for the policy @damienmg is working on for backwards incompatible changes.

[damienmg]

/cc @brandjon @laurentlb

We have an almost finished policy about how we are going to handle backwards incompatible changes and we will publish it soonish. Error-prone checks are definitely going to be included in it.

[meisterT]

We now have backwards incompatible change policy.

[brandjon]

What's the resolution w.r.t. ErrorProne? Is it decoupled from the Bazel version, or do we no longer update its version outside of a major Bazel release? Note that even with a major Bazel release, we theoretically should have an --incompatible_* flag to enable the new ErrorProne checks in the preceding Bazel release.

[meisterT]

The general idea is to have all incompatible changes guarded behind flags. ErrorProne is not special in this.

[brandjon]

What I mean is the issue title is asking us to decouple ErrorProne, and the closing comment is that we have a policy. So which is it -- did we actually decouple it, or did we resolve not to bump the version again until it's decoupled/controlled? And if it's the latter, should this issue stay open (at perhaps lower priority) to track decoupling?

[meisterT]

I don't think we want to decouple. The main complaint (if I reread the comments correctly) is about breaking changes on every update. That shouldn't happen anymore.

[brandjon]

Maybe I'm making a mountain out of a molehill -- If we want to upgrade the ErrorProne version in the future while following backwards compatibility policy, is it technically easy to add an --incompatible_* flag at that time? If so, we can forget this bug because there's no work needed at this time. If not, that suggests this bug should stay open to track an ongoing blocker to updating ErrorProne.

[meisterT]

That's a valid point about the technical simplicity, let me reopen this.

[davido]

Say EP is not decoupled from Bazel and there is no --incompatible-use-error-prone-version-2.3.4 added. Say EP 2.3.4 introduced breaking changes, e.g.: google/error-prone@11e28aa, google/error-prone@5ad37b7 that will break every Bazel user, who is relying on code generation libraries like auto-value or dagger and is using Java language source level 8 and does not have javax.annotation on the classpath.

Would it be still OK, to bump EP version in major Bazel releases, like upcoming Bazel 4.0.0 release. That would still break all Bazel users but we wouldn't care, because it's a major Bazel release?

[cushon]

EP is decoupled from Bazel in the sense that it's part of the java_tools archive. Is the idea that we seed that as an implementation detail, and since a Bazel release ships with a reference to a particular default java_tools version, it's effectively still coupled? What would applying the incompatible change policy look like here, should there be --incompatible_use_version_X_of_toolchain_dependency_Y flags?

[davido]

What would applying the incompatible change policy look like here, should there be --incompatible_use_version_X_of_toolchain_dependency_Y flags?

Just --incompatible_use_java_tools_version_X should be enough.

java_tools has many tools, so that I wouldn't break the incompatible options to 17 different permutations used there (javac, EP, singlejar, ...).

Consider my beta release for Linux and Darwin to switch to using java_tools 11 (that includes Error Prone version 2.3.4): [1].

To activate new java_tools, I still have to patch WORKSPACE file:

diff --git a/WORKSPACE b/WORKSPACE
index 38cc59b..6e0f092 100644
--- a/WORKSPACE
+++ b/WORKSPACE
@@ -1,4 +1,13 @@
 load("@bazel_tools//tools/build_defs/repo:maven_rules.bzl", "maven_jar")
+load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive", "http_file")
+
+http_archive(
+    name = "remote_java_tools_linux",
+    sha256 = "74d30ccf161c58bb69db9b2171c954a0563b2d1ff6f5831debbe71ced105c388",
+    urls = [
+        "https://github.com/davido/java_tools/releases/download/javac11-v11.0/java_tools_javac11_linux-v11.0.zip",
+    ],
+)

And invoke Bazel like this:

  $ bazel build \
   --java_toolchain=@remote_java_tools_linux//:toolchain \
   --host_java_toolchain=@remote_java_tools_linux//:toolchain \
    //...

If there wouldn't be any breaking changes in java_tools_11, and given that java_tools_11 is officially published java_tools version, a generic vanilla Bazel option --use_java_tools_version=X could help. Bazel CI could be extended and help to detect breakages with new java_tools release: --use_java_tools_version=11 in this case.

Of course, a dedicated --incompatible_use_java_tools_version_11 could be added as well, but I am not sure how well that would scale, given that last java_tools release was conducted in February 2020: [2].

[1] https://github.com/davido/java_tools/releases/tag/javac11-v11.0
[2] https://github.com/bazelbuild/java_tools/releases/tag/javac11_v8.0

[brandjon]

Would it be still OK, to bump EP version in major Bazel releases, like upcoming Bazel 4.0.0 release. That would still break all Bazel users but we wouldn't care, because it's a major Bazel release?

There must be a way for users to migrate their code easily. Our normal policy is that there must be at least one Bazel release that has a flag users can toggle to control whether or not the new behavior is enabled. That's how it works when a Starlark API is deleted, for instance, and @meisterT says we won't treat EP any different.

The existence of a workaround, like adding something to WORKSPACE, is not enough by itself because an unsuspecting user still ends up broken. With --incompatible_* flags, automated tooling can determine exactly what flags will break a build in an upcoming release.

That said, we may not adhere to this policy for very minor changes that do not break people in practice. Though I don't think requiring a large fraction of users to update their toolchain definitions qualifies as minor.

[davido]

Thanks @brandjon for clarification.

I started the attempt to demote two offending checks in recent EP releases to severity level warning, that is disabled in Bazel: [1]. Let's wait and see if and when this PR: [2] is accepted and how long would it take to show up as new EP release. I can see your point, that adding --incompatible_use_java_tools_release_X would be useful anyway and can try to look into adding it.

[1] google/error-prone#1619
[2] #11426

[hisener]

We have another "use case" for this issue. Say you want to upgrade JDK to a version that Bazel's current Error Prone version doesn't support. In that case, the Bazel upgrade will block the JDK upgrade since it's the only way to upgrade Error Prone.

In our case, we were using Bazel 4.2.1, and we couldn't upgrade to Java 17 because Bazel 4.2.1 uses Error Prone 2.4.0, which doesn't support Java 17. So, we had to upgrade to Bazel 5 first. It would have been nice to have a way to upgrade Error Prone version separately so that we don't need to wait for the Bazel upgrade.

Let me know if there is a way to overcome this problem because I presume a similar situation will happen again.

[rdesgroppes]

Another use case is that, even with the recent Bazel 6.0.0, we still can't leverage Mockito's ability to mocking/spying without specifying class:

$ bazel build //...
ERROR: /xxx/BUILD:NN:NN: Building xxx/xxx.jar (N source files) failed: (Exit 1): java failed: error executing command (from target //xxx:xxx) external/remotejdk17_linux/bin/java '--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED' '--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED' ... (remaining 16 arguments skipped)
xxx/Xxx.java:NN: error: An unhandled exception was thrown by the Error Prone static analysis plugin.
        final Xxx<Yyy> xxx = mock();
                                 ^
     Please report this at https://github.com/google/error-prone/issues/new and include the following:
  
     error-prone version: 2.11.0
     BugPattern: DoNotMock
     Stack Trace:
     java.lang.IndexOutOfBoundsException: Index: 0, Size: 0
  	at jdk.compiler/com.sun.tools.javac.util.List.get(List.java:490)
  	at com.google.errorprone.bugpatterns.AbstractMockChecker.lambda$extractFirstArg$2(AbstractMockChecker.java:172)
[...]
  	at com.google.devtools.build.lib.worker.WorkRequestHandler.lambda$startResponseThread$1(WorkRequestHandler.java:384)
  	at java.base/java.lang.Thread.run(Thread.java:833)

This was fixed with Error Prone 2.17.0: how may we switch to it independently from Bazel's release cycle?

[cushon]

Overall: I am supportive of having it be less coupled to a particular Bazel version, it's mostly a question of engineering and priorities.

FWIW we generally try to avoid enabling aggressive new diagnostics as errors by default, but sometimes things get enabled that probably shouldn't be. It's also possible to disable specific diagnostics for a repository by passing -Xep:CheckName:OFF flags, if a new version introduces a diagnostic that's not useful for a particular codebase it can be disabled.

A few notes since this issue was filed:

• Error Prone is more decoupled from Bazel than when this issue was first filed, it's possible (but not super convenient) to rebuild java_tools with a newer version of Error Prone and override the versions that Bazel includes by default: Decouple Error Prone version from Bazel version #2713

• Error Prone is using the javac -Xplugin: API in a more standard way than it used to, so one option might be to use that for the decoupling, and making it easier to drop in a new version of the EP plugin without having to rebuild all of java_tools

• I think the approach to incompatible changes has also evolved a bit since some of the earlier discussion, but I'm not sure what the latest official policy on that is, but I think we backed away from requiring --incompatible_ flags for everything?

[rdesgroppes]

Good news #17470 (Update error prone to 2.18.0 in the WORKSPACE.) got merged and is available since bazel 6.2.0, which voids my above comment.

[davido]

Error Prone is more decoupled from Bazel than when this issue was first filed, it's possible (but not super convenient) to rebuild java_tools with a newer version of Error Prone and override the versions that Bazel includes by default

Note, that this is only a theoretical option to upgrade Bazel, and thus benefit from the bump of Error Prone version, but downgrade the java_tools, because to support the latest compiler toolchains and recent operating systems and cpu architectures, you actually need the latest and greatest java_tools release, e. g.: [1] and friends.

[1] #17956

[github-actions]

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 90 days unless any other activity occurs. If you think this issue is still relevant and should stay open, please post any comment here and the issue will no longer be marked as stale.

[hisener]

I think it's still relevant.