Support configuration cache

[sebek64]

Support configuration cache.

This is the current standard for plugins. It makes the builds faster by supporting heavy parallelism in builds, and slow whenever a non-compliant plugin is part of the build.

I tried carefully to set all inputs / outputs of tasks. It is however quite hard to handle all imaginable cases.

The commits should be small and reviewable. If anything is not clear from the commits, I'm ready to explain.

[sebek64]

I already found one case where it doesn't work correctly, so a fix should be added soon (hopefully next Monday).

[sebek64]

The fix has been pushed - bufTool configuration creation before task creation.
With the new inputs/outputs configured, it can be a bit tricky to use this plugin in combination with other plugins like java(-library), in particular its processResources task. The user has to explicitly specify dependency of BufLint / BufFormatCheck (and possibly others) on the processResources task.
Because of that, if this PR gets merged, I suggest releasing the plugin with new "major" version (0.10.0?), to make the compatibility breaking obvious.

[andrewparmet]

Thanks for all the work in this contribution!

Is the processResources dependency the only complication for users that this PR introduces? Did that dependency always exist and the plugin just happened to work?

I think requiring users to declare that dependency is a non-starter - we should try as hard as we can not to require users to do any task dependency declarations like that.

[sebek64]

Thanks for quick reaction. After a few days of usage of this "modified" plugin version, I think I understand the traps and problems a bit more.
In the current version, the tasks do not specify any inputs/outputs. So whenever the task is requested, it is always run. You can see that on info logging level:

./gradlew bufBuild --info
...
Task ':...:bufBuild' is not up-to-date because:
  Task has not declared any outputs despite executing actions.
Running buf from ...: `buf build --output ...build/bufbuild/image.json`

In the new version, the inputs/outputs are declared, so the buf is only executed if the inputs change. So for example

./gradlew BufBuild --info
...
> Task :...:bufBuild UP-TO-DATE
Caching disabled for task ':...:bufBuild' because:
  Build cache is disabled
  Caching has not been enabled for the task
Skipping task ':...:bufBuild' as it is up-to-date.

Now we come to the fact that buf tool itself is not very compatible with gradle way of organizing repositories. The standard gradle way is to have two directories in the module - src and build. Simply speaking, task inputs are usually in src, task outputs are usually in build.
Sometimes, tasks can have inputs in build. Then the tasks are chained and explicit dependencies between them is needed. A real-world example is processResources task, reading some files from src/.../resources, and storing them to build/.../resources. Then some other tasks like jar have build/.../resources as the inputs. Therefore, jar has to depend on processResources.
Sometimes, tasks can have outputs in src directory. For example, ktlint (kotlin linter) has a ktlintFormat task, which applies linting rules to existing sources. This task has outputs in the src directory.
With buf directory layout, the sources are placed directly into the root tree (or at least that's one of the options). Then the inputs are basically taken from there. It is possible to have inputs even in the build directory, leading to the need of explicit specification of task dependencies.
In this PR, I explicitly excluded build from task inputs. Then the build doesn't fail because of missing task dependencies. But it can possibly incorrectly skip a task run if the inputs are in the build directory (outputs of some other task), but not declared explicitly.
Part of the problem is that this plugin is not configured entirely inside gradle, but partially also via external files (buf.yaml). I'm afraid there is no perfect solution, except for doing even bigger refactoring, and encapsulating all the configuration (at least regarding inputs/outputs) into the gradle itself, and generating buf.yaml as part of plugin execution.
I'm open to discussion. We currenly use a forked version for our needs, so we may polish this PR and take time with it.

[andrewparmet]

Thanks for the detailed explanation. I guess phrased differently my question is: what is it about this change that makes the plugin now depend on processResources?

[sebek64]

Let me reply shortly this time. There is nothing that makes this plugin depend on processResources. The first comment mentioning this task was before the build directory was added to excludes (see the last commit in this PR).

[andrewparmet]

Sorry for the delay here. @drice-buf for visibility.

Ok, I think I see what you're saying - where previously there was some awkwardness, you've now arrived at a place where there is no complexity imparted upon the user of the plugin to support the configuration cache. That's great news, and I think it is a Pareto improvement so long as there are few edge cases you haven't considered. To be honest, the Gradle configuration cache is an area I know little about (perhaps this is obvious), so bear with me as I try to get up to speed on this PR.

[andrewparmet]

Nice dogfooding.

[andrewparmet]

Is there any way to restrict the visibility of all of these properties on the various task classes? I'd rather them not become part of the plugin's public API.

Also, while it's fresh, I think we should get proper KDoc on each property (even if we can make them internal). I'll never remember what they are a year from now...

[sebek64]

Added a few kdocs, although their usefulness is a bit low.
Not sure about the way to hide the inputs/outputs - kotlin internal doesn't work, Internal annotation in gradle has different meaning and semantics.

[andrewparmet]

There's an @Internal Gradle annotation - I wonder what that does.

E.g. https://github.com/google/protobuf-gradle-plugin/blob/master/src/main/groovy/com/google/protobuf/gradle/ProtobufExtract.groovy#L71

Ah just read your last sentence more carefully. It doesn't hide the element? I thought I saw something like that before.

[andrewparmet]

What goes wrong with Kotlin internal? I can add it to all of the abstract vals locally and the tests run just fine.

[andrewparmet]

although their usefulness is a bit low.

I happen to think the nontrivial ones (and even some of the simpler ones) will be helpful and establish good practice. Thanks for adding them!

[sebek64]

The issue with kotlin internal is that I cannot put that on parent classes (AbstractBufExecTask, AbstractBufTask) - I don't know exactly why. Putting it just on the concrete classes seems very inconsistent.

[andrewparmet]

That's exactly what I tried and it worked. What happens when you try?

[sebek64]

When I change AbstractBufTask.projectDir to internal, the test fail on

FAILURE: Build failed with an exception.

* What went wrong:
Could not determine the dependencies of task ':publishBufImagePublicationPublicationToMavenRepository'.
> Could not create task ':bufBuild'.
   > 'org.gradle.api.provider.Property build.buf.gradle.AbstractBufTask.getProjectDir()'

[andrewparmet]

How strange - they pass for me. What could be happening? I'm running OpenJDK Runtime Environment Corretto-11.0.18.10.1 (build 11.0.18+10-LTS).

[sebek64]

Ok, found the issue. It is broken this way if I don't run gradle clean after adding the internal modifier. That's quite strange, but anyway, it's not a blocker for changing it to internal. I'll prepare an extra commit that internalizes all new input/output properties.

[andrewparmet]

Looks good to me, but a single test is failing on Windows.

[sebek64]

Is there a way to see which test / how it failed?

[andrewparmet]

CI used to publish the results to the actions summary and it looks like that broke at some point during maintenance.

I would try using Gradle test logging events?

[sebek64]

Ok, the failing test is GenerateTest > generate java with --include-imports() - failing on bufLint. The crash is quite strange, almost like some out-of-memory in the test worker. Not sure what to do with that.

[andrewparmet]

@sebek64 I've done my best to merge in the main branch here but there's some semantic conflicts with Buf's v2 configurations. I'm pausing here but feel free to step in or I'll come back later.

[sebek64]

I have already a rebase in progress locally, but I wasn't able to finish it yet. I'll come back to it next week.

[andrewparmet]

Ok, feel free to ditch my commits if you want.

[sebek64]

Rebased & adapted to changes. Also merged the commits added during previous reviews into correct commits.

[andrewparmet]

Looks like the failure in the Windows test is related to rate limiting:

2024-08-26T16:39:45.4073581Z GenerateWithWorkspaceTest > buf generate with buf gen template file override() FAILED
2024-08-26T16:39:45.4077823Z     org.gradle.testkit.runner.UnexpectedBuildFailure: Unexpected build execution failure in C:\Users\RUNNER~1\AppData\Local\Temp\junit-10693879979549064926 with arguments [-PprotobufGradleVersion=0.9.3, -PprotobufVersion=3.23.4, -PkotlinVersion=1.7.20, -PandroidGradleVersion=7.3.0, --configuration-cache, build]
2024-08-26T16:39:45.4080400Z 
2024-08-26T16:39:45.4080586Z     Output:
2024-08-26T16:39:45.4081371Z     Calculating task graph as no cached configuration is available for tasks: build
2024-08-26T16:39:45.4082566Z     > Task :processResources NO-SOURCE
2024-08-26T16:39:45.4083422Z     > Task :processTestResources NO-SOURCE
2024-08-26T16:39:45.4084125Z     > Task :bufFormatCheck
2024-08-26T16:39:45.4084702Z     > Task :bufLint
2024-08-26T16:39:45.4085239Z     > Task :bufGenerate FAILED
2024-08-26T16:39:45.4085603Z 
2024-08-26T16:39:45.4085821Z     FAILURE: Build failed with an exception.
2024-08-26T16:39:45.4086110Z 
2024-08-26T16:39:45.4086221Z     * What went wrong:
2024-08-26T16:39:45.4086652Z     Execution failed for task ':bufGenerate'.
2024-08-26T16:39:45.4089024Z     > > arguments: [D:\a\buf-gradle-plugin\buf-gradle-plugin\build\tmp\test\work\.gradle-test-kit\caches\modules-2\files-2.1\build.buf\buf\1.38.0\d6e207e0d0ca484bbc165b7c10acb85ec0c9eb60\buf-1.38.0-windows-x86_64.exe, generate, --output, C:\Users\runneradmin\AppData\Local\Temp\junit-10693879979549064926\build\bufbuild\generated, --template, C:\Users\runneradmin\AppData\Local\Temp\junit-10693879979549064926\subdir\buf.gen.yaml]
2024-08-26T16:39:45.4091085Z       > exit code: 1
2024-08-26T16:39:45.4091378Z       >    stdout: (empty)
2024-08-26T16:39:45.4091680Z       >    stderr: (below)
2024-08-26T16:39:45.4092530Z       > Failure: Unauthenticated remote plugin rate limit exceeded. For info on how to resolve this, visit: https://buf.build/b/auth-required
2024-08-26T16:39:45.4093296Z       > 
2024-08-26T16:39:45.4093430Z 
2024-08-26T16:39:45.4093436Z 
2024-08-26T16:39:45.4093535Z     * Try:
2024-08-26T16:39:45.4093925Z     > Run with --stacktrace option to get the stack trace.
2024-08-26T16:39:45.4094512Z     > Run with --info or --debug option to get more log output.
2024-08-26T16:39:45.4095032Z     > Run with --scan to get full insights.
2024-08-26T16:39:45.4095462Z     > Get more help at https://help.gradle.org.

@pkwarren @drice-buf any ideas on the best way to go about fixing this?

[pkwarren]

@pkwarren @drice-buf any ideas on the best way to go about fixing this?

To avoid rate limits w/ code generation, clients need to be authenticated with the BSR. Alternatively, we could switch to using local generation for the tests instead of relying on remote generation. Looking at how we might do the latter.

[andrewparmet]

It's probably overkill to exercise remote generation in tests - not a bad idea to see if we can do it locally.

[pkwarren]

Opened #230 to switch the tests over to local generation.

[andrewparmet]

Marvelous.