Generate error-prone patches #21640

rsalvador · 2024-03-12T08:28:26Z

The current error-prone integration doesn't generate patches: #5729 (comment)

This PR enables error-prone patch generation and relies on making some existing error-prone members public: google/error-prone#4318

Error-prone requires the caller to specify the location of the generated error-prone.patch files: https://errorprone.info/docs/patching, callers can handle this by for example dynamically generating a javacopts with a different -XepPatchLocation for each java target rule. Another option is to use the IN_PLACE option: -XepPatchLocation:IN_PLACE

rsalvador · 2024-03-18T17:38:20Z

Hi @sgowroji, I see you changed the status to awaiting-user-response, but I don't see any question directed to me.
Note that the build failures are because this PR needs changes in error-prone: google/error-prone#4318

msridhar

In an amazing coincidence, I just worked on this same thing a couple of days back and ended up with a very similar approach! This code is definitely cleaner than what I did. I just had a comment on one thing I did differently

msridhar · 2024-03-19T03:32:57Z

...ldjar/java/com/google/devtools/build/buildjar/javac/plugins/errorprone/ErrorPronePlugin.java

@@ -121,6 +127,9 @@ public void postFlow(Env<AttrContext> env) {
    elapsed.start();
    try {
      errorProneAnalyzer.finished(new TaskEvent(Kind.ANALYZE, env.toplevel, env.enclClass.sym));
+      if (refactoringTask != null) {
+        refactoringTask.finished(new TaskEvent(Kind.GENERATE, env.toplevel, env.enclClass.sym));


With this call here, the refactoring task won't actually run after the GENERATE phase is completed. Assuming that it is important to run the task at that time, an alternative is to register the task as a TaskListener with the compiler. To do this, around line 115 where the RefactoringTask is created, you can do:

MultiTaskListener.instance(context).add(refactoringTask);

Then you won't need this call. Not sure if it's important or not.

Thanks @msridhar, I implemented your suggestion.

sgowroji · 2024-03-19T04:16:24Z

Hi @rsalvador, Thanks for sharing details on above failures.

msridhar · 2024-03-19T16:52:15Z

@rsalvador have you explored how this change interacts with sandboxing for your use case? With weaker sandboxes, I found that the original source files do get modified with the IN_PLACE strategy, since symlinks are used. I haven't tested with a stronger sandbox, but I suspect the patching might just fail with IN_PLACE if the input files are viewed as read-only. In such cases patch files could be used, but then they might need to be set as an additional output of the action. Just wondering how this is working out for you all.

rsalvador · 2024-03-20T08:13:11Z

Hi @sgowroji, the error-prone PR (google/error-prone#4318) has been in a "1 workflow awaiting approval. This workflow requires approval from a maintainer" state since it was opened last week and it has not had any activity or checks run. Do you know how to get the error-prone PR moving forward?

rsalvador · 2024-03-20T08:18:04Z

@rsalvador have you explored how this change interacts with sandboxing for your use case? With weaker sandboxes, I found that the original source files do get modified with the IN_PLACE strategy, since symlinks are used. I haven't tested with a stronger sandbox, but I suspect the patching might just fail with IN_PLACE if the input files are viewed as read-only. In such cases patch files could be used, but then they might need to be set as an additional output of the action. Just wondering how this is working out for you all.

For local builds, we tried both IN_PLACE and dynamically generating a javacopts with a different -XepPatchLocation for each java target and both worked well, didn't try stronger sandboxing. We used it to perform a mass fixing of wildcard imports by running a full local build with patching enabled and plan to use it for similar use cases only for now.

sgowroji · 2024-03-20T15:38:36Z

Hi @sgowroji, the error-prone PR (google/error-prone#4318) has been in a "1 workflow awaiting approval. This workflow requires approval from a maintainer" state since it was opened last week and it has not had any activity or checks run. Do you know how to get the error-prone PR moving forward?

@rsalvador we don't Triage or manage above mentioned repository.

Summary: This diff contains the changes in bazelbuild#21640, ported to the uber/java/7.0.2.001 branch. We also commit a modified version of the Error Prone check_api jar. The modifications in that jar are those changes in google/error-prone#4318 applied to Error Prone 2.22.0 (the version used in our branch). Probably, rather than committing the modified jar, we could upload the modified jar to artifactory and change the version we depend on here. I have tested that these changes work locally on a devpod. `IN_PLACE` patching will most likely not work under stronger sandboxes, as it relies on modifying the source files in the sandbox (which, by default on a devpod, are just symlinks to the original source files). Also, there seem to be some weird interactions with the Bazel cache, such that when I tried to run the same autopatch twice (after undoing the initial changes), I had to blow away my local `~/.java_bazelcache` directory to make it work. Test Plan: Tested locally by copying over the built java_tools jars and running: ``` EP_PATCH_CHECK=MissingOverride EP_PATCH_CHECK_PATH_ONLY=cme/clients/flipr-java-client/flipr-client tools/bazel build //cme/clients/flipr-java-client/flipr-client:src_main ``` It would be good to write an integration test for this at some point, since it hasn't been upstreamed. Reviewers: cjk Subscribers: ravirajj, ravi Revert Plan: n/a JIRA Issues: JAVADEVX-8326 Differential Revision: https://code.uberinternal.com/D13309409

Summary: This diff contains the changes in bazelbuild#21640, ported to the uber/java/7.0.2.001 branch. We also commit a modified version of the Error Prone check_api jar. The modifications in that jar are those changes in google/error-prone#4318 applied to Error Prone 2.22.0 (the version used in our branch). Probably, rather than committing the modified jar, we could upload the modified jar to artifactory and change the version we depend on here. I have tested that these changes work locally on a devpod. `IN_PLACE` patching will most likely not work under stronger sandboxes, as it relies on modifying the source files in the sandbox (which, by default on a devpod, are just symlinks to the original source files). Also, there seem to be some weird interactions with the Bazel cache, such that when I tried to run the same autopatch twice (after undoing the initial changes), I had to blow away my local `~/.bazelcache` directory to make it work. Test Plan: Tested locally by copying over the built java_tools jars and running: ``` EP_PATCH_CHECK=MissingOverride EP_PATCH_CHECK_PATH_ONLY=a/b/c tools/bazel build //a/b/c:src ``` It would be good to write an integration test for this at some point, since it hasn't been upstreamed. Reviewers: cjk Subscribers: ravirajj, ravi Revert Plan: n/a JIRA Issues: JAVADEVX-8326 Differential Revision: https://code.uberinternal.com/D13309409

cushon · 2024-05-03T20:30:24Z

Just adding support for -XepPatchLocation: doesn't seem ideal for Bazel, because as mentioned earlier IN_PLACE doesn't work with sandboxing, and setting a specific location doesn't make Bazel aware of those outputs, so it requires something like running the action locally and writing them outside the output tree.

One alternative would be to add support for this to Bazel, so it could handle setting up the additional output for the action and passing that path through to JavaBuilder.

At Google we're generating Error Prone patches by using an aspect to create an additional action to run Error Prone for refactoring (instead of re-using the same invocation that happens in JavaBuilder), and having the aspect handle collecting all of the transitive refactoring outputs.

rsalvador · 2024-05-06T06:12:39Z

Just adding support for -XepPatchLocation: doesn't seem ideal for Bazel, because as mentioned earlier IN_PLACE doesn't work with sandboxing, and setting a specific location doesn't make Bazel aware of those outputs, so it requires something like running the action locally and writing them outside the output tree.

One alternative would be to add support for this to Bazel, so it could handle setting up the additional output for the action and passing that path through to JavaBuilder.

Another option could be to use a wrapper rule for java_library that declares the output.

Agreed that the way this PR allows patching is not ideal, but modifying the native bazel rules seemed too involved, maybe it will be better to do once the java rules are refactored to Starlark?

Summary: This diff contains the changes in bazelbuild#21640, ported to the uber/java/7.0.2.001 branch. We also commit a modified version of the Error Prone check_api jar. The modifications in that jar are those changes in google/error-prone#4318 applied to Error Prone 2.22.0 (the version used in our branch). Probably, rather than committing the modified jar, we could upload the modified jar to artifactory and change the version we depend on here. I have tested that these changes work locally on a devpod. `IN_PLACE` patching will most likely not work under stronger sandboxes, as it relies on modifying the source files in the sandbox (which, by default on a devpod, are just symlinks to the original source files). Also, there seem to be some weird interactions with the Bazel cache, such that when I tried to run the same autopatch twice (after undoing the initial changes), I had to blow away my local `~/.bazelcache` directory to make it work. Test Plan: Tested locally by copying over the built java_tools jars and running: ``` EP_PATCH_CHECK=MissingOverride EP_PATCH_CHECK_PATH_ONLY=a/b/c tools/bazel build //a/b/c:src ``` It would be good to write an integration test for this at some point, since it hasn't been upstreamed. Reviewers: cjk Subscribers: ravirajj, ravi Revert Plan: n/a JIRA Issues: JAVADEVX-8326 Differential Revision: https://code.uberinternal.com/D13309409 Update Error Prone jar in Bazel java_tools with further fix Summary: Jar now includes the fix in https://code.uberinternal.com/D13331711?vs=37208341&id=37966971 JIRA Issues: JAVADEVX-8326 Differential Revision: https://code.uberinternal.com/D13628673

to be used by bazel’s error prone plugin: bazelbuild/bazel#21640 Fixes #4318 FUTURE_COPYBARA_INTEGRATE_REVIEW=#4318 from rsalvador:make-patching-api-public 905d626 PiperOrigin-RevId: 634811351

to be used by bazel’s error prone plugin: bazelbuild/bazel#21640 Fixes #4318 COPYBARA_INTEGRATE_REVIEW=#4318 from rsalvador:make-patching-api-public 905d626 PiperOrigin-RevId: 652555417

When running in normal checking mode, Error Prone loads plugin checks lazily to delay the loading until after the javac file manager is initialized; see [here](https://github.com/google/error-prone/blob/8f9e7da16b81b9fd65cca5fef9a79a4e6019eabd/check_api/src/main/java/com/google/errorprone/ErrorProneAnalyzer.java#L145-L159). This change introduces the same lazy loading when running Error Prone in auto-patching mode. In most scenarios I've seen, applying suggested fixes from plugin checks works fine, but when trying to generate patches on Bazel (see bazelbuild/bazel#21640), things do not work without this change. The previous code was "forcing" a `Supplier` and then re-wrapping the result in a `Supplier` again; this change mostly just gets rid of that. Fixes #4467 FUTURE_COPYBARA_INTEGRATE_REVIEW=#4467 from msridhar:lazy-load-plugin-checks-when-autopatching 728e3f9 PiperOrigin-RevId: 652625022

When running in normal checking mode, Error Prone loads plugin checks lazily to delay the loading until after the javac file manager is initialized; see [here](https://github.com/google/error-prone/blob/8f9e7da16b81b9fd65cca5fef9a79a4e6019eabd/check_api/src/main/java/com/google/errorprone/ErrorProneAnalyzer.java#L145-L159). This change introduces the same lazy loading when running Error Prone in auto-patching mode. In most scenarios I've seen, applying suggested fixes from plugin checks works fine, but when trying to generate patches on Bazel (see bazelbuild/bazel#21640), things do not work without this change. The previous code was "forcing" a `Supplier` and then re-wrapping the result in a `Supplier` again; this change mostly just gets rid of that. Fixes #4467 COPYBARA_INTEGRATE_REVIEW=#4467 from msridhar:lazy-load-plugin-checks-when-autopatching 728e3f9 PiperOrigin-RevId: 652638190

Summary: This diff contains the changes in bazelbuild#21640, ported to the uber/java/7.0.2.001 branch. We also commit a modified version of the Error Prone check_api jar. The modifications in that jar are those changes in google/error-prone#4318 applied to Error Prone 2.22.0 (the version used in our branch). Probably, rather than committing the modified jar, we could upload the modified jar to artifactory and change the version we depend on here. I have tested that these changes work locally on a devpod. `IN_PLACE` patching will most likely not work under stronger sandboxes, as it relies on modifying the source files in the sandbox (which, by default on a devpod, are just symlinks to the original source files). Also, there seem to be some weird interactions with the Bazel cache, such that when I tried to run the same autopatch twice (after undoing the initial changes), I had to blow away my local `~/.bazelcache` directory to make it work. Test Plan: Tested locally by copying over the built java_tools jars and running: ``` EP_PATCH_CHECK=MissingOverride EP_PATCH_CHECK_PATH_ONLY=a/b/c tools/bazel build //a/b/c:src ``` It would be good to write an integration test for this at some point, since it hasn't been upstreamed. Reviewers: cjk Subscribers: ravirajj, ravi Revert Plan: n/a JIRA Issues: JAVADEVX-8326 Differential Revision: https://code.uberinternal.com/D13309409 Update Error Prone jar in Bazel java_tools with further fix Summary: Jar now includes the fix in https://code.uberinternal.com/D13331711?vs=37208341&id=37966971 JIRA Issues: JAVADEVX-8326 Differential Revision: https://code.uberinternal.com/D13628673

Generate error-prone patches

034281a

rsalvador requested a review from a team as a code owner March 12, 2024 08:28

github-actions bot added team-Rules-Java Issues for Java rules awaiting-review PR is awaiting review from an assigned reviewer labels Mar 12, 2024

rsalvador mentioned this pull request Mar 13, 2024

Make patching API public google/error-prone#4318

Closed

sgowroji added awaiting-user-response Awaiting a response from the author and removed awaiting-review PR is awaiting review from an assigned reviewer labels Mar 18, 2024

msridhar reviewed Mar 19, 2024

View reviewed changes

sgowroji added awaiting-review PR is awaiting review from an assigned reviewer and removed awaiting-user-response Awaiting a response from the author labels Mar 19, 2024

Switch to MultiTaskListener

bf2107e

rsalvador mentioned this pull request Mar 22, 2024

Error-prone patching cannot be used in bazel google/error-prone#4341

Open

Hasmuni5757 approved these changes Apr 12, 2024

View reviewed changes

copybara-service bot mentioned this pull request Jul 15, 2024

Make patching API public google/error-prone#4466

Closed

msridhar mentioned this pull request Jul 15, 2024

Lazily load plugin checks when running auto-patching google/error-prone#4467

Closed

copybara-service bot mentioned this pull request Jul 15, 2024

Lazily load plugin checks when running auto-patching google/error-prone#4468

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate error-prone patches #21640

Generate error-prone patches #21640

rsalvador commented Mar 12, 2024 •

edited

Loading

rsalvador commented Mar 18, 2024 •

edited

Loading

msridhar left a comment

msridhar Mar 19, 2024

rsalvador Mar 19, 2024

sgowroji commented Mar 19, 2024

msridhar commented Mar 19, 2024

rsalvador commented Mar 20, 2024

rsalvador commented Mar 20, 2024

sgowroji commented Mar 20, 2024

cushon commented May 3, 2024

rsalvador commented May 6, 2024

Generate error-prone patches #21640

Are you sure you want to change the base?

Generate error-prone patches #21640

Conversation

rsalvador commented Mar 12, 2024 • edited Loading

rsalvador commented Mar 18, 2024 • edited Loading

msridhar left a comment

Choose a reason for hiding this comment

msridhar Mar 19, 2024

Choose a reason for hiding this comment

rsalvador Mar 19, 2024

Choose a reason for hiding this comment

sgowroji commented Mar 19, 2024

msridhar commented Mar 19, 2024

rsalvador commented Mar 20, 2024

rsalvador commented Mar 20, 2024

sgowroji commented Mar 20, 2024

cushon commented May 3, 2024

rsalvador commented May 6, 2024

rsalvador commented Mar 12, 2024 •

edited

Loading

rsalvador commented Mar 18, 2024 •

edited

Loading