flutter / flutter

Flutter makes it easy and fast to build beautiful apps for mobile and beyond
https://flutter.dev
BSD 3-Clause "New" or "Revised" License
166.03k stars 27.42k forks source link

Comparison method violates its general contract! error across many Android tests. #103230

Open keyonghan opened 2 years ago

keyonghan commented 2 years ago

Current status: https://github.com/flutter/flutter/issues/103230#issuecomment-1447020884

The root of this issue is Groovy dynamic dispatch.

The flutter teams current plan for this bug is to migrate all of our groovy code to kotlin as defined in https://github.com/flutter/flutter/issues/121541 which has the added benefit of removing a language from the set maintainers need to know. This is aligned with the android teams recommendations.

This bug might be solved by removing the groovy dynamic dispatch called code in https://github.com/flutter/flutter/pull/114660. This pr was supposed to do exactly that but instead converted the code to kotlin and still uses groovy dynamic dispatch.

Another possible work around without migrating code to kotlin is to use compileStatic which converts the groovy code to bytecode and restricts the use of incompatible code.

(previously https://github.com/flutter/flutter/issues/103230#issuecomment-1125569034, https://github.com/flutter/flutter/issues/103230#issuecomment-1123007313 )


(part of https://github.com/flutter/flutter/issues/97036)

Two flakes in recent 100 commits: https://ci.chromium.org/ui/p/flutter/builders/prod/Windows%20build_tests_2_3/4557/overview https://ci.chromium.org/ui/p/flutter/builders/prod/Windows%20build_tests_2_3/4552/overview

 > Configure project :
[        ] Evaluating root project 'android' using build file 'C:\b\s\w\ir\x\w\flutter\examples\hello_world\android\build.gradle'.
[        ] Loaded lock state for configuration ':classpath'
[+1604 ms] > Configure project :app
[        ] Evaluating project ':app' using build file 'C:\b\s\w\ir\x\w\flutter\examples\hello_world\android\app\build.gradle'.
[        ] Creating configuration androidTestUtil
[   +1 ms] FAILURE: Build failed with an exception.
[   +2 ms] * Where:
[        ] Script 'C:\b\s\w\ir\x\w\flutter\packages\flutter_tools\gradle\flutter.gradle' line: 293
[        ] * What went wrong:
[        ] A problem occurred evaluating script.
[        ] > Failed to apply plugin class 'FlutterPlugin'.
[        ]    > Comparison method violates its general contract!
[        ] * Try:
[        ] Run with --debug option to get more log output. Run with --scan to get full insights.
[        ] * Exception is:
[        ] org.gradle.api.GradleScriptException: A problem occurred evaluating script.
[        ]  at org.gradle.groovy.scripts.internal.DefaultScriptRunnerFactory$ScriptRunnerImpl.run(DefaultScriptRunnerFactory.java:93)
[        ]  at org.gradle.configuration.DefaultScriptPluginFactory$ScriptPluginImpl.lambda$apply$0(DefaultScriptPluginFactory.java:133)
[        ]  at org.gradle.configuration.DefaultScriptTarget.addConfiguration(DefaultScriptTarget.java:74)
[        ]  at org.gradle.configuration.DefaultScriptPluginFactory$ScriptPluginImpl.apply(DefaultScriptPluginFactory.java:136)
[        ]  at org.gradle.configuration.BuildOperationScriptPlugin$1.run(BuildOperationScriptPlugin.java:65)
[        ]  at org.gradle.internal.operations.DefaultBuildOperationRunner$1.execute(DefaultBuildOperationRunner.java:29)
[        ]  at org.gradle.internal.operations.DefaultBuildOperationRunner$1.execute(DefaultBuildOperationRunner.java:26)
[        ]  at org.gradle.internal.operations.DefaultBuildOperationRunner$3.execute(DefaultBuildOperationRunner.java:75)
[        ]  at org.gradle.internal.operations.DefaultBuildOperationRunner$3.execute(DefaultBuildOperationRunner.java:68)
[        ]  at org.gradle.internal.operations.DefaultBuildOperationRunner.execute(DefaultBuildOperationRunner.java:153)
[        ]  at org.gradle.internal.operations.DefaultBuildOperationRunner.execute(DefaultBuildOperationRunner.java:68)
[        ]  at org.gradle.internal.operations.DefaultBuildOperationRunner.run(DefaultBuildOperationRunner.java:56)
[        ]  at org.gradle.internal.operations.DefaultBuildOperationExecutor.lambda$run$1(DefaultBuildOperationExecutor.java:71)
[        ]  at org.gradle.internal.operations.UnmanagedBuildOperationWrapper.runWithUnmanagedSupport(UnmanagedBuildOperationWrapper.java:45)
[        ]  at org.gradle.internal.operations.DefaultBuildOperationExecutor.run(DefaultBuildOperationExecutor.java:71)
[        ]  at org.gradle.configuration.BuildOperationScriptPlugin.lambda$apply$0(BuildOperationScriptPlugin.java:62)
[        ]  at org.gradle.configuration.internal.DefaultUserCodeApplicationContext.apply(DefaultUserCodeApplicationContext.java:43)
[        ]  at org.gradle.configuration.BuildOperationScriptPlugin.apply(BuildOperationScriptPlugin.java:62)
[        ]  at org.gradle.api.internal.plugins.DefaultObjectConfigurationAction.applyScript(DefaultObjectConfigurationAction.java:149)
[        ]  at org.gradle.api.internal.plugins.DefaultObjectConfigurationAction.access$000(DefaultObjectConfigurationAction.java:42)
[        ]  at org.gradle.api.internal.plugins.DefaultObjectConfigurationAction$1.run(DefaultObjectConfigurationAction.java:75)
[        ]  at org.gradle.api.internal.plugins.DefaultObjectConfigurationAction.execute(DefaultObjectConfigurationAction.java:183)
[        ]  at org.gradle.api.internal.project.AbstractPluginAware.apply(AbstractPluginAware.java:49)
[        ]  at org.gradle.api.internal.project.ProjectScript.apply(ProjectScript.java:37)
[        ]  at org.gradle.api.Script$apply.callCurrent(Unknown Source)
zanderso commented 2 years ago

@blasten In https://github.com/flutter/flutter/issues/96784, you mentioned adding code to log a stack trace. Does this help?

blasten commented 2 years ago

This issue is coming from https://github.com/flutter/flutter/blob/a9ac7fb03be1d574d726e98b90743015716de7fd/packages/flutter_tools/gradle/flutter.gradle#L293.

I asked on the Android Gradle chat to see if there's a fix for this specific issue. For now, the lowest hanging fruit is to upgrade Gradle and the Android plugin to see if that fixes this issue as well. https://github.com/flutter/flutter/issues/102183.

cc @GaryQian

blasten commented 2 years ago

From the AGP team:

it looks like a bug in Groovy's dynamic dispatch. If you invoke the method from something statically compiled (Java or Kotlin) you should be fine (or even just use @CompileStatic if you don't want to convert)

For context, we got rid of all groovy production code in AGP many years ago for performance and correctness reasons

keyonghan commented 2 years ago

This has caused multiple builders to be flaky with high ratios today. 3 flakes on Windows tool_integration_tests_1_6 This also cause 1 flake on Windows build_tests_1_3 1 flake on Windows tool_integration_tests_2_6

Also considering the above referenced flaky bugs, I am bumping the priority to P1.

christopherfujino commented 2 years ago

Sounds like the best plan we have is to apply https://github.com/flutter/flutter/issues/102183?

christopherfujino commented 2 years ago

I raised the priority of https://github.com/flutter/flutter/issues/102183 to p1, as I believe that is the root cause here.

GaryQian commented 2 years ago

The tests seem to be stable over past 200 runs, with one failure. This puts it well under threshold.

zanderso commented 2 years ago

We downgraded this issue because the specific test dropped below the threshold, but it looks like this might be a systemic issue causing flakes across many different tests, which will cause different tests at different times to jump above the threshold. See for example https://github.com/flutter/flutter/issues/107549.

Because of that, I'm going to upgrade this to P1 again, deduplicate other instances against this one, and re-title this issue to be about the general problem rather than a specific test.

zanderso commented 2 years ago

Another example https://github.com/flutter/flutter/issues/107546

GaryQian commented 2 years ago

I have not had the bandwidth to look at this in the past week, but will try to get to it this week.

stuartmorgan commented 2 years ago

https://github.com/flutter/flutter/issues/108020 is another issue to look for to re-enable tests after this is fixed.

GaryQian commented 2 years ago

To efficiently bump gradle versions (and sync other things to modern versions), https://github.com/flutter/flutter/pull/107665 and https://github.com/flutter/flutter/pull/108146 need to land to have operational migrate tool.

zanderso commented 2 years ago

Is there any way to not gate this on the migrate command? It's going to take me awhile to review those PRs.

GaryQian commented 2 years ago

I could run this with a local version of the command, or bump gradle versions manually. I believe Emmanuel tried to do so before, but was blocked on test failures and had to roll back. Let me find the PR.

GaryQian commented 2 years ago

https://github.com/flutter/flutter/pull/108197 Manually performs upgrades of gradle across a subset of projects.

GaryQian commented 2 years ago

https://github.com/flutter/flutter/pull/108355 relands after revert.

christopherfujino commented 2 years ago

~https://github.com/flutter/flutter/pull/108355 landed July 26 4:23 PM PDT. When the flaky bot comments next week on the particular shard bugs, we should verify if there any instances of this flake after that commit.~

Never mind, it was reverted https://github.com/flutter/flutter/pull/108407

GaryQian commented 2 years ago

https://github.com/flutter/flutter/pull/108510 landed again, this time it seems to have stuck. :)

There should be other sample apps/test apps that can also be version bumped as https://github.com/flutter/flutter/pull/108510 only upgraded a subset

christopherfujino commented 2 years ago

Another instance https://ci.chromium.org/ui/p/flutter/builders/prod/Windows%20build_tests_1_3/5902/overview at commit https://github.com/flutter/flutter/commit/3ce2ab91598819cc31e614debf357b1c0859f56e, merged after https://github.com/flutter/flutter/pull/108510

zanderso commented 2 years ago

Still getting reports of this from the flaky bot: https://github.com/flutter/flutter/issues/108890

GaryQian commented 2 years ago

https://github.com/flutter/flutter/pull/109211 Updates AGP used in flutter.gradle

Hixie commented 2 years ago

I saw this one in CI on my PR too.

GaryQian commented 2 years ago

This issue doesn't seem to have triggered the threshold for 20+ days, though when it is actively flaking, it seems to flake much more consistently. Continuing to monitor.

GaryQian commented 2 years ago

Reducing priority as we are no longer actively seeing this

zanderso commented 2 years ago

Saw this on Engine -> Framework roll presubmits here https://ci.chromium.org/ui/p/flutter/builders/try/Windows%20tool_integration_tests_1_6/7443/overview

christopherfujino commented 2 years ago

Saw this on Windows module_custom_host_app_name_test

keyonghan commented 2 years ago

Bumping the priority to P1 considering recent high flakes: Windows build_tests_2_3 is 3.13% flaky Windows build_tests_1_3 is 2.08% flaky Windows module_custom_host_app_name_test is 3.13% flaky

GaryQian commented 2 years ago

Landed https://github.com/flutter/flutter/pull/111747 to see if it helps

GaryQian commented 2 years ago

https://github.com/flutter/flutter/pull/113845 Removed the map literal, waiting to observe if the flake is still present.

GerogeLeon commented 2 years ago

I upgrade flutter to 3.3.5 and it incurred this issue. To avoid it, I tried to downgrade to 3.3.4 , and it worked

GaryQian commented 2 years ago

3.3.5 vs 3.3.4 should not have made a difference as this issue predates both those versions by quite a bit. https://github.com/flutter/flutter/pull/113845 made it into 3.3.6, could you try to see if you encounter it on 3.3.6?

christopherfujino commented 2 years ago

Just hit this on framework post-submit https://ci.chromium.org/p/flutter/builders/prod/Windows%20build_tests_2_3/7564

GaryQian commented 1 year ago

Converting this section of the code to use the Kotlin DSL should work around the problem as well as move towards using kotlin as recommended by Android.

reidbaker commented 1 year ago

Gary is on vacation but when you return can you give us some guidance around what the next steps should be? @GaryQian

ricardoamador commented 1 year ago

Updating that this is still an issue and has been happening relatively frequently. https://ci.chromium.org/ui/p/flutter/builders/prod/Windows%20build_tests_2_3/8142/overview https://ci.chromium.org/ui/p/flutter/builders/prod/Windows%20build_tests_2_3/8230/overview https://ci.chromium.org/ui/p/flutter/builders/prod/Windows%20build_tests_2_3/8233/overview

@GaryQian is there a fix planned for this?

reidbaker commented 1 year ago

@GaryQian and I believe that this has slightly improved and has taken a significant amount of time (40+ engineering hours) and he is out of ideas for the remaining windows flakes.

Our current plan is to drop the priority of this bug given the stability on linux, that if this test fails a re-run succeeds so it does not block the tree, and it is set to automatically rerun. Basically the cost to the team for continuing to make unknown progress doesnt match the value. If you disagree please let me know.

reidbaker commented 1 year ago

We expect that the bots will continue to escalate flakes here but there no way to disable this specific line as it is used in all android builds. Please keep attaching issues to this ticket even though we will not make any progress on this in 22q4 or 23q1. Our hope is that is magically fixed by updating the versions of gradle but have no evidence to say that will work.

ricardoamador commented 1 year ago

Yup no problem. Will keep an eye out for future failures.

gnprice commented 1 year ago

I filed #121541 to track following the AGP team's broader recommendation (at https://github.com/flutter/flutter/issues/103230#issuecomment-1125569034) by migrating Flutter's whole Gradle plugin to Kotlin.

In #114660 this specific piece of code was migrated to Kotlin, but in a form that still uses Groovy dynamic dispatch:

        // Use withGroovyBuilder and getProperty() to access Groovy metaprogramming.
        project.withGroovyBuilder {
            getProperty("android").withGroovyBuilder {

That seems not to have fixed this issue. Probably it could be fixed by converting that code to use static dispatch. But it may be that that's possible only by someone sitting down and really learning how to write in the Gradle Kotlin DSL. That's likely to be a large fraction of the total work required for a full migration to Kotlin.

flutter-triage-bot[bot] commented 1 year ago

This issue is assigned to @reidbaker but has had no recent status updates. Please consider unassigning this issue if it is not going to be addressed in the near future. This allows people to have a clearer picture of what work is actually planned. Thanks!

flutter-triage-bot[bot] commented 10 months ago

This issue is assigned to @reidbaker but has had no recent status updates. Please consider unassigning this issue if it is not going to be addressed in the near future. This allows people to have a clearer picture of what work is actually planned. Thanks!

flutter-triage-bot[bot] commented 8 months ago

This issue was assigned to @reidbaker but has had no status updates in a long time. To remove any ambiguity about whether the issue is being worked on, the assignee was removed.