Preprovision Android NDK for flavored builds and reuse matching unflavored NDKs#183555
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors Android NDK provisioning for flavored builds. It introduces a new Gradle task, printNdkVersion, to query the effective NDK version from the Android project. For flavored builds, the Flutter tool now pre-provisions the required NDK version before the main Gradle build starts. For unflavored builds, it adds an optimization to skip forcing an NDK download if one is already discoverable via environment variables or in the Android SDK directory. This avoids unnecessary Gradle overhead and potential build graph contamination in flavored apps. The changes are accompanied by new unit and integration tests.
| for (final Directory ndkDirectory in ndkRoot.listSync().whereType<Directory>()) { | ||
| if (_isExistingAndroidNdkDirectory(ndkDirectory)) { | ||
| return ndkDirectory.basename; | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation for finding an existing NDK version iterates through directories in an order that is not guaranteed, which could lead to non-deterministic builds if multiple NDK versions are installed. To ensure that the latest available NDK is always chosen, it would be better to sort the directories by version number in descending order before iterating.
You'll also need to add import '../base/version.dart'; at the top of the file to use the Version class.
final List<Directory> ndkDirectories = ndkRoot
.listSync()
.whereType<Directory>()
.toList();
// Sort by version descending to pick the latest valid NDK.
ndkDirectories.sort((a, b) {
Version? aVersion;
Version? bVersion;
try {
aVersion = Version.parse(a.basename);
} on FormatException {
// Not a valid version string.
}
try {
bVersion = Version.parse(b.basename);
} on FormatException {
// Not a valid version string.
}
if (aVersion == null && bVersion == null) {
return 0;
}
if (aVersion == null) {
return 1;
}
if (bVersion == null) {
return -1;
}
return bVersion.compareTo(aVersion);
});
for (final Directory ndkDirectory in ndkDirectories) {
if (_isExistingAndroidNdkDirectory(ndkDirectory)) {
return ndkDirectory.basename;
}
}|
I pushed follow-up fixes for lint and tool_tests_general failures! @gmackall |
| val androidExtension = project.extensions.getByType(AbstractAppExtension::class.java) | ||
| project.tasks.register(TASK_PRINT_NDK_VERSION) { | ||
| description = "Prints out the configured ndkVersion for this Android project" | ||
| doLast { |
There was a problem hiding this comment.
Thanks. I left the current printing format in place to keep this change scoped to the NDK preprovisioning fix, and I will treat the migration to the new printing format as follow-up once #182788 lands.
| gradleProject: Project, | ||
| flutterSdkRootPath: String | ||
| ) { | ||
| if (isFlutterAppProject(gradleProject) && shouldSkipForcedNdkDownload(gradleProject)) { |
There was a problem hiding this comment.
Should the isFlutterAppProject check move inside the shouldSkipForcedNdkDownload?
There was a problem hiding this comment.
I ended up removing the combined skip helper in 80e04af rather than moving the app-project check into it. forceNdkDownload() is still invoked from a broader call site, so this kept the existing behavior while making the app-project gate and the individual skip reasons explicit in the method body.
| } | ||
|
|
||
| @JvmStatic | ||
| @JvmName("isInvokingMetadataNdkVersionTask") |
There was a problem hiding this comment.
I am struggling to understand this method. Why would shouldSkipForcedNdkDownload check to see if we had added our own print task to the list of tasks?
There was a problem hiding this comment.
The reason for that check was that printNdkVersion is a metadata-only preflight query. Without the early return, we would still run forceNdkDownload() during that invocation and mutate the synthetic CMake/native path just to answer the metadata query. I split that condition out in 80e04af so the metadata-task skip is explicit instead of being hidden behind shouldSkipForcedNdkDownload().
| throw TaskResult.failure( | ||
| 'Not producing external native build output directory in $modifiedPath', | ||
| ); | ||
| if (Directory(modifiedPath).existsSync()) { |
There was a problem hiding this comment.
I dont follow why the not was removed from this failure.
There was a problem hiding this comment.
The old test expected forceNdkDownload() to create the synthetic external native build staging dir under build/.cxx. Now that we're using a preprovisioned NDK path, we don't go through that synthetic CMake path at all — so neither android/app/.cxx nor build/.cxx should get created.
| } | ||
| // Pipe stdout/stderr from Gradle. | ||
| return line; | ||
| return printOutput ? line : null; |
There was a problem hiding this comment.
@gmackall what do you think about running gradle commands with no output?
There was a problem hiding this comment.
This is just for the metadata-only printNdkVersion preflight, not regular Gradle builds. Didn't want that preflight output bleeding into the normal build logs. We still surface handled Gradle errors once a matcher wins, but happy to change this if we'd rather keep the preflight output visible too.
|
@reidbaker All presubmits are green now, including the latest CICD rerun. If you have any remaining concerns, I’m happy to address them. |
|
@reidbaker @gmackall All checks are green again, including tree-status. Would you mind taking another look when you have a chance? Happy to address anything else if needed. |
|
@gmackall when you approve can you add autosubmit? I would but the pr needs 2 approvals. |
gmackall
left a comment
There was a problem hiding this comment.
Mostly looks good to me, added some comments. Sorry for the delay in review
| } | ||
|
|
||
| String? _getExistingAndroidNdkSkipValue() { | ||
| final AndroidSdk? androidSdk = globals.androidSdk; |
There was a problem hiding this comment.
Given we have a handle on the androidSdk here, is there a reason not to use androidSdk.getNdkBinaryPath ?
I suppose we aren't looking for a binary, but probably we can pull the method out which finds the home dir
expose that and use it here? I think it would be good to avoid duplicating the logic of how we find the ndk, as they are likely to drift from each other.
There was a problem hiding this comment.
Good catch. I pulled the NDK lookup order into AndroidSdk so gradle.dart is not carrying its own copy anymore. Pushed in c1747a4.
| gradleExecutablePath: gradleExecutablePath, | ||
| buildInfo: buildInfo, | ||
| ) | ||
| : _getExistingAndroidNdkSkipValue(); |
There was a problem hiding this comment.
Unfortunately we can't skip in this case.
Flutter forces the download of the ndk specifically because, for some reason, AGP only strips debug symbols from native libraries if the ndk is present (otherwise it silently fails and the debug symbols end up in the app delivered to users, making it massive). And unfortunately this behavior only works if the installed ndk version is actually the one specified in your gradle files.
So we could only skip here if we were certain that the existing installed ndk version is the one returned from _getNdkVersion.
I think it would be reasonable to either add that to the check here (in which case we should update the title to reflect that this impacts non flavored builds as well)
or to remove this case and file a follow up issue to consider using this path for non-flavored apps as well.
There was a problem hiding this comment.
That makes sense. I will try the stricter route first: only skip for unflavored builds when the installed NDK exactly matches _getNdkVersion(). If that lands cleanly, I will also update the PR title so it reflects the non-flavored impact. If it starts getting too broad, I will drop the unflavored case from this PR and move it into a follow-up.
There was a problem hiding this comment.
I tightened the unflavored path instead of just checking for any installed NDK. It now only skips the forced download when the configured NDK from printNdkVersion is already installed. I pushed that in aa2afa5 and updated the title to reflect the unflavored behavior too. I also re-ran the updated tests and did a couple of real flutter build checks for both flavored and unflavored repro apps.
There was a problem hiding this comment.
Looks like first invocation takes around 5 seconds, and subsequent take ~600-700 ms on my mac.
I think that's reasonable, if a bit slower than I would hope (maybe we buy some time back from what AGP would have done in the cmake config route). LGTM, thanks for the contribution! I think @reidbaker will need to approve again before we can merge, as we've recently turned on a github feature where new commits dismiss existing reviews as stale.
Also it seems there are internal failures so we will need to add a g3fix to get this landed, though you won't need to make any changes for that (just might delay merging)
|
Looks like the g3fix resolves the issues, just need to get it approved and this should be good to land |
…hing unflavored NDKs (flutter/flutter#183555)
…hing unflavored NDKs (flutter/flutter#183555)
flutter/flutter@2fa45e0...c1b14e9 2026-04-14 15619084+vashworth@users.noreply.github.com Rebuild flutter tool skill (flutter/flutter#184975) 2026-04-14 engine-flutter-autoroll@skia.org Roll Skia from 0851d988db03 to 4c382df6a25a (1 revision) (flutter/flutter#185025) 2026-04-14 engine-flutter-autoroll@skia.org Roll Dart SDK from 5504504b38c2 to ee5afcef0596 (1 revision) (flutter/flutter#185024) 2026-04-14 dacoharkes@google.com [ci] Split up integration.shard record_use_test.dart (flutter/flutter#185022) 2026-04-14 engine-flutter-autoroll@skia.org Roll Skia from d34c84df4c37 to 0851d988db03 (3 revisions) (flutter/flutter#185012) 2026-04-14 dacoharkes@google.com [record_use] Add recorded uses to link hooks (flutter/flutter#184869) 2026-04-14 engine-flutter-autoroll@skia.org Roll Skia from 0e98a9c635bb to d34c84df4c37 (5 revisions) (flutter/flutter#185009) 2026-04-14 engine-flutter-autoroll@skia.org Roll Dart SDK from ef28089d6533 to 5504504b38c2 (3 revisions) (flutter/flutter#185008) 2026-04-14 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from K_2AkZL3Drs6cGE1q... to rB8LAuZL_DwHMssTU... (flutter/flutter#185007) 2026-04-14 rmacnak@google.com [fuchsia] Replace ambient-replace-as-executable with VmexResource. (flutter/flutter#184967) 2026-04-13 chinmaygarde@google.com [Impeller] Commands that don't specify their own viewports get the viewport of the render pass. (flutter/flutter#177473) 2026-04-13 engine-flutter-autoroll@skia.org Roll Skia from bc1df263ff3f to 0e98a9c635bb (1 revision) (flutter/flutter#184995) 2026-04-13 737941+loic-sharma@users.noreply.github.com Update autosubmit guide with the emergency label (flutter/flutter#184993) 2026-04-13 69043738+aNOOBisTheGod@users.noreply.github.com [flutter_tools] Cache pubspec reads and share PackageGraph/PackageConfig across workspace packages during pub get post-processing (flutter/flutter#184528) 2026-04-13 15619084+vashworth@users.noreply.github.com Fix codesign verification test for SwiftPM Add to App (flutter/flutter#184980) 2026-04-13 87962825+kyungilcho@users.noreply.github.com Preprovision Android NDK for flavored builds and reuse matching unflavored NDKs (flutter/flutter#183555) 2026-04-13 15619084+vashworth@users.noreply.github.com Reland "Disable async mode with LLDB" (flutter/flutter#184970) 2026-04-13 engine-flutter-autoroll@skia.org Roll Skia from 55ddd6bb8be5 to bc1df263ff3f (6 revisions) (flutter/flutter#184968) 2026-04-13 matej.knopp@gmail.com Expose platform specific handles for multi-window API (flutter/flutter#184662) 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 boetger@google.com,stuartmorgan@google.com 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
…vored NDKs (flutter#183555) Flutter currently forces AGP to download the Android NDK by configuring a synthetic empty CMake project in `forceNdkDownload()`. For flavored Android apps, that synthetic native configuration can cause AGP to fold generic CMake tasks across flavors. In practice, building one flavor can pull native/CMake work from another flavor into the active build. This change moves NDK provisioning into the Flutter tool flow for flavored app builds: - query the effective `ndkVersion` with a lightweight Gradle metadata task - install the required NDK before the main build when needed - pass a preprovisioned NDK property so `forceNdkDownload()` can return early To avoid adding unnecessary Gradle overhead to common unflavored builds, the Flutter tool now also skips `forceNdkDownload()` when an existing NDK is already discoverable from the Android SDK or standard Android NDK environment variables. This preserves the original goal of ensuring the NDK is available, while avoiding flavored CMake/native graph contamination. Manual validation: - verified on a minimal flavored Android app that `dev` builds no longer pull `prod` native/CMake work into the task graph - verified locally on a downstream flavored Flutter app for `build apk`, `build appbundle`, and `flutter run` Fixes flutter#183296 --------- Co-authored-by: Gray Mackall <34871572+gmackall@users.noreply.github.com>
…vored NDKs (flutter#183555) Flutter currently forces AGP to download the Android NDK by configuring a synthetic empty CMake project in `forceNdkDownload()`. For flavored Android apps, that synthetic native configuration can cause AGP to fold generic CMake tasks across flavors. In practice, building one flavor can pull native/CMake work from another flavor into the active build. This change moves NDK provisioning into the Flutter tool flow for flavored app builds: - query the effective `ndkVersion` with a lightweight Gradle metadata task - install the required NDK before the main build when needed - pass a preprovisioned NDK property so `forceNdkDownload()` can return early To avoid adding unnecessary Gradle overhead to common unflavored builds, the Flutter tool now also skips `forceNdkDownload()` when an existing NDK is already discoverable from the Android SDK or standard Android NDK environment variables. This preserves the original goal of ensuring the NDK is available, while avoiding flavored CMake/native graph contamination. Manual validation: - verified on a minimal flavored Android app that `dev` builds no longer pull `prod` native/CMake work into the task graph - verified locally on a downstream flavored Flutter app for `build apk`, `build appbundle`, and `flutter run` Fixes flutter#183296 --------- Co-authored-by: Gray Mackall <34871572+gmackall@users.noreply.github.com>
|
@kyungilcho This looks like it caused a regression in our compile time benchmarks. Is this expected? The cc @flar |
|
Hmm this seems like a more significant increase than expected on windows (and for compiling after editing). Unfortunately I think we should revert this and plan to reland with the approach of only using this pre-provisioning approach when building with flavors. Sorry @kyungilcho I only tested on my mac and did not expect the increase to be so significant on windows (it's a 40+% increase in first compile time) |
|
Reason for revert: larger than expected impact on build times, particularly on windows and for builds after editing |
|
Time to revert pull request flutter/flutter/183555 has elapsed. |
flutter#185439) …ng unflavored NDKs (flutter#183555)" This reverts commit fb3b552. See context starting at flutter#183555 (comment) This will need a separate g3fix, as the original pr had one
…r#11506) flutter/flutter@2fa45e0...c1b14e9 2026-04-14 15619084+vashworth@users.noreply.github.com Rebuild flutter tool skill (flutter/flutter#184975) 2026-04-14 engine-flutter-autoroll@skia.org Roll Skia from 0851d988db03 to 4c382df6a25a (1 revision) (flutter/flutter#185025) 2026-04-14 engine-flutter-autoroll@skia.org Roll Dart SDK from 5504504b38c2 to ee5afcef0596 (1 revision) (flutter/flutter#185024) 2026-04-14 dacoharkes@google.com [ci] Split up integration.shard record_use_test.dart (flutter/flutter#185022) 2026-04-14 engine-flutter-autoroll@skia.org Roll Skia from d34c84df4c37 to 0851d988db03 (3 revisions) (flutter/flutter#185012) 2026-04-14 dacoharkes@google.com [record_use] Add recorded uses to link hooks (flutter/flutter#184869) 2026-04-14 engine-flutter-autoroll@skia.org Roll Skia from 0e98a9c635bb to d34c84df4c37 (5 revisions) (flutter/flutter#185009) 2026-04-14 engine-flutter-autoroll@skia.org Roll Dart SDK from ef28089d6533 to 5504504b38c2 (3 revisions) (flutter/flutter#185008) 2026-04-14 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from K_2AkZL3Drs6cGE1q... to rB8LAuZL_DwHMssTU... (flutter/flutter#185007) 2026-04-14 rmacnak@google.com [fuchsia] Replace ambient-replace-as-executable with VmexResource. (flutter/flutter#184967) 2026-04-13 chinmaygarde@google.com [Impeller] Commands that don't specify their own viewports get the viewport of the render pass. (flutter/flutter#177473) 2026-04-13 engine-flutter-autoroll@skia.org Roll Skia from bc1df263ff3f to 0e98a9c635bb (1 revision) (flutter/flutter#184995) 2026-04-13 737941+loic-sharma@users.noreply.github.com Update autosubmit guide with the emergency label (flutter/flutter#184993) 2026-04-13 69043738+aNOOBisTheGod@users.noreply.github.com [flutter_tools] Cache pubspec reads and share PackageGraph/PackageConfig across workspace packages during pub get post-processing (flutter/flutter#184528) 2026-04-13 15619084+vashworth@users.noreply.github.com Fix codesign verification test for SwiftPM Add to App (flutter/flutter#184980) 2026-04-13 87962825+kyungilcho@users.noreply.github.com Preprovision Android NDK for flavored builds and reuse matching unflavored NDKs (flutter/flutter#183555) 2026-04-13 15619084+vashworth@users.noreply.github.com Reland "Disable async mode with LLDB" (flutter/flutter#184970) 2026-04-13 engine-flutter-autoroll@skia.org Roll Skia from 55ddd6bb8be5 to bc1df263ff3f (6 revisions) (flutter/flutter#184968) 2026-04-13 matej.knopp@gmail.com Expose platform specific handles for multi-window API (flutter/flutter#184662) 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 boetger@google.com,stuartmorgan@google.com 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
…r#11506) flutter/flutter@2fa45e0...c1b14e9 2026-04-14 15619084+vashworth@users.noreply.github.com Rebuild flutter tool skill (flutter/flutter#184975) 2026-04-14 engine-flutter-autoroll@skia.org Roll Skia from 0851d988db03 to 4c382df6a25a (1 revision) (flutter/flutter#185025) 2026-04-14 engine-flutter-autoroll@skia.org Roll Dart SDK from 5504504b38c2 to ee5afcef0596 (1 revision) (flutter/flutter#185024) 2026-04-14 dacoharkes@google.com [ci] Split up integration.shard record_use_test.dart (flutter/flutter#185022) 2026-04-14 engine-flutter-autoroll@skia.org Roll Skia from d34c84df4c37 to 0851d988db03 (3 revisions) (flutter/flutter#185012) 2026-04-14 dacoharkes@google.com [record_use] Add recorded uses to link hooks (flutter/flutter#184869) 2026-04-14 engine-flutter-autoroll@skia.org Roll Skia from 0e98a9c635bb to d34c84df4c37 (5 revisions) (flutter/flutter#185009) 2026-04-14 engine-flutter-autoroll@skia.org Roll Dart SDK from ef28089d6533 to 5504504b38c2 (3 revisions) (flutter/flutter#185008) 2026-04-14 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from K_2AkZL3Drs6cGE1q... to rB8LAuZL_DwHMssTU... (flutter/flutter#185007) 2026-04-14 rmacnak@google.com [fuchsia] Replace ambient-replace-as-executable with VmexResource. (flutter/flutter#184967) 2026-04-13 chinmaygarde@google.com [Impeller] Commands that don't specify their own viewports get the viewport of the render pass. (flutter/flutter#177473) 2026-04-13 engine-flutter-autoroll@skia.org Roll Skia from bc1df263ff3f to 0e98a9c635bb (1 revision) (flutter/flutter#184995) 2026-04-13 737941+loic-sharma@users.noreply.github.com Update autosubmit guide with the emergency label (flutter/flutter#184993) 2026-04-13 69043738+aNOOBisTheGod@users.noreply.github.com [flutter_tools] Cache pubspec reads and share PackageGraph/PackageConfig across workspace packages during pub get post-processing (flutter/flutter#184528) 2026-04-13 15619084+vashworth@users.noreply.github.com Fix codesign verification test for SwiftPM Add to App (flutter/flutter#184980) 2026-04-13 87962825+kyungilcho@users.noreply.github.com Preprovision Android NDK for flavored builds and reuse matching unflavored NDKs (flutter/flutter#183555) 2026-04-13 15619084+vashworth@users.noreply.github.com Reland "Disable async mode with LLDB" (flutter/flutter#184970) 2026-04-13 engine-flutter-autoroll@skia.org Roll Skia from 55ddd6bb8be5 to bc1df263ff3f (6 revisions) (flutter/flutter#184968) 2026-04-13 matej.knopp@gmail.com Expose platform specific handles for multi-window API (flutter/flutter#184662) 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 boetger@google.com,stuartmorgan@google.com 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
Flutter currently forces AGP to download the Android NDK by configuring a
synthetic empty CMake project in
forceNdkDownload().For flavored Android apps, that synthetic native configuration can cause AGP
to fold generic CMake tasks across flavors. In practice, building one flavor
can pull native/CMake work from another flavor into the active build.
This change moves NDK provisioning into the Flutter tool flow for flavored app
builds:
ndkVersionwith a lightweight Gradle metadata taskforceNdkDownload()can return earlyTo avoid adding unnecessary Gradle overhead to common unflavored builds, the
Flutter tool now also skips
forceNdkDownload()when an existing NDK isalready discoverable from the Android SDK or standard Android NDK environment
variables.
This preserves the original goal of ensuring the NDK is available, while
avoiding flavored CMake/native graph contamination.
Manual validation:
devbuilds no longer pullprodnative/CMake work into the task graphbuild apk,build appbundle, andflutter runFixes #183296