From ebbd69b2f22f7130347241d3ec4d3c2a8359a2d9 Mon Sep 17 00:00:00 2001 From: Bjarne Koll Date: Thu, 24 Apr 2025 21:22:49 +0200 Subject: [PATCH 1/4] Allow selecting exactly n patches during roulette The package local implementation of the selection strategy works well for large updates as single packages offer a great "unit" of patches to work on. When updating paper to specific snapshots on a weekly basis, generally the amount of non-applicable patches is a lot lower. Being restricted to a package in this case is bad dx, as most time is spent selecting more patches and finishing them, when 10 patches would be a much better "unit" of patches than a package with a single failed patch. --- .../src/main/kotlin/config-kotlin.gradle.kts | 1 + .../AbstractPatchRouletteTask.kt | 2 +- .../tasks/patchroulette/PatchRouletteApply.kt | 25 +++- .../patchroulette/PatchRouletteApplyTest.kt | 127 ++++++++++++++++++ 4 files changed, 150 insertions(+), 5 deletions(-) create mode 100644 paperweight-core/src/test/kotlin/io/papermc/paperweight/core/tasks/patchroulette/PatchRouletteApplyTest.kt diff --git a/buildSrc/src/main/kotlin/config-kotlin.gradle.kts b/buildSrc/src/main/kotlin/config-kotlin.gradle.kts index e8e4e8e0e..d21258b9c 100644 --- a/buildSrc/src/main/kotlin/config-kotlin.gradle.kts +++ b/buildSrc/src/main/kotlin/config-kotlin.gradle.kts @@ -68,6 +68,7 @@ testing { useKotlinTest(embeddedKotlinVersion) dependencies { implementation("org.junit.jupiter:junit-jupiter-engine:5.10.1") + implementation("org.junit.jupiter:junit-jupiter-params:5.10.1") implementation("org.junit.platform:junit-platform-launcher:1.10.1") } diff --git a/paperweight-core/src/main/kotlin/io/papermc/paperweight/core/tasks/patchroulette/AbstractPatchRouletteTask.kt b/paperweight-core/src/main/kotlin/io/papermc/paperweight/core/tasks/patchroulette/AbstractPatchRouletteTask.kt index abcd8a9f2..d54a3ff87 100644 --- a/paperweight-core/src/main/kotlin/io/papermc/paperweight/core/tasks/patchroulette/AbstractPatchRouletteTask.kt +++ b/paperweight-core/src/main/kotlin/io/papermc/paperweight/core/tasks/patchroulette/AbstractPatchRouletteTask.kt @@ -54,7 +54,7 @@ abstract class AbstractPatchRouletteTask : BaseTask() { override fun init() { super.init() - endpoint.convention("https://patch-roulette.papermc.io/api") + endpoint.convention("http://localhost:8080/api") authToken.convention(providers.gradleProperty("paperweight.patch-roulette-token")) doNotTrackState("Run when requested") } diff --git a/paperweight-core/src/main/kotlin/io/papermc/paperweight/core/tasks/patchroulette/PatchRouletteApply.kt b/paperweight-core/src/main/kotlin/io/papermc/paperweight/core/tasks/patchroulette/PatchRouletteApply.kt index 8df9780f7..856a0c1f0 100644 --- a/paperweight-core/src/main/kotlin/io/papermc/paperweight/core/tasks/patchroulette/PatchRouletteApply.kt +++ b/paperweight-core/src/main/kotlin/io/papermc/paperweight/core/tasks/patchroulette/PatchRouletteApply.kt @@ -87,7 +87,7 @@ abstract class PatchRouletteApply : AbstractPatchRouletteTask() { } var tries = 5 - var patches = listOf() + var patches: List val patchSelectionStrategy = patchSelectionStrategy .map { PatchSelectionStrategy.parse(it) } .getOrElse(PatchSelectionStrategy.NumericInPackage(5)) @@ -166,11 +166,25 @@ abstract class PatchRouletteApply : AbstractPatchRouletteTask() { data class Config(val skip: List, val suggestedPackage: Path?, val currentPatches: List) sealed interface PatchSelectionStrategy { - data class NumericInPackage(val count: Int) : PatchSelectionStrategy { + data class NumericInPackage(val count: Int, val enforceCount: Boolean = false) : PatchSelectionStrategy { override fun select(config: Config, available: List): Pair> { if (config.suggestedPackage != null) { val possiblePatches = available.filter { it.parent.equals(config.suggestedPackage) }.take(count) - if (possiblePatches.isNotEmpty()) return config to possiblePatches + if (possiblePatches.isNotEmpty()) { + if (!enforceCount) return config to possiblePatches + // The patches we found satisfy the count param or the entire available set simply does not offer enough patches. + if (possiblePatches.size >= count || possiblePatches.size == available.size) return config to possiblePatches + + // The patches found in the package do not satisfy the requested count *and* the strategy was configured to enforce the + // count. Re-select from a new package and different patch set, add them to our already fetched patches and update the + // config, as the last suggested package is the one to suggest in potentially next runs. + val additionalPatches = select( + config.copy(suggestedPackage = null), + available.filter { !possiblePatches.contains(it) } + ) + + return additionalPatches.first to possiblePatches + additionalPatches.second + } } return select(config.copy(suggestedPackage = available.first().parent), available) @@ -182,7 +196,10 @@ abstract class PatchRouletteApply : AbstractPatchRouletteTask() { companion object { fun parse(input: String): PatchSelectionStrategy { try { - return NumericInPackage(input.toInt()) + return when { + input.endsWith("!") -> NumericInPackage(input.substring(0, input.length - 1).toInt(), true) + else -> NumericInPackage(input.toInt()) + } } catch (e: Exception) { throw PaperweightException("Failed to parse patch selection strategy $input", e) } diff --git a/paperweight-core/src/test/kotlin/io/papermc/paperweight/core/tasks/patchroulette/PatchRouletteApplyTest.kt b/paperweight-core/src/test/kotlin/io/papermc/paperweight/core/tasks/patchroulette/PatchRouletteApplyTest.kt new file mode 100644 index 000000000..bd1b56bdf --- /dev/null +++ b/paperweight-core/src/test/kotlin/io/papermc/paperweight/core/tasks/patchroulette/PatchRouletteApplyTest.kt @@ -0,0 +1,127 @@ +/* + * paperweight is a Gradle plugin for the PaperMC project. + * + * Copyright (c) 2023 Kyle Wood (DenWav) + * Contributors + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; + * version 2.1 only, no later versions. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 + * USA + */ + +package io.papermc.paperweight.core.tasks.patchroulette + +import kotlin.io.path.* +import kotlin.test.Test +import kotlin.test.assertEquals +import org.junit.jupiter.api.Assertions.assertFalse +import org.junit.jupiter.api.Assertions.assertTrue +import org.junit.jupiter.params.ParameterizedTest +import org.junit.jupiter.params.provider.Arguments +import org.junit.jupiter.params.provider.MethodSource + +class PatchRouletteApplyTest { + + @Test + fun `test patch strategy parsing non enforced`() { + val strategy = PatchRouletteApply.PatchSelectionStrategy.parse("10") + when (strategy) { + is PatchRouletteApply.PatchSelectionStrategy.NumericInPackage -> { + assertEquals(10, strategy.count) + assertFalse(strategy.enforceCount) + } + } + } + + @Test + fun `test patch strategy parsing enforced`() { + val strategy = PatchRouletteApply.PatchSelectionStrategy.parse("20!") + when (strategy) { + is PatchRouletteApply.PatchSelectionStrategy.NumericInPackage -> { + assertEquals(20, strategy.count) + assertTrue(strategy.enforceCount) + } + } + } + + @ParameterizedTest + @MethodSource("testPatchSelectionSource") + fun `test patch selection`( + strategy: PatchRouletteApply.PatchSelectionStrategy, + allPatches: List, + expectedPatchBatches: List> + ) { + var config = PatchRouletteApply.Config(listOf(), null, listOf()) + var availablePatches = allPatches.map { Path(it) } + for (batch in expectedPatchBatches) { + val selectionResult = strategy.select(config, availablePatches) + assertEquals(batch.map { Path(it) }, selectionResult.second) + + config = selectionResult.first + availablePatches = availablePatches - selectionResult.second + } + + assertTrue(availablePatches.isEmpty(), "Patches remained after exhausting expected batches") + } + + companion object { + @JvmStatic + fun testPatchSelectionSource(): Collection = listOf( + Arguments.of( + PatchRouletteApply.PatchSelectionStrategy.NumericInPackage(2), + mockAvailablePatches(), + listOf( + listOf("io/papermc/paper/block/Block.java", "io/papermc/paper/block/BlockData.java"), + listOf("io/papermc/paper/block/BlockState.java"), + listOf("io/papermc/paper/entity/Entity.java") + ) + ), + Arguments.of( + PatchRouletteApply.PatchSelectionStrategy.NumericInPackage(5), + mockAvailablePatches(), + listOf( + listOf("io/papermc/paper/block/Block.java", "io/papermc/paper/block/BlockData.java", "io/papermc/paper/block/BlockState.java"), + listOf("io/papermc/paper/entity/Entity.java") + ) + ), + Arguments.of( + PatchRouletteApply.PatchSelectionStrategy.NumericInPackage(2, true), + mockAvailablePatches(), + listOf( + listOf("io/papermc/paper/block/Block.java", "io/papermc/paper/block/BlockData.java"), + listOf("io/papermc/paper/block/BlockState.java", "io/papermc/paper/entity/Entity.java"), + ) + ), + Arguments.of( + PatchRouletteApply.PatchSelectionStrategy.NumericInPackage(5, true), + mockAvailablePatches(), + listOf( + listOf( + "io/papermc/paper/block/Block.java", + "io/papermc/paper/block/BlockData.java", + "io/papermc/paper/block/BlockState.java", + "io/papermc/paper/entity/Entity.java" + ), + ) + ), + ) + + fun mockAvailablePatches() = listOf( + "io/papermc/paper/block/Block.java", + "io/papermc/paper/block/BlockData.java", + "io/papermc/paper/block/BlockState.java", + "io/papermc/paper/entity/Entity.java" + ) + } +} From 30ed8d449f5e9bb232dd9dd6673433e7514d2525 Mon Sep 17 00:00:00 2001 From: Bjarne Koll Date: Tue, 29 Apr 2025 16:19:40 +0200 Subject: [PATCH 2/4] Revert local testing diff --- .../core/tasks/patchroulette/AbstractPatchRouletteTask.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/paperweight-core/src/main/kotlin/io/papermc/paperweight/core/tasks/patchroulette/AbstractPatchRouletteTask.kt b/paperweight-core/src/main/kotlin/io/papermc/paperweight/core/tasks/patchroulette/AbstractPatchRouletteTask.kt index d54a3ff87..abcd8a9f2 100644 --- a/paperweight-core/src/main/kotlin/io/papermc/paperweight/core/tasks/patchroulette/AbstractPatchRouletteTask.kt +++ b/paperweight-core/src/main/kotlin/io/papermc/paperweight/core/tasks/patchroulette/AbstractPatchRouletteTask.kt @@ -54,7 +54,7 @@ abstract class AbstractPatchRouletteTask : BaseTask() { override fun init() { super.init() - endpoint.convention("http://localhost:8080/api") + endpoint.convention("https://patch-roulette.papermc.io/api") authToken.convention(providers.gradleProperty("paperweight.patch-roulette-token")) doNotTrackState("Run when requested") } From 1b212f5fca6ca5e3262f5bbeed8f10d7ff1e9fe6 Mon Sep 17 00:00:00 2001 From: Bjarne Koll Date: Tue, 29 Apr 2025 19:42:45 +0200 Subject: [PATCH 3/4] Correctly pass down smaller selection --- .../tasks/patchroulette/PatchRouletteApply.kt | 9 ++++++-- .../patchroulette/PatchRouletteApplyTest.kt | 23 +++++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/paperweight-core/src/main/kotlin/io/papermc/paperweight/core/tasks/patchroulette/PatchRouletteApply.kt b/paperweight-core/src/main/kotlin/io/papermc/paperweight/core/tasks/patchroulette/PatchRouletteApply.kt index 856a0c1f0..44a0ff7b2 100644 --- a/paperweight-core/src/main/kotlin/io/papermc/paperweight/core/tasks/patchroulette/PatchRouletteApply.kt +++ b/paperweight-core/src/main/kotlin/io/papermc/paperweight/core/tasks/patchroulette/PatchRouletteApply.kt @@ -168,6 +168,10 @@ abstract class PatchRouletteApply : AbstractPatchRouletteTask() { sealed interface PatchSelectionStrategy { data class NumericInPackage(val count: Int, val enforceCount: Boolean = false) : PatchSelectionStrategy { override fun select(config: Config, available: List): Pair> { + return this.select(config, available, this.count) + } + + fun select(config: Config, available: List, count: Int): Pair> { if (config.suggestedPackage != null) { val possiblePatches = available.filter { it.parent.equals(config.suggestedPackage) }.take(count) if (possiblePatches.isNotEmpty()) { @@ -180,14 +184,15 @@ abstract class PatchRouletteApply : AbstractPatchRouletteTask() { // config, as the last suggested package is the one to suggest in potentially next runs. val additionalPatches = select( config.copy(suggestedPackage = null), - available.filter { !possiblePatches.contains(it) } + available.filter { !possiblePatches.contains(it) }, + count - possiblePatches.size ) return additionalPatches.first to possiblePatches + additionalPatches.second } } - return select(config.copy(suggestedPackage = available.first().parent), available) + return select(config.copy(suggestedPackage = available.first().parent), available, count) } } diff --git a/paperweight-core/src/test/kotlin/io/papermc/paperweight/core/tasks/patchroulette/PatchRouletteApplyTest.kt b/paperweight-core/src/test/kotlin/io/papermc/paperweight/core/tasks/patchroulette/PatchRouletteApplyTest.kt index bd1b56bdf..dca09e16e 100644 --- a/paperweight-core/src/test/kotlin/io/papermc/paperweight/core/tasks/patchroulette/PatchRouletteApplyTest.kt +++ b/paperweight-core/src/test/kotlin/io/papermc/paperweight/core/tasks/patchroulette/PatchRouletteApplyTest.kt @@ -115,6 +115,29 @@ class PatchRouletteApplyTest { ), ) ), + Arguments.of( + PatchRouletteApply.PatchSelectionStrategy.NumericInPackage(4, true), + listOf( + "io/papermc/paper/block/Block.java", + "io/papermc/paper/block/BlockData.java", + "io/papermc/paper/block/BlockState.java", + "io/papermc/paper/entity/Entity.java", + "io/papermc/paper/entity/Entity2.java", + "io/papermc/paper/entity/Entity3.java" + ), + listOf( + listOf( + "io/papermc/paper/block/Block.java", + "io/papermc/paper/block/BlockData.java", + "io/papermc/paper/block/BlockState.java", + "io/papermc/paper/entity/Entity.java", + ), + listOf( + "io/papermc/paper/entity/Entity2.java", + "io/papermc/paper/entity/Entity3.java" + ) + ) + ) ) fun mockAvailablePatches() = listOf( From 9588a40e6458fd0dce254b1b3475c3d6bf244e92 Mon Sep 17 00:00:00 2001 From: Bjarne Koll Date: Sun, 18 May 2025 20:25:56 +0200 Subject: [PATCH 4/4] Comments --- .../core/tasks/patchroulette/PatchRouletteApply.kt | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/paperweight-core/src/main/kotlin/io/papermc/paperweight/core/tasks/patchroulette/PatchRouletteApply.kt b/paperweight-core/src/main/kotlin/io/papermc/paperweight/core/tasks/patchroulette/PatchRouletteApply.kt index 44a0ff7b2..a86b069b7 100644 --- a/paperweight-core/src/main/kotlin/io/papermc/paperweight/core/tasks/patchroulette/PatchRouletteApply.kt +++ b/paperweight-core/src/main/kotlin/io/papermc/paperweight/core/tasks/patchroulette/PatchRouletteApply.kt @@ -38,6 +38,19 @@ import org.gradle.api.tasks.OutputDirectory import org.gradle.api.tasks.OutputFile import org.gradle.api.tasks.options.Option +/** + * Patch roulette apply allows selection a set of patches from the remote patch roulette instance to work on. + * To control the amount/strategy of selecting these patches, the `--select` option can be passed. + * The following options are available: + * - `n`: Any positive integer number. + * Paperweight will select *up to* `n` patches from the current package the user is working in. + * If the package offers 0 patches, a new package will be chosen. + * If the package offers `m` patches, and `m < n`, only `m` patches will be returned. + * - `n!`: Any positive integer number followed by a `!`. + * Paperweight will select `n` patches, prioritizing patches in the current package. + * The only time less than `n` patches will be selected is if the entire patch roulette + * instance has less than `n` patches available, in which case all of them will be selected. + */ abstract class PatchRouletteApply : AbstractPatchRouletteTask() { @get:InputDirectory