Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add incompatible_use_java_tools_beta_release option #11447

Conversation

davido
Copy link
Contributor

@davido davido commented May 19, 2020

Fixes #11446.

Java tools are decoupled from Bazel distribution and downloaded from
java_tools repository.

If some of the tools is updated, the archive name is changed in Bazel
and new Bazel release is conducted. If, however, new tool has some
breaking changes, then the upgrade needs to be rolled back.

To make user experience with upgrading tools less disruptive but still
not unnecessary procrastinate releases of important Java tools like
javac and Error Prone --incompatible_use_java_tools_beta_release is
added and CI-job should be set up to check all downstream projects
against upcoming java_tools release.

Test Plan:

  1. Upgrade Error Prone version to 2.3.4 Bazel that is known to have
    breaking changes and will break all Bazel users who rely on cross
    compilation use case. There are ongoing efforts to demote the offensive
    EP checks to warning severity instead of error, though: [1].

  2. Bump java_tools in beta_release archive to new java_tools
    distribution that includes Error Prone 2.3.4

  3. Build rules_closure with --incompatible_use_java_tools_beta_release
    would fail with the known issue, because of unsatisfied dependency
    on javax.annotation:

ERROR: /home/davido/projects/rules_closoure/java/io/bazel/rules/closure/BUILD:41:13: Building java/io/bazel/rules/closure/libtarjan.jar (1 source file) and running annotation processors (AutoAnnotationProcessor, AutoOneOfProcessor, AutoValueProcessor) failed (Exit 1)
bazel-out/host/bin/java/io/bazel/rules/closure/_javac/tarjan/libtarjan_sourcegenfiles/io/bazel/rules/closure/AutoValue_Tarjan_Result.java:6: error: [ExtendsAutoValue] Do not extend an @AutoValue/@AutoOneOf class in non-generated code.
final class AutoValue_Tarjan_Result extends Tarjan.Result {
^
(see https://errorprone.info/bugpattern/ExtendsAutoValue)

See also upstream issue with in depth explanation what is going on
there: [2]

  1. Build rules_closure without the incompatible option: all is fine.
    That would mean, that all broken downstream projects would have
    enough time to adapt their build tool chains for upgraded
    dependencies in java_tools distribution.

[1] google/error-prone#1619
[2] bazelbuild/rules_closure#483

Change-Id: I352297a8d0d86cecf30821c132c7871383e49b50

@davido davido requested a review from lberki as a code owner May 19, 2020 22:41
@davido
Copy link
Contributor Author

davido commented May 20, 2020

I am seeing some failures on the Bazel CI that seems to be related to the connectivity to the GitHub. I filed this issue.

@aiuto aiuto requested a review from philwo May 21, 2020 03:25
@aiuto aiuto added the area-EngProd Bazel CI, infrastructure, bootstrapping, release, and distribution tooling label May 21, 2020
@davido davido force-pushed the incompatible_use_java_tools_beta_release branch from aab45e0 to a5de695 Compare May 21, 2020 07:46
Fixes bazelbuild#11446.

Java tools are decoupled from Bazel distribution and downloaded from
java_tools repository.

If some of the tools is updated, the archive name is changed in Bazel
and new Bazel release is conducted. If, however, new tool has some
breaking changes, then the upgrade needs to be rolled back.

To make user experience with upgrading tools less disruptive but still
not unnecessary procrastinate releases of important Java tools like
javac and Error Prone --incompatible_use_java_tools_beta_release is
added and CI-job should be set up to check all downstream projects
against upcoming java_tools release.

Test Plan:

1. Upgrade Error Prone version to 2.3.4 Bazel that is known to have
breaking changes and will break all Bazel users who rely on cross
compilation use case. There are ongoing efforts to demote the offensive
EP checks to warning severity instead of error, though: [1].

2. Bump java_tools in beta_release archive to new java_tools
   distribution that includes Error Prone 2.3.4

3. Build rules_closure with --incompatible_use_java_tools_beta_release
   would fail with the known issue, because of unsatisfied dependency
   on javax.annotation:

ERROR: /home/davido/projects/rules_closoure/java/io/bazel/rules/closure/BUILD:41:13: Building java/io/bazel/rules/closure/libtarjan.jar (1 source file) and running annotation processors (AutoAnnotationProcessor, AutoOneOfProcessor, AutoValueProcessor) failed (Exit 1)
bazel-out/host/bin/java/io/bazel/rules/closure/_javac/tarjan/libtarjan_sourcegenfiles/io/bazel/rules/closure/AutoValue_Tarjan_Result.java:6: error: [ExtendsAutoValue] Do not extend an @AutoValue/@AutoOneOf class in non-generated code.
final class AutoValue_Tarjan_Result<V> extends Tarjan.Result<V> {
      ^
    (see https://errorprone.info/bugpattern/ExtendsAutoValue)

See also upstream issue with in depth explanation what is going on
there: [2]

4. Build rules_closure without the incompatible option: all is fine.
   That would mean, that all broken downstream projects would have
   enough time to adapt their build tool chains for upgraded
   dependencies in java_tools distribution.

[1] google/error-prone#1619
[2] bazelbuild/rules_closure#483

Change-Id: I352297a8d0d86cecf30821c132c7871383e49b50
@davido davido force-pushed the incompatible_use_java_tools_beta_release branch from a5de695 to 8727648 Compare May 21, 2020 07:58
Copy link
Member

@philwo philwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's fine.

@lberki Any concerns from your side?

@davido
Copy link
Contributor Author

davido commented Jun 3, 2020

@philwo, Thanks for the review!

However, given that @cushon demoted 2 Error Prone checks from error severity to warning (that broke rules_kotlin during last attempt to bump Error Prone version), and this change is included in EP 2.4.0 release, we actually don't need this change as a prerequisite for EP upgrade PR. My expectation would be, that all Bazel downstream projects would just work fine once new java_tools release with new and shiny EP 2.4.0 is conducted.

@philwo
Copy link
Member

philwo commented Jun 3, 2020

@davido Great, thanks for the explanation. I'll look at the error prone / java_tools update next!

@davido
Copy link
Contributor Author

davido commented Jun 3, 2020

I'll look at the error prone / java_tools update next!

There are basically 4 steps:

  1. Merge EP upgrade PR
  2. Kick the Pipeline from Bazel@HEAD in java_tools: as the final artifacts we would get three new java_tools for Windows|Mac Os X|Linux OS
  3. Those three new artifacts would have to be published on Bazel Mirror
  4. Replace default java_tools in Bazel with new java_tools releases

I could help with 4. change and with testing of the new java_tools release.

@philwo philwo added the team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website label Jun 15, 2020
@davido davido mentioned this pull request Jul 1, 2020
10 tasks
@meisterT meisterT requested a review from comius October 7, 2020 19:41
Comment on lines +719 to +728
http_archive(
name = "remote_java_tools_linux_beta_for_testing",
patch_cmds = EXPORT_WORKSPACE_IN_BUILD_FILE,
patch_cmds_win = EXPORT_WORKSPACE_IN_BUILD_FILE_WIN,
sha256 = "74d30ccf161c58bb69db9b2171c954a0563b2d1ff6f5831debbe71ced105c388",
urls = [
"https://github.com/davido/java_tools/releases/download/javac11-v11.0/java_tools_javac11_linux-v11.0.zip",
],
)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this mean, that before every release, we have to change this archive? Because of the version, i.e. rc1 and because of the sha sum?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this mean, that before every release, we have to change this archive?

Yes, that's correct.

@comius
Copy link
Contributor

comius commented Oct 9, 2020

I'm not sure using flags and adding repos is needed here.

There is an easier way. According to release instructions https://github.com/bazelbuild/java_tools/blob/master/docs/release.md, release candidate is already prepared first as a pull request. It should be easy to also execute Downstream pipeline on that PR.

@davido
Copy link
Contributor Author

davido commented Oct 9, 2020

I'm not sure using flags and adding repos is needed here.
There is an easier way. According to release instructions https://github.com/bazelbuild/java_tools/blob/master/docs/release.md, release candidate is already prepared first as a pull request. It should be easy to also execute Downstream pipeline on that PR.

This PR is not about testing beta releases in Bazel pipeline, this is about providing a trivial way to downstream projects to adapt their code base to the upcoming java_tools release, because we cannot guarantee, that new java_tools will be always backwards compatible. In fact, it wouldn't be backwards compatible, but would break user code.

Right now, there is only way to upgrade content of java_tools, like Java compiler and error_prone version: Bump those versions, publish new java_tools and consume this new java_tools in next Bazel release. While this upgrade path certainly works, it could lead to breakages in downstream projects. Of course, we could publish beta java_tools release, and urgently ask folks in the wild, please, test it against your code base. However, it's is not easy doable, because they would have to consume java_tools_beta release in their WORKSPACE and provide obscure options to try new release and fix issues. So, I wouldn't expect that they would do it.

That's where this PR should help. Say in Bazel 4.0 we would include error_prone 3.0 that would add 42 new checks with non-trivial to fix breakages.

  $ bazel build //...

would still use old error_prone release 2.4, however:

  $ bazel build --incompatible_use_java_tools_beta_release //...

would already use beta java_tools with error_prone 3.0.

Then we would wait for 1 release, and rename the option --incompatible_use_java_tools_beta_release to --incompatible_use_java_tools_deprecated_release. In Bazel 4.1 the java_tools_beta would become the default java_tools with EP 3.0. With the new option --incompatible_use_java_tools_deprecated_release old java_tools release with EP 2.4 will be used. In Bazel 4.2 we would remove this option as well: --incompatible_use_java_tools_deprecated_release.

With the above migration path, folks would have two Bazel releases time (4.0 and 4.1) to adapt their code to the new java_tools release. I hope this clarifies what this PR is all about. (Also see real life example: #11769, how new java_tools broke user code and why this feature is needed and justified even though it is adding yet another obscure build option).

@comius
Copy link
Contributor

comius commented Oct 9, 2020

So additional testing during release would detect problems on rules_closure, right?

I agree it is an important use-case, that with bazel upgrade we don't want to also automatically upgrade java_tools, because this potentially breaks user's builds.

The flag, seems to handle only one release of java_tools? Or would we keep the flag and leap-frog java_tools on the releases that might break users?

Currently it is hard to determine what will break users builds. It is even hard to determine what changed in java_tools as changes are mixed with bazel changes. And there might be releases of java_tools that are incompatible with previous bazel version.

Not just error_prone. We might want to update default java release version in the future. And users probably want to have more control over this.

If this is a single time flag just for one change, I don't see how it solves future problems.

Assuming this is a leap-frog kind of a flag, i.e the flag always sets you one version back of java_tools. The problem I see is deciding which releases to select for the flag, i.e. what changes are the most potential breakers. I see another problem whenever we have a java_tools release that break blaze compatibility - in such cases previous version wouldn't work anyway.

Other options besides the a single flag I can think of:

  • having a flag that specifies which java_tools version to use (but this is hard to implement; need a list of shasums, a lot of logic how to generate jdk.WORKSPACE file)
  • Would it be helpful to provide instructions, how to add http_archives to WORKSPACE and flags for .bazelrc to use an older release?

Did I understand you correctly? What do you think about alternative options or do you think there another alternative?

@davido
Copy link
Contributor Author

davido commented Oct 9, 2020

rules_closure was just one example. Gerrit Code Review removed dependency on rules_closure (yay) so we don't really care.

Don't get me wrong: presonally I don't need this PR to be merged, and I'm going to close it. The reason I uploded it, because I wanted to bump Error Prone to 2.4.0, because this version added support to recent Java compilers. Now, that EP was bumped to 2.4.0 and @meisterT refused to revert the EP upgrade despite user complaints (#11769) we don't need it anymore.

And yes you got me right: the idea was to only add one single option, and remove it again, and only support simultaneously 2 java_tools versions: current, and beta, or, after the flip, new and the old one. If user would like older java_tools per default, they would have to use older Bazel releases.

And yes, there are multiple alternatives for adding yet another --incompatible-foo build options and making Bazel code even more complex: Better documentation how to upgrade and test beta java_tools releases, write blog, and ask for help on mailing list.

Say, we would like to bump Java language level from currently 8 to 11 in Bazel 4.0/5.0 per default.

While it is long overdue change, it would break 95% Bazel users. Of course there would be another options how to downgrade java toolchain to language level 8 again, but as you know, the folks would tell you, that all devs would have to add new options to their build tool chain, to their IDE and CI-pipelines.

That's why it would be great if changes with such huge impact on Bazel users could be shipped in a less disruptive way. And even let users (at least) 2 major releases time to adapt their code base.

@davido davido closed this Oct 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-EngProd Bazel CI, infrastructure, bootstrapping, release, and distribution tooling cla: yes team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website
Projects
None yet
5 participants