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.09k stars 339 forks source link

Fixes `CommitEdit` issues when enabling ConfigurationCache #1086

Closed gmazzo closed 1 year ago

gmazzo commented 1 year ago

Follow up PR of https://github.com/Triple-T/gradle-play-publisher/pull/1084 partially addressing #1083

In order to work con Gradle 8.1.1 Configuration Cache issues, we need to bump Gradle, AGP to 8.0.0 and set the minimum JDK to 11 (17 for running the tests). All these changes were done in the first commit.

Focussing exclusively on CommitEdit task, I've made some changes as @ZacSweers suggested but still I didn't manage to get it to work as onlyIf seems to be computed at configuration time (asked in Slack if this is expected, probably will fill an issue)

Said that, there are still many tests broken if we enable CC for the whole test suite, so being fully compatible with CC is going to be a more complex task. If any is eager to contribute, I've added a withConfigurationCache property to IntegrationTestBase's constructor so we can start fixing them incrementally:

class CommitEditIntegrationTest : IntegrationTestBase(withConfigurationCache = true), SharedIntegrationTest

Once the migration is done, we can just remove this flag and turn it of by adding the properties into play/plugin/src/test/fixtures/app/gradle.properties:

org.gradle.configuration-cache=true

Here is the list of failing tests after this PR (if CC is enabled for all of them): image

SUPERCILEX commented 1 year ago

Taking a step back, I think we need a PR focussed on just upgrading dependencies/compat with AGP 8 etc. And then we can have PRs for the test changes + CC fixes.

SUPERCILEX commented 1 year ago

I think we need a PR focussed on just upgrading dependencies/compat with AGP 8 etc.

Can we still do this? Or am I misunderstanding and upgrading requires fixing the CC issue?

gmazzo commented 1 year ago

I think we need a PR focussed on just upgrading dependencies/compat with AGP 8 etc.

Can we still do this? Or am I misunderstanding and upgrading requires fixing the CC issue?

I have split already in two commits targeting this, I'll open another PR with the first one, yes

gmazzo commented 1 year ago

I think we need a PR focussed on just upgrading dependencies/compat with AGP 8 etc.

Can we still do this? Or am I misunderstanding and upgrading requires fixing the CC issue?

PR: https://github.com/Triple-T/gradle-play-publisher/pull/1087

SUPERCILEX commented 1 year ago

@ZacSweers Any opinions?

ZacSweers commented 1 year ago

I must confess this feels like a roundabout solution and likely to have issues in the future. Tasks shouldn't run based on the status of the entire build, they should usually just run with either explicit task dependencies or finalizedBy on the relevant tasks. Either that or it should probably be a separate, non-automatic gradle invocation just running that task. I get the appeal of convenience, but correctness is what ultimately matters and this is working against how gradle tries to manage tasks IMO.

SUPERCILEX commented 1 year ago

Would it feel more correct if we always ran the task and then just didn't do anything when the build fails? If yes, that's exactly what onlyIf is doing from the docs:

The closure will be evaluated at task execution time, not during configuration.

If no, then I don't know how else you'd avoid committing when one of the publishing tasks fails.

ZacSweers commented 1 year ago

I don't think a task should know anything about the build's failure. I would make the commit task explicitly depend on all publishing tasks (even if it doesn't use their outputs), that way it wouldn't run if one fails

SUPERCILEX commented 1 year ago

Then how does a user only run certain publishing tasks? That solution would build an apk, a bundle, the listing, products, etc. What would be nice is if each publishing type could have its own commit, but that's not possible due the play API not allowing concurrent edits.

ZacSweers commented 1 year ago

Hmm, maybe take a look at what this plugin does? https://github.com/vanniktech/gradle-maven-publish-plugin

It creates a repository on sonatype and then publishes artifacts to that repository, then finally closes+releases it at the end of the build. Maybe that's similar? I believe it uses a build service somewhere too

SUPERCILEX commented 1 year ago

Took a cursory look and I don't think they have to deal with the "subsets of tasks can contribute to closing/committing" problem.

I think this works for now.

gmazzo commented 1 year ago

subsets of tasks can contribute to closing/committing

Exactly. I also check with @Vampire on Gradle's Slack and he also advised me to do something like this. I'm wondering if we should do a Feature Request on Gradle to extend the API to either have a finalizedByIfSucceed method or some kind of CC compatible Provider<TaskOutcome> that we can use with onlyIf.