Identify and re-throw our dependency checking errors in `flutter.groovy` #149609

gmackall · 2024-06-03T19:32:30Z

The application of the dependency_version_checker gradle plugin is wrapped in a try catch, which prevented blocking the build in #149204. This pr re-throws the errors we intended to throw.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

reidbaker · 2024-06-03T19:46:25Z

packages/flutter_tools/gradle/src/main/groovy/flutter.groovy

                project.apply from: dependencyCheckerPluginPath
-            } catch (Exception ignored) {
+            } catch (Exception e) {
+                // If the exception was thrown by us in the dependency version checker plugin then


This is a bit gross as any change to that message would cause this unrelated code to fail.

Can you modify the exception to have a root class maybe and catch that root class and rethrow?

name idea: DependencyValidationException :P

Yeah, I agree it's pretty gross.

The reason I wrote it this way is that I thought at first that any exception class I wrote for use in the kotlin side (where we throw) wouldn't resolve in the groovy side (where we catch). But I think I'm mistaken about that, it just gets wrapped in a GradleException, so I'll still have to dig in like I am here, but won't have to rely on error message at least

Ok, this is still gross but hopefully less so.

I've made changes so that we have a DependencyValidationException defined in flutter.groovy, and then we throw that from within the kotlin file. We still catch broadly, because Gradle wraps our exception in a couple others, but now we test for that error class with instanceof.

…tify it

…_checking_errors_throwable' into make_our_dependency_checking_errors_throwable

reidbaker · 2024-06-04T14:29:15Z

packages/flutter_tools/gradle/src/main/groovy/flutter.groovy

                        "dependency_version_checker.gradle.kts")
                project.apply from: dependencyCheckerPluginPath
-            } catch (Exception ignored) {
+            } catch (Exception e) {


Why can't we do something like the suggestion below this comment with multiple catch blocks? Is is a quirk of throwing an exception from kotlin to groovy?

Suggested change

} catch (Exception e) {

} catch (DependencyValidationException e) {

// Rethrow

} catch (Exception e) {

// eat exception Do other code path.

}

Is is a quirk of throwing an exception from kotlin to groovy?

Yes, that is correct. The DependencyValidationException gets wrapped in a couple of GradleExceptions

I think when the Gradle plugin is fully rewritten to Kotlin, then we'll no longer have to depend on such hacks, right?

Correct. I also haven't looked too far into this, but I think when the full re-write happens it can also be done in actual Kotlin source, and not Kotlin script (I believe AGP is written this way https://android.googlesource.com/platform/tools/base/+/1839aa23b8dc562005e2f0f0cc8e8b4c5caa37d0/build-system/gradle-core/src/main/java/com/android/build/gradle/internal/utils/agpVersionChecker.kt)

Yes, that's my understanding as well (Kotlin "proper" instead of kts). Sweet!

…ter.groovy` (flutter/flutter#149609)

flutter/flutter@c246ecd...27e0656 2024-06-05 98614782+auto-submit[bot]@users.noreply.github.com Reverts "TreeSliver & associated classes (#147171)" (flutter/flutter#149754) 2024-06-05 [email protected] Feature: Add AnimatedList with separators (flutter/flutter#144899) 2024-06-05 [email protected] make output of flutter run web tests verbose (flutter/flutter#149694) 2024-06-05 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Request focus if `SemanticsAction.focus` is sent to a focusable widget (#142942)" (flutter/flutter#149741) 2024-06-05 [email protected] Roll Flutter Engine from e88469090fed to 11a32d43e3f6 (2 revisions) (flutter/flutter#149699) 2024-06-05 [email protected] Request focus if `SemanticsAction.focus` is sent to a focusable widget (flutter/flutter#142942) 2024-06-04 [email protected] Roll Flutter Engine from 3dd40156afb6 to e88469090fed (2 revisions) (flutter/flutter#149695) 2024-06-04 [email protected] TreeSliver & associated classes (flutter/flutter#147171) 2024-06-04 [email protected] Roll Flutter Engine from fe00f023666c to 3dd40156afb6 (3 revisions) (flutter/flutter#149692) 2024-06-04 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.25.7 to 3.25.8 (flutter/flutter#149691) 2024-06-04 [email protected] Prepares semantics_update_test for upcoming heading level changes (flutter/flutter#149671) 2024-06-04 [email protected] Identify and re-throw our dependency checking errors in `flutter.groovy` (flutter/flutter#149609) 2024-06-04 [email protected] Scrollbar thumb drag gestures now produce one start and one end scroll notification (flutter/flutter#146654) 2024-06-04 [email protected] Disable sandboxing for macOS apps and tests in CI (flutter/flutter#149618) 2024-06-04 [email protected] Roll Flutter Engine from e211c43f3dc1 to fe00f023666c (3 revisions) (flutter/flutter#149680) 2024-06-04 [email protected] Allow `find.byTooltip` to use a RegEx (flutter/flutter#149348) 2024-06-04 [email protected] Roll Flutter Engine from a6aa5d826649 to e211c43f3dc1 (8 revisions) (flutter/flutter#149658) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

gmackall · 2024-06-07T18:34:23Z

Reason for revert: Long spam in certain circumstances:

e: /Users/mackall/development/flutter/flutter/packages/flutter_tools/gradle/src/main/kotlin/dependency_version_checker.gradle.kts:238:23: Unresolved reference: DependencyValidationException
e: /Users/mackall/development/flutter/flutter/packages/flutter_tools/gradle/src/main/kotlin/dependency_version_checker.gradle.kts:263:23: Unresolved reference: DependencyValidationException
e: /Users/mackall/development/flutter/flutter/packages/flutter_tools/gradle/src/main/kotlin/dependency_version_checker.gradle.kts:288:23: Unresolved reference: DependencyValidationException
e: /Users/mackall/development/flutter/flutter/packages/flutter_tools/gradle/src/main/kotlin/dependency_version_checker.gradle.kts:313:23: Unresolved reference: DependencyValidationException
Warning: Flutter was unable to detect project Gradle, Java, AGP, and KGP versions. Skipping dependency version checking. Error was: org.gradle.internal.exceptions.LocationAwareException: Script '/Users/mackall/development/flutter/flutter/packages/flutter_tools/gradle/src/main/kotlin/dependency_version_checker.gradle.kts' line: 238
Script compilation errors:

  Line 238:                 throw DependencyValidationException(errorMessage)
                                  ^ Unresolved reference: DependencyValidationException

  Line 263:                 throw DependencyValidationException(errorMessage)
                                  ^ Unresolved reference: DependencyValidationException

  Line 288:                 throw DependencyValidationException(errorMessage)
                                  ^ Unresolved reference: DependencyValidationException

  Line 313:                 throw DependencyValidationException(errorMessage)
                                  ^ Unresolved reference: DependencyValidationException

auto-submit · 2024-06-07T18:35:05Z

Time to revert pull request flutter/flutter/149609 has elapsed.
You need to open the revert manually and process as a regular pull request.

…ter.groovy` (flutter#149609)" This reverts commit 9d1de7b.

…ter.groovy` (#149609)" (#149918) This reverts commit 9d1de7b. Reverts due to log spam and crashing of the dependency version checker (which doesn't block the build but still isn't the desired outcome).

…tter.groovy` (flutter#149609)" (flutter#149918) This reverts commit 15307a9.

…er.groovy" (#150128) The original approach in #149609 didn't work when the Flutter Gradle plugin was applied using the deprecated script apply - the kotlin portion couldn't resolve the custom exception defined in `flutter.groovy`: ``` e: /Users/mackall/development/flutter/flutter/packages/flutter_tools/gradle/src/main/kotlin/dependency_version_checker.gradle.kts:238:23: Unresolved reference: DependencyValidationException e: /Users/mackall/development/flutter/flutter/packages/flutter_tools/gradle/src/main/kotlin/dependency_version_checker.gradle.kts:263:23: Unresolved reference: DependencyValidationException e: /Users/mackall/development/flutter/flutter/packages/flutter_tools/gradle/src/main/kotlin/dependency_version_checker.gradle.kts:288:23: Unresolved reference: DependencyValidationException e: /Users/mackall/development/flutter/flutter/packages/flutter_tools/gradle/src/main/kotlin/dependency_version_checker.gradle.kts:313:23: Unresolved reference: DependencyValidationException Warning: Flutter was unable to detect project Gradle, Java, AGP, and KGP versions. Skipping dependency version checking. Error was: org.gradle.internal.exceptions.LocationAwareException: Script '/Users/mackall/development/flutter/flutter/packages/flutter_tools/gradle/src/main/kotlin/dependency_version_checker.gradle.kts' line: 238 Script compilation errors: Line 238: throw DependencyValidationException(errorMessage) ^ Unresolved reference: DependencyValidationException Line 263: throw DependencyValidationException(errorMessage) ^ Unresolved reference: DependencyValidationException Line 288: throw DependencyValidationException(errorMessage) ^ Unresolved reference: DependencyValidationException Line 313: throw DependencyValidationException(errorMessage) ^ Unresolved reference: DependencyValidationException ``` This new approach of setting one of the [`extra` properties](https://docs.gradle.org/current/dsl/org.gradle.api.Project.html#N14FF7) works in both cases (tested with the `camera_android` example app, which uses the script apply, and a freshly created counter app). It also removes some brittleness in that we don't have to unwrap the exception anymore, and aren't subject to breaking if Gradle decides one day to wrap our custom exception 1 layer deeper in additional exceptions.

…er.groovy" (flutter#150128) The original approach in flutter#149609 didn't work when the Flutter Gradle plugin was applied using the deprecated script apply - the kotlin portion couldn't resolve the custom exception defined in `flutter.groovy`: ``` e: /Users/mackall/development/flutter/flutter/packages/flutter_tools/gradle/src/main/kotlin/dependency_version_checker.gradle.kts:238:23: Unresolved reference: DependencyValidationException e: /Users/mackall/development/flutter/flutter/packages/flutter_tools/gradle/src/main/kotlin/dependency_version_checker.gradle.kts:263:23: Unresolved reference: DependencyValidationException e: /Users/mackall/development/flutter/flutter/packages/flutter_tools/gradle/src/main/kotlin/dependency_version_checker.gradle.kts:288:23: Unresolved reference: DependencyValidationException e: /Users/mackall/development/flutter/flutter/packages/flutter_tools/gradle/src/main/kotlin/dependency_version_checker.gradle.kts:313:23: Unresolved reference: DependencyValidationException Warning: Flutter was unable to detect project Gradle, Java, AGP, and KGP versions. Skipping dependency version checking. Error was: org.gradle.internal.exceptions.LocationAwareException: Script '/Users/mackall/development/flutter/flutter/packages/flutter_tools/gradle/src/main/kotlin/dependency_version_checker.gradle.kts' line: 238 Script compilation errors: Line 238: throw DependencyValidationException(errorMessage) ^ Unresolved reference: DependencyValidationException Line 263: throw DependencyValidationException(errorMessage) ^ Unresolved reference: DependencyValidationException Line 288: throw DependencyValidationException(errorMessage) ^ Unresolved reference: DependencyValidationException Line 313: throw DependencyValidationException(errorMessage) ^ Unresolved reference: DependencyValidationException ``` This new approach of setting one of the [`extra` properties](https://docs.gradle.org/current/dsl/org.gradle.api.Project.html#N14FF7) works in both cases (tested with the `camera_android` example app, which uses the script apply, and a freshly created counter app). It also removes some brittleness in that we don't have to unwrap the exception anymore, and aren't subject to breaking if Gradle decides one day to wrap our custom exception 1 layer deeper in additional exceptions.

…ter.groovy` (flutter/flutter#149609)

identify and re-throw our dependency checking errors

220e783

github-actions bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jun 3, 2024

gmackall changed the title ~~identify and re-throw our dependency checking errors~~ Identify and re-throw our dependency checking errors in flutter.groovy Jun 3, 2024

Merge branch 'master' into make_our_dependency_checking_errors_throwable

942478a

gmackall marked this pull request as ready for review June 3, 2024 19:38

gmackall requested review from a team and bartekpacia June 3, 2024 19:39

reidbaker requested changes Jun 3, 2024

View reviewed changes

gmackall marked this pull request as draft June 3, 2024 20:15

Gray Mackall added 3 commits June 3, 2024 14:45

make custom exception in kotlin, and use reflection in groovy to iden…

af26605

…tify it

Merge remote-tracking branch 'refs/remotes/origin/make_our_dependency…

b0237be

…_checking_errors_throwable' into make_our_dependency_checking_errors_throwable

don't use reflection - define the error in groovy instead

b02c1bd

gmackall requested a review from reidbaker June 3, 2024 22:23

Merge branch 'master' into make_our_dependency_checking_errors_throwable

323ed92

gmackall marked this pull request as ready for review June 3, 2024 22:23

bartekpacia approved these changes Jun 4, 2024

View reviewed changes

reidbaker reviewed Jun 4, 2024

View reviewed changes

reidbaker approved these changes Jun 4, 2024

View reviewed changes

gmackall added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 4, 2024

auto-submit bot merged commit 9d1de7b into flutter:master Jun 4, 2024

engine-flutter-autoroll mentioned this pull request Jun 4, 2024

Manual roll Flutter from c246ecdf8e8d to b50eb97b7d9e (10 revisions) flutter/packages#6867

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 4, 2024

9d1de7b Identify and re-throw our dependency checking errors in `flut…

45211b4

…ter.groovy` (flutter/flutter#149609)

engine-flutter-autoroll mentioned this pull request Jun 5, 2024

Roll Flutter from c246ecdf8e8d to ec9965bd89e6 (14 revisions) flutter/packages#6870

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2024

9d1de7b Identify and re-throw our dependency checking errors in `flut…

ab3e6b2

…ter.groovy` (flutter/flutter#149609)

engine-flutter-autoroll mentioned this pull request Jun 5, 2024

Roll Flutter from c246ecdf8e8d to cd8e84f46511 (15 revisions) flutter/packages#6872

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2024

9d1de7b Identify and re-throw our dependency checking errors in `flut…

6feba4a

…ter.groovy` (flutter/flutter#149609)

engine-flutter-autoroll mentioned this pull request Jun 5, 2024

Roll Flutter from c246ecdf8e8d to 39472d9b6193 (16 revisions) flutter/packages#6873

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2024

9d1de7b Identify and re-throw our dependency checking errors in `flut…

3f00354

…ter.groovy` (flutter/flutter#149609)

engine-flutter-autoroll mentioned this pull request Jun 5, 2024

Roll Flutter from c246ecdf8e8d to 27e06569a157 (17 revisions) flutter/packages#6875

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2024

9d1de7b Identify and re-throw our dependency checking errors in `flut…

ee915cc

…ter.groovy` (flutter/flutter#149609)

gmackall added the revert Autorevert PR (with "Reason for revert:" comment) label Jun 7, 2024

auto-submit bot removed the revert Autorevert PR (with "Reason for revert:" comment) label Jun 7, 2024

gmackall pushed a commit to gmackall/flutter that referenced this pull request Jun 7, 2024

Revert "Identify and re-throw our dependency checking errors in `flut…

c3853b6

…ter.groovy` (flutter#149609)" This reverts commit 9d1de7b.

gmackall pushed a commit to gmackall/flutter that referenced this pull request Jun 12, 2024

Reapply "Identify and re-throw our dependency checking errors in `flu…

db0c9bd

…tter.groovy` (flutter#149609)" (flutter#149918) This reverts commit 15307a9.

gmackall mentioned this pull request Jun 12, 2024

Reland "Identify and re-throw our dependency checking errors in flutter.groovy" #150128

Merged

9 tasks

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 6, 2024

9d1de7b Identify and re-throw our dependency checking errors in `flut…

77b0fa8

…ter.groovy` (flutter/flutter#149609)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Identify and re-throw our dependency checking errors in `flutter.groovy` #149609

Identify and re-throw our dependency checking errors in `flutter.groovy` #149609

Uh oh!

gmackall commented Jun 3, 2024 •

edited

Loading

Uh oh!

reidbaker Jun 3, 2024

Uh oh!

bartekpacia Jun 3, 2024

Uh oh!

gmackall Jun 3, 2024

Uh oh!

gmackall Jun 3, 2024

Uh oh!

reidbaker Jun 4, 2024

Uh oh!

gmackall Jun 4, 2024

Uh oh!

bartekpacia Jun 4, 2024

Uh oh!

gmackall Jun 4, 2024 •

edited

Loading

Uh oh!

bartekpacia Jun 4, 2024

Uh oh!

gmackall commented Jun 7, 2024

Uh oh!

auto-submit bot commented Jun 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

-            } catch (Exception e) {
+            } catch (DependencyValidationException e) {
+              // Rethrow
+            } catch (Exception e) {
+              // eat exception Do other code path.
+            }

Identify and re-throw our dependency checking errors in flutter.groovy #149609

Identify and re-throw our dependency checking errors in flutter.groovy #149609

Uh oh!

Conversation

gmackall commented Jun 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmackall Jun 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmackall commented Jun 7, 2024

Uh oh!

auto-submit bot commented Jun 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Identify and re-throw our dependency checking errors in `flutter.groovy` #149609

Identify and re-throw our dependency checking errors in `flutter.groovy` #149609

gmackall commented Jun 3, 2024 •

edited

Loading

gmackall Jun 4, 2024 •

edited

Loading