Get Gradle to do more of the work in our Gradle plugin #2838

swankjesse · 2024-02-27T00:05:56Z

In particular leave more of the dependency management up to Gradle.

oldergod

Very exciting.

oldergod · 2024-02-27T10:07:03Z

wire-gradle-plugin/src/main/kotlin/com/squareup/wire/gradle/WirePlugin.kt

@@ -218,7 +197,16 @@ class WirePlugin : Plugin<Project> {
      val task = project.tasks.register(taskName, WireTask::class.java) { task: WireTask ->
        task.group = GROUP
        task.description = "Generate protobuf implementation for ${source.name}"
-        task.source(protoSourceInput.configuration)
+
+        // Flatten all the input files here. Changes to any of them will cause the task to re-run.


~~Is it even required? Given there is sourceInput and protoInput below, might be possible for WireTask to extend DefaultTask instead of DefaultTask?~~

Edit I read below we want NO-SOURCE if no source is found. In which case, I think protoSource should be in source and protoPath passed ~~out of the band?~~ in a regular task property?

I think you’re strictly correct, and also I don’t think it’ll matter much in practice?
Both skipping the task and running it when there’s no sources will both be super fast and rare.

Agreed 👍

oldergod · 2024-02-27T10:08:16Z

wire-gradle-plugin/src/test/kotlin/com/squareup/wire/gradle/WirePluginTest.kt

  fun sourcePathDirDoesNotExist() {
    val fixtureRoot = File("src/test/projects/sourcepath-nonexistent-dir")

-    val result = gradleRunner.runFixture(fixtureRoot) { buildAndFail() }
+    val result = gradleRunner.runFixture(fixtureRoot) { build() }

-    assertThat(result.task(":generateProtos")).isNull()
-    assertThat(result.output).contains(
-      """
-            |Invalid path string: "src/main/proto".
-            |For individual files, use the following syntax:
-            |wire {
-            |  sourcePath {
-            |    srcDir 'dirPath'
-            |    include 'relativePath'
-            |  }
-            |}
-      """.trimMargin(),
-    )
+    assertThat(result.task(":generateMainProtos")?.getOutcome())
+      .isEqualTo(TaskOutcome.NO_SOURCE)


I'm cool with it

oldergod · 2024-02-27T10:13:51Z

I pushed a commit with spotless and apiDump

martinbonnin

I left a few non-blocking comments, mainly for possible future improvements.

This change is a nice simplification 👍

martinbonnin · 2024-02-27T10:48:31Z

wire-gradle-plugin/api/wire-gradle-plugin.api

-	public final fun getSourceJars ()Ljava/util/Set;
-	public final fun getSourcePaths ()Ljava/util/Set;
-	public final fun getSourceTrees ()Ljava/util/Set;


Technically breaking but I'm assuming these APIs that are really supposed to be called from user land?

Yep, exactly. These should have always been implementation details.

martinbonnin · 2024-02-27T11:15:22Z

wire-gradle-plugin/src/main/kotlin/com/squareup/wire/gradle/WirePlugin.kt

-        (protoSourceInput.dependencies + protoPathInput.dependencies).filterIsInstance<ProjectDependency>()
-      for (projectDependency in projectDependencies) {
-        projectDependenciesJvmConfiguration.dependencies.add(projectDependency)
+      if (extension.protoSourceProtoRootSets.all { it.roots.isEmpty() }) {


Note: this probably resolves configurations eagerly. I think this is the case already so not a huge deal in the grand scheme of things but would be a good candidate for future improvements.

Great idea. We can move this behaviour to execution time.

In follow-up!

martinbonnin · 2024-02-27T11:17:24Z

wire-gradle-plugin/src/test/kotlin/com/squareup/wire/gradle/WirePluginTest.kt

+        |    srcDir 'dirPath'
+        |    include 'relativePath'


For those cases, I like using a syntax that works in both Groovy and Kotlin:

Suggested change

| srcDir 'dirPath'

| include 'relativePath'

| srcDir("dirPath")

| include("relativePath")

It's less "idiomatic" in Groovy though so either work with me

Great idea.

martinbonnin · 2024-02-27T11:21:51Z

wire-gradle-plugin/src/main/kotlin/com/squareup/wire/gradle/WirePlugin.kt

@@ -218,7 +197,16 @@ class WirePlugin : Plugin<Project> {
      val task = project.tasks.register(taskName, WireTask::class.java) { task: WireTask ->
        task.group = GROUP
        task.description = "Generate protobuf implementation for ${source.name}"
-        task.source(protoSourceInput.configuration)
+
+        // Flatten all the input files here. Changes to any of them will cause the task to re-run.


~~Is it even required? Given there is sourceInput and protoInput below, might be possible for WireTask to extend DefaultTask instead of DefaultTask?~~

Edit I read below we want NO-SOURCE if no source is found. In which case, I think protoSource should be in source and protoPath passed ~~out of the band?~~ in a regular task property?

martinbonnin · 2024-02-27T11:36:27Z

wire-gradle-plugin/src/main/kotlin/com/squareup/wire/gradle/WireInput.kt

+  // We store [file] relative to the [project] in order to not invalidate the cache when we don't
+  // have to.
+  return InputLocation(project.relativePath(file.path), includes, excludes)


Note: It's technically ok to move the directory around as long as its contents stay the same so there might still be spurious cache invalidations here.

Remote caching is the big thing I wanna preserve. I think we get that as long as the paths are relative to the project.

swankjesse · 2024-02-27T12:20:43Z

Build failure in samples means I’ve made an unintentional behaviour change and our tests didn’t catch it! Yikes!

> Task :samples:android-lib-java-sample:javaPreCompileDebug
e: file:///home/runner/work/wire/wire/samples/android-app-variants-sample/src/testRelease/java/com/squareup/wire/android/app/variants/ReleaseUnitTest.kt:50:23 Unresolved reference: ReleaseType

oldergod · 2024-02-27T12:24:29Z

Samples are also used as tests. Way simpler to work with than testing against the plugin directly.

swankjesse · 2024-02-27T16:15:00Z

Ugh, the failing Windows build shows me I’ve made a design error in my implementation.

The string I’m passing to InputLocation() is the relative path on the file system from my projects’ directory to the .jar or directory. For example, given these inputs:
projectDirectory=/Users/jwilson/Development/greeter/hello
path=/Users/jwilson/Development/greeter/hello/src/main/proto
It correctly returns a relative path output:
result=src/main/proto

Unfortunately if my path isn’t in the project directory, the result I’m producing is bananas and won’t work with Gradle’s cache:
projectDirectory=/Users/jwilson/Development/greeter/hello
path=/Users/jwilson/.gradle/caches/com/example/library/protos.jar
This produces a nonsense relative path:
result=../../../.gradle/caches/com/example/library/1.2.3/protos-1.2.3.jar
In particular, the relative path is now conditional on where my project and library directories are on the file system, and I don’t want that.

I think instead I need to change InputLocation to be either a File relative to the project, or a symbolic identifier for whatever library it’s pulling in, like say
library=com.example.library:protos:1.2.3

Ugh.

swankjesse · 2024-02-27T17:36:35Z

Here’s a tracking issue to fix that path problem: #2842

swankjesse force-pushed the jwilson.0226.gradle_more_work branch from 1009bd5 to 9b51f6b Compare February 27, 2024 02:45

Get Gradle to do more of the work in our Gradle plugin

14f19a3

In particular leave more of the dependency management up to Gradle.

swankjesse force-pushed the jwilson.0226.gradle_more_work branch from 9b51f6b to 14f19a3 Compare February 27, 2024 03:48

swankjesse marked this pull request as ready for review February 27, 2024 03:51

swankjesse requested review from oldergod and martinbonnin February 27, 2024 03:51

oldergod approved these changes Feb 27, 2024

View reviewed changes

Applied spotless

3151263

oldergod force-pushed the jwilson.0226.gradle_more_work branch from dbfed0e to 3151263 Compare February 27, 2024 10:38

martinbonnin approved these changes Feb 27, 2024

View reviewed changes

Fix regression with custom source sets

41ad420

Only relativize project paths

8eeca3f

squarejesse added 3 commits February 27, 2024 12:42

Handle fully-qualified path in toLocations

65065a4

Restore the data class

990bd5b

Fixup api dump

ed480f3

swankjesse merged commit df90610 into master Feb 28, 2024
7 checks passed

swankjesse deleted the jwilson.0226.gradle_more_work branch February 28, 2024 03:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get Gradle to do more of the work in our Gradle plugin #2838

Get Gradle to do more of the work in our Gradle plugin #2838

swankjesse commented Feb 27, 2024

oldergod left a comment

oldergod Feb 27, 2024

martinbonnin Feb 27, 2024 •

edited

Loading

swankjesse Feb 29, 2024

martinbonnin Feb 29, 2024

oldergod Feb 27, 2024

oldergod commented Feb 27, 2024 •

edited

Loading

martinbonnin left a comment

martinbonnin Feb 27, 2024

swankjesse Feb 27, 2024

martinbonnin Feb 27, 2024

swankjesse Feb 27, 2024

swankjesse Feb 27, 2024

martinbonnin Feb 27, 2024

swankjesse Feb 27, 2024

swankjesse Feb 28, 2024

martinbonnin Feb 27, 2024 •

edited

Loading

martinbonnin Feb 27, 2024

swankjesse Feb 27, 2024

swankjesse commented Feb 27, 2024

oldergod commented Feb 27, 2024 •

edited

Loading

swankjesse commented Feb 27, 2024

swankjesse commented Feb 27, 2024

Get Gradle to do more of the work in our Gradle plugin #2838

Get Gradle to do more of the work in our Gradle plugin #2838

Conversation

swankjesse commented Feb 27, 2024

oldergod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinbonnin Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oldergod commented Feb 27, 2024 • edited Loading

martinbonnin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinbonnin Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swankjesse commented Feb 27, 2024

oldergod commented Feb 27, 2024 • edited Loading

swankjesse commented Feb 27, 2024

swankjesse commented Feb 27, 2024

martinbonnin Feb 27, 2024 •

edited

Loading

oldergod commented Feb 27, 2024 •

edited

Loading

martinbonnin Feb 27, 2024 •

edited

Loading

oldergod commented Feb 27, 2024 •

edited

Loading