Refactor gradle.dart #43479
Refactor gradle.dart #43479
Conversation
fluttergithubbot
added
the
tool
blasten
force-pushed
the
faster_android_build
branch
from
41da3d2
to
90c55fe
Compare
packages/flutter_tools/test/general.shard/android/gradle_errors_test.dart
Show resolved
Hide resolved
| // currently impossible as the test is not hermetic. Luckily, our bots don't have Android | ||
| // SDKs yet so androidSdk should be null by default. | ||
| // | ||
| // This test is written to fail if our bots get Android SDKs in the future: shouldBeToolExit |
There was a problem hiding this comment.
What is the status of this part of the comment?
There was a problem hiding this comment.
This test was moved to gradle_utils_test.dart and it now checks for the exact exception message.
|
|
||
| /// Warning mark to use in stdout or stderr. | ||
| String get warningMark { | ||
| return terminal.bolden(terminal.color('[!]', TerminalColor.red)); |
There was a problem hiding this comment.
I tried. Unfortunately, it doesn't render on PowerShell/cmd.exe. We could detect that or leave as is.
| @@ -6,11 +6,11 @@ import 'dart:async'; | |||
|
|
|||
There was a problem hiding this comment.
Seems like AndroidBuilder is mostly just a trivial wrapper around buildGradleAar and buildGradleApp now. Can we remove the entire file? (Maybe in a follow-up PR.)
There was a problem hiding this comment.
+1. I filed #43863. Originally this was needed to test the build commands because the current build process is messy and requires mocking too many files/process calls, etc.
| } | ||
| // Don't log analytics for downstream Flutter commands. | ||
| // e.g. `flutter build bundle`. | ||
| env['FLUTTER_SUPPRESS_ANALYTICS'] = 'true'; |
There was a problem hiding this comment.
it would be cleaner if flutter was always called with an argument that did that, rather than relying on the environment variables.
There was a problem hiding this comment.
+1. When the tool calls Gradle, it doesn't log analytics. However, in add2app scenarios, when a developer calls Gradle directly (from Android Studio, for example), it's preferred to log the calls to the tool.
| throwToolExit('Unable to locate gradlew script'); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Should much of the contents of gradle.dart also be in a GradeUtils-like class? (maybe GradleUtils itself?)
|
|
blasten
force-pushed
the
faster_android_build
branch
3 times, most recently
from
6fe8aaf
to
16246b4
Compare
blasten
force-pushed
the
faster_android_build
branch
from
16246b4
to
a86a1fd
Compare
blasten
force-pushed
the
faster_android_build
branch
2 times, most recently
from
82b4f15
to
abe34e2
Compare
|
PTAL @zanderso |
blasten
force-pushed
the
faster_android_build
branch
from
abe34e2
to
f67a150
Compare
| break; | ||
| } | ||
| } | ||
| // Pipe stdout/sterr from Gradle. |
| } else { | ||
| settings.values[key] = value; | ||
| if (retries >= 1) { | ||
| final String successEventLabel = 'gradle--${detectedGradleError.eventLabel}-success'; |
There was a problem hiding this comment.
nit: is there an extra '-' here?
| if (isBuildingBundle) { | ||
| final File bundleFile = findBundleFile(project, buildInfo); | ||
| if (bundleFile == null) { | ||
| throwToolExit('Gradle build failed to produce an Android bundle package.'); |
There was a problem hiding this comment.
Is it possible to supply some advice here? Like, "Try again." or "Look up in the logs for your error message from the Java build." etc.
| } | ||
| changed = true; | ||
| BuildEvent('gradle--${detectedGradleError.eventLabel}-failure').send(); |
| } | ||
| changed = true; | ||
| BuildEvent('gradle--${detectedGradleError.eventLabel}-failure').send(); |
There was a problem hiding this comment.
You could include the exit code in one of the other fields of the event if that would be useful.
| } | ||
| if (isBuildingBundle) { | ||
| final File bundleFile = findBundleFile(project, buildInfo); | ||
| if (bundleFile == null) { |
There was a problem hiding this comment.
Would a BuildEvent for this be useful?
| // Gradle produced an APK. | ||
| final Iterable<File> apkFiles = findApkFiles(project, androidBuildInfo); | ||
| if (apkFiles.isEmpty) { | ||
| throwToolExit('Gradle build failed to produce an Android package.'); |
There was a problem hiding this comment.
Same question about the error message.
| settings.writeContents(localProperties); | ||
| // Gradle produced an APK. | ||
| final Iterable<File> apkFiles = findApkFiles(project, androidBuildInfo); | ||
| if (apkFiles.isEmpty) { |
There was a problem hiding this comment.
Same question about a BuildEvent.
| @@ -686,41 +533,24 @@ Future<void> buildGradleAar({ | |||
| if (result.exitCode != 0) { | |||
| printStatus(result.stdout, wrap: false); | |||
| printError(result.stderr, wrap: false); | |||
| throwToolExit('Gradle task $aarTask failed with exit code $exitCode.', exitCode: exitCode); | |||
| throwToolExit( | |||
| if (!repoDirectory.existsSync()) { | ||
| printStatus(result.stdout, wrap: false); | ||
| printError(result.stderr, wrap: false); | ||
| throwToolExit('Gradle task $aarTask failed to produce $repoDirectory.', exitCode: exitCode); | ||
| throwToolExit( |
Follow-up on flutter/flutter#43479.
This PR refactors
gradle.dart.Here's the summary of the two main changes:
First, running
flutter build apk/appbundle/aarinvolves 3 calls to Gradle. One to get the version, one to get the list of flavors and finally the last call to build the target artifacts. These calls add about 2s overhead to the build calls, but most importantly add complexity to the code. e.g. Caching ofstdoutis required, so tests don't time out.I'm essentially getting rid of that. Once this PR is merged, we will call Gradle only once to build the artifacts. We will only get the list of flavor if Gradle failed due to an undefined flavor set via the tool's
--flavorflag. This is the only use case for getting the list of flavors from Gradle.Second, we have logic to detect common Gradles errors that attempts to guide the user toward completing the journey. This logic is all over the place. Some code checks if an error is triggered when checking the Gradle version. Other code checks if an error is triggered when building the artifact.
I don't think this is necessary since these errors occur not matter when the call to Gradle is made. As a result, I organized these logic around a new data structured called
GradleBuildError, which makes it easier to add new error handler in the near future.