Skip to content

Commit 45baa91

Browse files
gmazzogmazzo
authored
Fix CommitEdit issues when enabling Configuration Cache (#1086)
1 parent 4831050 commit 45baa91

10 files changed

Lines changed: 65 additions & 31 deletions

File tree

‎play/plugin/src/main/kotlin/com/github/triplet/gradle/play/PlayPublisherPlugin.kt‎

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -41,28 +41,37 @@ import com.github.triplet.gradle.play.tasks.internal.GlobalPublishableArtifactLi
4141
import com.github.triplet.gradle.play.tasks.internal.GlobalUploadableArtifactLifecycleTask
4242
import com.github.triplet.gradle.play.tasks.internal.PlayApiService
4343
import com.github.triplet.gradle.play.tasks.internal.PublishArtifactTaskBase
44+
import com.github.triplet.gradle.play.tasks.internal.PublishTaskBase
4445
import com.github.triplet.gradle.play.tasks.internal.PublishableTrackLifecycleTask
4546
import com.github.triplet.gradle.play.tasks.internal.UpdatableTrackLifecycleTask
4647
import com.github.triplet.gradle.play.tasks.internal.WriteTrackLifecycleTask
48+
import org.gradle.api.DomainObjectSet
4749
import org.gradle.api.Plugin
4850
import org.gradle.api.Project
4951
import org.gradle.api.Task
5052
import org.gradle.api.plugins.ExtensionAware
5153
import org.gradle.api.provider.Provider
5254
import org.gradle.api.services.BuildServiceRegistration
55+
import org.gradle.build.event.BuildEventsListenerRegistry
5356
import org.gradle.kotlin.dsl.container
5457
import org.gradle.kotlin.dsl.create
58+
import org.gradle.kotlin.dsl.domainObjectSet
5559
import org.gradle.kotlin.dsl.findPlugin
5660
import org.gradle.kotlin.dsl.getByType
5761
import org.gradle.kotlin.dsl.invoke
62+
import org.gradle.kotlin.dsl.mapProperty
5863
import org.gradle.kotlin.dsl.named
5964
import org.gradle.kotlin.dsl.newInstance
6065
import org.gradle.kotlin.dsl.registerIfAbsent
66+
import org.gradle.kotlin.dsl.setProperty
6167
import org.gradle.kotlin.dsl.the
6268
import org.gradle.kotlin.dsl.withType
69+
import javax.inject.Inject
6370

6471
@Suppress("unused") // Used by Gradle
65-
internal class PlayPublisherPlugin : Plugin<Project> {
72+
internal abstract class PlayPublisherPlugin @Inject constructor(
73+
private val buildEventsListenerRegistry: BuildEventsListenerRegistry,
74+
) : Plugin<Project> {
6675
override fun apply(project: Project) {
6776
project.validateRuntime()
6877

@@ -236,6 +245,8 @@ internal class PlayPublisherPlugin : Plugin<Project> {
236245
parameters.appId.set(appId)
237246
parameters.editIdFile.set(project.layout.buildDirectory.file("$OUTPUT_PATH/$appId.txt"))
238247
}
248+
buildEventsListenerRegistry.onTaskCompletion(api)
249+
239250
project.gradle.sharedServices.registrations.named<
240251
BuildServiceRegistration<PlayApiService, PlayApiService.Params>
241252
>("playApi-$appId") {
@@ -248,6 +259,11 @@ internal class PlayPublisherPlugin : Plugin<Project> {
248259
}
249260
}
250261

262+
fun PublishTaskBase.bindApi(api: Provider<PlayApiService>) {
263+
usesService(api)
264+
apiService.value(api).disallowChanges()
265+
}
266+
251267
// ------------------- START: ALL BUILD TYPE VARIANT SPECIFIC TASKS -------------------
252268

253269
val publishInternalSharingApkTask = project.newTask<PublishInternalSharingApk>(
@@ -258,7 +274,7 @@ internal class PlayPublisherPlugin : Plugin<Project> {
258274
""".trimMargin(),
259275
arrayOf(extension, executionDir),
260276
) {
261-
apiService.set(api)
277+
bindApi(api)
262278
apks.from(findApkFiles())
263279
outputDirectory.set(project.layout.buildDirectory.dir(
264280
"outputs/internal-sharing/apk/${variant.name}"))
@@ -276,7 +292,7 @@ internal class PlayPublisherPlugin : Plugin<Project> {
276292
""".trimMargin(),
277293
arrayOf(extension, executionDir),
278294
) {
279-
apiService.set(api)
295+
bindApi(api)
280296
bundles.from(findBundleFiles())
281297
outputDirectory.set(project.layout.buildDirectory.dir(
282298
"outputs/internal-sharing/bundle/${variant.name}"))
@@ -317,7 +333,7 @@ internal class PlayPublisherPlugin : Plugin<Project> {
317333
""".trimMargin(),
318334
arrayOf(extension, bootstrapOptionsHolder),
319335
) {
320-
apiService.set(api)
336+
bindApi(api)
321337
srcDir.set(project.file("src/${variant.flavorNameOrDefault}/$PLAY_PATH"))
322338
}
323339
bootstrapAllTask { dependsOn(bootstrapTask) }
@@ -355,7 +371,7 @@ internal class PlayPublisherPlugin : Plugin<Project> {
355371
""".trimMargin(),
356372
arrayOf(extension, executionDir),
357373
) {
358-
apiService.set(api)
374+
bindApi(api)
359375
resDir.set(resourceDir)
360376

361377
finalizedBy(commitEditTask)
@@ -370,7 +386,7 @@ internal class PlayPublisherPlugin : Plugin<Project> {
370386
""".trimMargin(),
371387
arrayOf(extension),
372388
) {
373-
apiService.set(api)
389+
bindApi(api)
374390
productsDir.setFrom(resourceDir.map {
375391
it.dir(PRODUCTS_PATH).asFileTree.matching { include("*.json") }
376392
})
@@ -391,7 +407,7 @@ internal class PlayPublisherPlugin : Plugin<Project> {
391407
extension.resolutionStrategy.get() == ResolutionStrategy.AUTO_OFFSET,
392408
),
393409
) {
394-
apiService.set(api)
410+
bindApi(api)
395411
versionCodes.set(staticVersionCodes)
396412
playVersionCodes.set(project.layout.buildDirectory.file(
397413
"$INTERMEDIATES_OUTPUT_PATH/${variant.name}/available-version-codes.txt"))
@@ -406,7 +422,7 @@ internal class PlayPublisherPlugin : Plugin<Project> {
406422

407423

408424
fun PublishArtifactTaskBase.configureInputs() {
409-
apiService.set(api)
425+
bindApi(api)
410426
releaseNotesDir.set(resourceDir.map {
411427
it.dir(RELEASE_NOTES_PATH).also { it.asFile.safeMkdirs() }
412428
})

‎play/plugin/src/main/kotlin/com/github/triplet/gradle/play/internal/Plugins.kt‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,9 @@ internal fun Project.getCommitEditTask(
5353
): TaskProvider<CommitEdit> {
5454
val taskName = "commitEditFor" + appId.split(".").joinToString("Dot") { it.capitalize() }
5555
return rootProject.newTask(taskName, allowExisting = true, constructorArgs = arrayOf(extension)) {
56+
usesService(api)
5657
apiService.set(api)
58+
onlyIf { !api.get().buildFailed }
5759
}
5860
}
5961

‎play/plugin/src/main/kotlin/com/github/triplet/gradle/play/tasks/CommitEdit.kt‎

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -6,26 +6,18 @@ import com.github.triplet.gradle.play.tasks.internal.workers.PlayWorkerBase
66
import com.github.triplet.gradle.play.tasks.internal.workers.paramsForBase
77
import org.gradle.api.invocation.Gradle
88
import org.gradle.api.logging.Logging
9+
import org.gradle.api.tasks.Internal
910
import org.gradle.api.tasks.TaskAction
1011
import org.gradle.kotlin.dsl.submit
1112
import org.gradle.work.DisableCachingByDefault
1213
import org.gradle.workers.WorkerExecutor
1314
import javax.inject.Inject
1415

15-
@Suppress("LeakingThis")
1616
@DisableCachingByDefault
1717
internal abstract class CommitEdit @Inject constructor(
18-
private val gradle: Gradle,
1918
extension: PlayPublisherExtension,
2019
private val executor: WorkerExecutor,
2120
) : PublishTaskBase(extension) {
22-
init {
23-
onlyIf {
24-
val buildFailed = gradle.taskGraph.allTasks.any { it.state.failure != null }
25-
if (buildFailed) apiService.get().cleanup()
26-
!buildFailed
27-
}
28-
}
2921

3022
@TaskAction
3123
fun commit() {
@@ -36,14 +28,14 @@ internal abstract class CommitEdit @Inject constructor(
3628

3729
abstract class Committer : PlayWorkerBase<Committer.Params>() {
3830
override fun execute() {
39-
if (apiService.shouldCommit()) {
31+
if (apiService.shouldCommit) {
4032
println("Committing changes")
4133
try {
4234
apiService.commit()
4335
} finally {
4436
apiService.cleanup()
4537
}
46-
} else if (apiService.shouldSkip()) {
38+
} else if (apiService.shouldSkip) {
4739
println("Changes pending commit")
4840
try {
4941
apiService.validate()

‎play/plugin/src/main/kotlin/com/github/triplet/gradle/play/tasks/PublishApk.kt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ internal abstract class PublishApk @Inject constructor(
9999
}.sorted()
100100
apiService.edits.publishArtifacts(
101101
versions,
102-
apiService.shouldSkip(),
102+
apiService.shouldSkip,
103103
config.track,
104104
config.releaseStatus,
105105
findReleaseName(config.track),

‎play/plugin/src/main/kotlin/com/github/triplet/gradle/play/tasks/PublishBundle.kt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ internal abstract class PublishBundle @Inject constructor(
7676
}.sorted()
7777
apiService.edits.publishArtifacts(
7878
versions,
79-
apiService.shouldSkip(),
79+
apiService.shouldSkip,
8080
config.track,
8181
config.releaseStatus,
8282
findReleaseName(config.track),

‎play/plugin/src/main/kotlin/com/github/triplet/gradle/play/tasks/internal/PlayApiService.kt‎

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,13 +14,17 @@ import org.gradle.api.logging.Logging
1414
import org.gradle.api.provider.Property
1515
import org.gradle.api.services.BuildService
1616
import org.gradle.api.services.BuildServiceParameters
17+
import org.gradle.tooling.events.FinishEvent
18+
import org.gradle.tooling.events.OperationCompletionListener
19+
import org.gradle.tooling.events.task.TaskFailureResult
20+
import org.gradle.tooling.events.task.TaskFinishEvent
1721
import java.io.ByteArrayInputStream
1822
import java.io.InputStream
1923
import javax.inject.Inject
2024

2125
internal abstract class PlayApiService @Inject constructor(
2226
private val fileOps: FileSystemOperations,
23-
) : BuildService<PlayApiService.Params> {
27+
) : BuildService<PlayApiService.Params>, OperationCompletionListener, AutoCloseable {
2428
val publisher by lazy {
2529
credentialStream().use {
2630
PlayPublisher(it, parameters.appId.get())
@@ -37,11 +41,14 @@ internal abstract class PlayApiService @Inject constructor(
3741
private val editIdFileAndFriends
3842
get() = listOf(editIdFile, editIdFile.marked("commit"), editIdFile.marked("skipped"))
3943

44+
var buildFailed = false
45+
private set
46+
4047
fun scheduleCommit() {
4148
editIdFile.marked("commit").safeCreateNewFile()
4249
}
4350

44-
fun shouldCommit(): Boolean = editIdFile.marked("commit").exists()
51+
val shouldCommit get() = editIdFile.marked("commit").exists()
4552

4653
fun commit() {
4754
val response = publisher.commitEdit(editId)
@@ -59,7 +66,7 @@ internal abstract class PlayApiService @Inject constructor(
5966
editIdFile.marked("skipped").safeCreateNewFile()
6067
}
6168

62-
fun shouldSkip(): Boolean = editIdFile.marked("skipped").exists()
69+
val shouldSkip get() = editIdFile.marked("skipped").exists()
6370

6471
fun validate() {
6572
publisher.validateEdit(editId)
@@ -69,6 +76,16 @@ internal abstract class PlayApiService @Inject constructor(
6976
fileOps.delete { delete(editIdFileAndFriends) }
7077
}
7178

79+
override fun close() {
80+
if (shouldCommit) {
81+
cleanup()
82+
}
83+
}
84+
85+
override fun onFinish(event: FinishEvent) {
86+
buildFailed = buildFailed || event.result is TaskFailureResult
87+
}
88+
7289
private fun getOrCreateEditId(): String {
7390
val editId = editIdFile.orNull()?.readText().nullOrFull()?.takeIf {
7491
editIdFile.marked("skipped").exists()

‎play/plugin/src/test/kotlin/com/github/triplet/gradle/play/helpers/IntegrationTest.kt‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@ interface IntegrationTest {
1010

1111
val factoryInstallerStatement: String
1212

13+
val withConfigurationCache: Boolean
14+
1315
fun File.escaped(): String
1416

1517
fun execute(config: String, vararg tasks: String): BuildResult

‎play/plugin/src/test/kotlin/com/github/triplet/gradle/play/helpers/IntegrationTestBase.kt‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,9 @@ import java.util.concurrent.BlockingQueue
1313
import kotlin.math.max
1414
import kotlin.random.Random
1515

16-
abstract class IntegrationTestBase : IntegrationTest {
16+
abstract class IntegrationTestBase(
17+
override val withConfigurationCache: Boolean = false
18+
) : IntegrationTest {
1719
@TempDir
1820
@JvmField
1921
var _tempDir: File? = null
@@ -54,9 +56,10 @@ abstract class IntegrationTestBase : IntegrationTest {
5456
.withTestKitDir(testDir)
5557
.apply(block)
5658

57-
if (!expectFailure) {
58-
runner.withArguments(runner.arguments.toMutableList() + "-S")
59-
}
59+
runner.withArguments(runner.arguments + listOfNotNull(
60+
"-S".takeIf { !expectFailure },
61+
"--configuration-cache".takeIf { withConfigurationCache },
62+
))
6063

6164
// We're doing some pretty wack (and disgusting, shameful) shit to run integration tests without
6265
// actually publishing anything. The idea is have the build file call into the test class to run

‎play/plugin/src/test/kotlin/com/github/triplet/gradle/play/tasks/CommitEditIntegrationTest.kt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ import org.gradle.testkit.runner.TaskOutcome.SUCCESS
1313
import org.junit.jupiter.api.Test
1414
import java.io.File
1515

16-
class CommitEditIntegrationTest : IntegrationTestBase(), SharedIntegrationTest {
16+
class CommitEditIntegrationTest : IntegrationTestBase(withConfigurationCache = true), SharedIntegrationTest {
1717
override fun taskName(taskVariant: String) = ":commitEditForComDotExampleDotPublisher"
1818

1919
@Test

‎play/plugin/src/test/kotlin/com/github/triplet/gradle/play/tasks/GenerateEditIntegrationTest.kt‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -70,10 +70,12 @@ class GenerateEditIntegrationTest : IntegrationTestBase(), SharedIntegrationTest
7070
tasks.register("gen") { gen ->
7171
def service = gradle.sharedServices.registrations
7272
.named("playApi-com.example.publisher")
73-
.get().service.get() as PlayApiService
73+
.get().service
74+
75+
usesService(service)
7476
7577
doLast {
76-
service.edits
78+
service.get().edits
7779
}
7880
}
7981

0 commit comments

Comments
 (0)