Generate error-prone patches

[rsalvador]

The current error-prone integration doesn't generate patches: https://github.com/bazelbuild/bazel/issues/5729#issuecomment-1877875315

This PR enables error-prone patch generation and relies on making some existing error-prone members public: https://github.com/google/error-prone/pull/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]

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: https://github.com/google/error-prone/pull/4318

[sgowroji]

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

[msridhar]

@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]

Hi @sgowroji, the error-prone PR (https://github.com/google/error-prone/pull/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]

@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]

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.

[cushon]

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]

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?