bufbuild / buf-gradle-plugin

Gradle plugin for the Buf CLI
Apache License 2.0
49 stars 13 forks source link

Support configuration cache #206

Closed sebek64 closed 1 month ago

sebek64 commented 3 months ago

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 commented 3 months ago

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

sebek64 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

sebek64 commented 3 months ago

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

andrewparmet commented 3 months ago

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 commented 2 months ago

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 commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

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

sebek64 commented 2 months ago

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

andrewparmet commented 2 months ago

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 commented 2 months ago

@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 commented 2 months ago

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

pkwarren commented 2 months ago

Opened https://github.com/bufbuild/buf-gradle-plugin/pull/230 to switch the tests over to local generation.