Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade AGP to 7.3.0, Gradle to 7.6 #1075

Merged
merged 11 commits into from
Feb 1, 2023
Merged

Upgrade AGP to 7.3.0, Gradle to 7.6 #1075

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
a5442a3
Update Gradle to 7.6
scana Jan 24, 2023
35bf66a
Update AGP to 7.3.0
scana Jan 24, 2023
f230f4d
Adapt DefaultPlayPublisher to newer API
scana Jan 24, 2023
288418f
Temporarily ignore the test
scana Jan 24, 2023
378f45c
Update using `gradle wrapper --gradle-version 7.6` command
scana Jan 25, 2023
dbcd5a2
Check for both directories and filenames
scana Jan 26, 2023
5084bd9
Create 3 test cases for 'play'
scana Jan 31, 2023
f2e9e42
Cover new test cases
scana Jan 31, 2023
68f683d
Fix deprecations in settings.gradle.kts
SUPERCILEX Feb 1, 2023
adca7c4
Tidy
SUPERCILEX Feb 1, 2023
048bb6e
Upgrade testapp
SUPERCILEX Feb 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion gradle/wrapper/gradle-wrapper.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-7.2-all.zip
distributionUrl=https\://services.gradle.org/distributions/gradle-7.6-all.zip
scana marked this conversation as resolved.
Show resolved Hide resolved
zipStoreBase=GRADLE_USER_HOME
zipStorePath=wrapper/dists
2 changes: 2 additions & 0 deletions ...c/main/kotlin/com/github/triplet/gradle/androidpublisher/internal/DefaultPlayPublisher.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,8 @@ internal class DefaultPlayPublisher(
println("Uploading $thing: ${(it.progress * 100).roundToInt()}% complete")
MediaHttpUploader.UploadState.MEDIA_COMPLETE ->
println("${thing.capitalize()} upload complete")
MediaHttpUploader.UploadState.NOT_STARTED,
MediaHttpUploader.UploadState.INITIATION_COMPLETE -> {}
}
}
return this
Expand Down
2 changes: 2 additions & 0 deletions .../src/test/kotlin/com/github/triplet/gradle/play/tasks/GenerateResourcesIntegrationTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import org.gradle.testkit.runner.TaskOutcome.FROM_CACHE
import org.gradle.testkit.runner.TaskOutcome.NO_SOURCE
import org.gradle.testkit.runner.TaskOutcome.SUCCESS
import org.gradle.testkit.runner.TaskOutcome.UP_TO_DATE
import org.junit.Ignore
import org.junit.jupiter.api.Test
import java.io.File

Expand Down Expand Up @@ -380,6 +381,7 @@ class GenerateResourcesIntegrationTest : IntegrationTestBase(), SharedIntegratio
}

@Test
@Ignore("Previously this test worked because empty catalogs were generated and passed into the task")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be verified if this test/implementation for it should be rewritten

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check for this is here:

gradle-play-publisher/play/plugin/src/main/kotlin/com/github/triplet/gradle/play/tasks/GenerateResources.kt

Lines 102 to 107 in 4273731

val isPlayKeywordReserved = name != PLAY_PATH || parameters.inputDirs.get().any {
it.asFile == this
}
check(isPlayKeywordReserved) {
"The file name 'play' is illegal: $this"
}

This test absolutely has to pass because we rely on it for correctness. The way we pick which resource to use for a particular flavor is dependent on the parent of the play directory being a valid flavor name:

gradle-play-publisher/play/plugin/src/main/kotlin/com/github/triplet/gradle/play/tasks/GenerateResources.kt

Line 449 in 4273731

val flavor = checkNotNull(f.climbUpTo(PLAY_PATH)?.parentFile?.name) {

Anyway, off the top of my head I think you can fix name != PLAY_PATH by combining two climbUpTos and if both of them are non-null then you have two play directories. There might be a cleaner way though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SUPERCILEX could you please elaborate more on what this test is supposed to verify? 🙏

Looking at those two it seems like those test are identical except for different flavor name - it says "play file name is not allowed", but I can't grasp my head around it - it sounds like only "play" flavor is allowed and others are not (which is unexpected I guess)?

    @Test
    fun `Flavor named 'play' is allowed`() {
        // language=gradle
        val config = """
            flavorDimensions 'pricing'
            productFlavors {
                play { dimension 'pricing' }
            }
        """.withAndroidBlock()

        val result = execute(config, "generatePlayReleasePlayResources")

        result.requireTask(taskName("PlayRelease"), outcome = SUCCESS)
    }

    @Test
    @Ignore("Previously this test worked because empty catalogs were generated and passed into the task")
    fun `'play' file name is not allowed`() {
        // language=gradle
        val config = """
            flavorDimensions 'pricing'
            productFlavors {
                illegalPlay { dimension 'pricing' }
            }
        """.withAndroidBlock()

        val result = executeExpectingFailure(
                config, "generateIllegalPlayReleasePlayResources")

        result.requireTask(taskName("IllegalPlayRelease"), outcome = FAILED)
    }

Please read my PR's description on how now parameters provided to

abstract class Validator : WorkAction<Validator.Params> {
        override fun execute() {
            for (file in parameters.files.get()) {
                file.validate()
            }
        }

have changed with the recent version.

I'm happy to improve it but would really appreciate your explanation in here 😄

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shoot, forgot about that, makes life more complicated. For understanding the tests, it's easiest to look at the fixtures: https://github.com/Triple-T/gradle-play-publisher/tree/master/play/plugin/src/test/fixtures/GenerateResourcesIntegrationTest/src. Every directory in there is a flavor, so you'll find the play and illegalPlay dirs in there.

I've looked at this some more and I think we might be able to fix this by replacing it.asFile == this with it.asFile == climbUpTo(PLAY_PATH). That check is looking for file names that are play, but not part of the root input dirs. So if we look through every input file and find its parent play dir, it should always be a root play dir (I think).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SUPERCILEX please take a look at dbcd5a2.

My confusion came from this test's name being a little bit misleading I think.

From what I understand now is that those two cases are illegal:

  • One is not allowed to have any files named "play"
    project/my-project/src/flavorName/play/**/play.txt

  • One is not allowed to have any child directories named play - only root directory can have "play" name:
    project/my-project/src/flavorName/play/**/play/*.*

Even if the flavor is named play, this would be the root directory: src/play/play - my changes made it look only at the child directories of the subdirectory.
So src/play/play/default-listing.txt is ok, src/play/play/**/play/file.txt is not.

I tested this change with both old Gradle version and new one and in both cases it now passes.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One is not allowed to have any files named "play"
project/my-project/src/flavorName/play/**/play.txt

Yes to the sentence, no to the example. /play.txt should be fine, /play isn't. On that note, can you add two test cases for this: one allowing play.foo and one disallowing plain play files? Then check that the tests pass pre-PR and we'll be in the clear for behavior changes.

I like the simplicity of your approach, but I'm a little bothered by the performance implications. Previously, we only did validation on changed files (since the input files are incremental). Now, we always run a full directory expansion. I think just because of that'd I'd still prefer the "climb up every file's path and see if its root play dir is contained in the input dir set" approach.

Copy link
Contributor Author

@scana scana Jan 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you - I was so focused on 'file' that I forgot that you could actually have a file play without an extension.

Got actually a little bit misled because GenerateResourcesIntegrationTest fixture does not contain a file named play but only a directory named play (listings/en-US/play/.keep).
This is the reason why the test would start to fail, as there was no file play in the input anymore with newer Gradle version (previously there was a a list of files and also directories, which were then treated asFile). .keep got ignored.

src/main/play/default-language.txt
src/main/play/listings/de-DE/full-description.txt
src/main/play/listings/en-US/full-description.txt
src/main/play/listings/en-US/graphics/phone-screenshots/foo.jpg
src/main/play/listings/en-US/graphics/tablet-screenshots/baz.jpg
src/main/play/listings/en-US/short-description.txt
src/main/play/listings/en-US/title.txt
src/main/play/listings/fr-FR/full-description.txt
src/main/play/listings/fr-FR/graphics/phone-screenshots/bar.jpg
src/main/play/listings/fr-FR/short-description.txt
src/main/play/listings/fr-FR/title.txt
src/main/play/products/sku.json
src/main/play/release-notes/en-US/default.txt
src/main/play/release-notes/fr-FR/default.txt

One is not allowed to have any child directories named play - only root directory can have "play" name:

Is this also valid?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this also valid?

Sorry, which part? Or do you mean in general? If so, then yeah your understanding is correct.

fun `'play' file name is not allowed`() {
// language=gradle
val config = """
Expand Down
4 changes: 2 additions & 2 deletions settings.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ dependencyResolutionManagement {
.toPluginId("io.github.gradle-nexus.publish-plugin")
.versionRef("nexusPublish")

version("agp", "7.0.0")
version("agp-tools", "30.0.0")
version("agp", "7.3.0")
version("agp-tools", "30.3.1")
version("android-publisher", "v3-rev20211021-1.32.1")
version("api-client", "1.32.2")
version("http-client", "1.40.1")
scana marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
2 changes: 1 addition & 1 deletion testapp/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ buildscript {

dependencies {
classpath(kotlin("gradle-plugin", embeddedKotlinVersion))
classpath("com.android.tools.build:gradle:7.0.0")
classpath("com.android.tools.build:gradle:7.3.0")
classpath("com.supercilex.gradle:version-orchestrator:0.9.0")
classpath("com.github.triplet.gradle:play-publisher:" +
file("../version.txt").readText().trim())
Expand Down