search

Triple-T / gradle-play-publisher

GPP is Android's unofficial release automation Gradle Plugin. It can do anything from building, uploading, and then promoting your App Bundle or APK to publishing app listings and other metadata.
MIT License
4.13k stars 341 forks source link

Fixed issues with Configuration Cache #1084

Closed gmazzo closed 1 year ago

gmazzo commented 1 year ago

Fixes https://github.com/Triple-T/gradle-play-publisher/issues/1083 by removing the Task.project reference inside the onlyIf closure of CommitEdit

ZacSweers commented 1 year ago

Are you sure gradle is safe to access here? Seems like this should be wrapped in a provider instead. A test would help

gmazzo commented 1 year ago

Are you sure gradle is safe to access here? Seems like this should be wrapped in a provider instead. A test would help

Provider of what exactly? Provider<Gradle>?

I didn't add all my changes (like bumping Gradle to 8.1.1 and AGP to 8.0.0, enabling Configuration Cache (CC) or setting java.toolchain DSL to 8) to reduce the PR to the bare minimum, but the task CommitEdit is getting instantiated as part the existing integrations tests, so it does work. Which kind of extra test do you have in mind?

Regarding keeping a reference to the Gradle object, as far I know, is not a violation of CC requirements. At least is not being notified as that by Gradle as it does with Task.project.

ZacSweers commented 1 year ago

A provider of a Boolean (so compute the whole thing). A test that verifies this task does not break configuration cache (I.e. run it twice with the same inputs, confirm the second time reuses the cache).

gmazzo commented 1 year ago

A provider of a Boolean (so compute the whole thing). A test that verifies this task does not break configuration cache (I.e. run it twice with the same inputs, confirm the second time reuses the cache).

A few things: 1) Let's say I add a onlyIf(buildHasNotFailedProvider::get), where should I move the logic for computing its valid, to the plugin? I personally try to avoid self config blocks with init at all, as that logic should belong to the plugin. In this case, not sure if the refactor will be justified as (and if I understood the logic correct) this seems to be to abort the transaction on Google Play in case any other previous task has failed. If that's the case, probably this logic is a bit hacky and it should be replaced with a finalizedBy instead. I feel changing this will be a bit unrelated to the purpose of this PR, which is just make it compatible with CC.

2) CC is not exactly the same than Build Cache, and not so easily to test. CC will fail depending on the tasks that were effectively run. So to make sure the whole plugin is compatible with it, I'd just enable CC on the tests as well. I tried that, and it was working fine, but later the testapp uses another version-orchestrator plugin which is also incompatible with CC, making the test to fail. That was when I decided to limit the PR to the bare minimum to fix this issue.

3) Regarding this:

I.e. run it twice with the same inputs, confirm the second time reuses the cache

As far I know, and I may incorrect, there is no API (yet?) from GradleRunner tests to tell if CC was a hit or not. There is one for BuildCache from TaskOutput API, but no for this one. What kind of test would you have in mind?

I decided to work on this because it's actually blocking us from updating to Gradle 8.1.1 as it will break our release automation. Going deeper on the refactor, bumping/changing plugins because they are also incompatible may extend too much in time and potentially introduce new issues.

SUPERCILEX commented 1 year ago

This thing is a hack because CommitEdit is a finalizer and gradle has no API to detect whether or not the finalized tasks have failed. The goal is to prevent publishing if something went wrong.

It's a bummer if this break CC, but we should probably make the plugin work on Gradle 8.1, so gonna merge.

gmazzo commented 1 year ago

Thankyou!

JavierSegoviaCordoba commented 1 year ago

@SUPERCILEX there is a new scope APIs, if I remember correctly, FlowScope.

SUPERCILEX commented 1 year ago

@SUPERCILEX there is a new scope APIs, if I remember correctly, FlowScope.

If someone wants to give that a whirl, I'd be happy to accept PRs.

jrgonzalezg commented 1 year ago

This does not seem to work for me. The tasks still fails but now it has a different error message:

Task .... of type `com.github.triplet.gradle.play.tasks.CommitEdit`: cannot serialize object of type 'org.gradle.invocation.DefaultGradle', a subtype of 'org.gradle.api.invocation.Gradle', as these are not supported with the configuration cache. See https://docs.gradle.org/8.1.1/userguide/configuration_cache.html#config_cache:requirements:disallowed_types
gmazzo commented 1 year ago

Damn, I thought I tested that. If you are seeing that error message then it's not going to work.

I'm traveling this weekend. One workaround will be to turn CC of when with --no-configuration-cache you invoke this task

gmazzo commented 1 year ago

I can take a look at this early next week, this time with a test

ZacSweers commented 1 year ago

Are you sure gradle is safe to access here? Seems like this should be wrapped in a provider instead. A test would help

^^ I wasn't trying to be annoying, this is exactly why I suggested it.

gmazzo commented 1 year ago

Nothing to say. This is what I get for trying to patch things quickly.

I had the misconception that Gradle object would be quickly rehydratable from the cache.

So as you pointed out, moving it to a Provider<Boolean> will be at least safe to do.

I don't have access right now to work on this, but I can do it next week.

And add tests to make sure I won't ***up again😅