From 522eda00cab07eeb34765df5fd34ae2266938bc4 Mon Sep 17 00:00:00 2001 From: Camille Simon <43054281+camsim99@users.noreply.github.com> Date: Wed, 29 Jan 2025 15:31:59 -0600 Subject: [PATCH] [Android] Fix integration test to check if dev dependencies are removed from release builds + address no non-dev dependency plugin edge case (#161826) Makes the following change to fix the case where dev dependencies are stripped in apps that only contain dev dependencies: - Changes the Flutter Gradle plugin to add the Flutter embedding as a direct dependency of a Flutter app if it contains no plugins that would include it transitively (this excludes dev dependencies in release builds) Makes the following changes to correct + improve the integration test that checks if dev dependencies are stripped from release builds: - Fixes the plugin that was supposed to be as dev dependency to actually be a dev dependency - Changes the test structure to check the output of `./gradlew app:dependencies` (see [more details on this call](https://docs.gradle.org/current/userguide/viewing_debugging_dependencies.html)) instead of the classes included in the APK. Checking the APK was leading us astray because it appears obfuscation occurs for release builds. - Adds more sections, comments, etc. Fixes https://github.com/flutter/flutter/issues/161780. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [ ] All existing and new tests are passing. [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --------- Co-authored-by: Matan Lurey --- ..._builds_exclude_dev_dependencies_test.dart | 96 ++++++++++++------- .../gradle/src/main/groovy/flutter.groovy | 24 ++++- 2 files changed, 84 insertions(+), 36 deletions(-) diff --git a/dev/devicelab/bin/tasks/android_release_builds_exclude_dev_dependencies_test.dart b/dev/devicelab/bin/tasks/android_release_builds_exclude_dev_dependencies_test.dart index 4bc635beb1..0c2dd5e6e2 100644 --- a/dev/devicelab/bin/tasks/android_release_builds_exclude_dev_dependencies_test.dart +++ b/dev/devicelab/bin/tasks/android_release_builds_exclude_dev_dependencies_test.dart @@ -8,72 +8,100 @@ import 'dart:io'; import 'package:flutter_devicelab/framework/apk_utils.dart'; import 'package:flutter_devicelab/framework/framework.dart'; import 'package:flutter_devicelab/framework/task_result.dart'; -import 'package:flutter_devicelab/framework/utils.dart'; +import 'package:flutter_devicelab/framework/utils.dart' as utils; import 'package:path/path.dart' as path; Future main() async { await task(() async { try { await runProjectTest((FlutterProject flutterProject) async { + utils.section( + 'Configure plugins to be marked as dev dependencies in .flutter-plugins-dependencies file', + ); + // Enable plugins being marked as dev dependncies in the .flutter-plugins-dependencies file. - await flutter('config', options: ['--explicit-package-dependencies']); + await utils.flutter('config', options: ['--explicit-package-dependencies']); // Create dev_dependency plugin to use for test. final Directory tempDir = Directory.systemTemp.createTempSync( 'android_release_builds_exclude_dev_dependencies_test.', ); const String devDependencyPluginOrg = 'com.example.dev_dependency_plugin'; + + utils.section('Create plugin dev_dependency_plugin that supports Android'); + await FlutterPluginProject.create( tempDir, 'dev_dependency_plugin', options: ['--platforms=android', '--org=$devDependencyPluginOrg'], ); + utils.section('Add dev_dependency_plugin as a dev dependency to the Flutter app project'); + // Add devDependencyPlugin as dependency of flutterProject. await flutterProject.addPlugin( - 'dev_dependency_plugin', + 'dev:dev_dependency_plugin', options: ['--path', path.join(tempDir.path, 'dev_dependency_plugin')], ); + utils.section( + 'Verify the app includes/excludes dev_dependency_plugin as dependency in each build mode as expected', + ); final List buildModesToTest = ['debug', 'profile', 'release']; for (final String buildMode in buildModesToTest) { - section('APK does contain methods from dev dependency in $buildMode mode'); + final String gradlew = Platform.isWindows ? 'gradlew.bat' : 'gradlew'; + final String gradlewExecutable = Platform.isWindows ? '.\\$gradlew' : './$gradlew'; + final RegExp regExpToMatchDevDependencyPlugin = RegExp( + r'--- project :dev_dependency_plugin', + ); + final RegExp regExpToMatchDevDependencyPluginWithTransitiveDependencies = RegExp( + r'--- project :dev_dependency_plugin\n(\s)*\+--- org.jetbrains.kotlin.*\s\(\*\)\n(\s)*\\---\sio.flutter:flutter_embedding_' + + buildMode, + ); + const String stringToMatchFlutterEmbedding = '+--- io.flutter:flutter_embedding_release:'; + final bool isTestingReleaseMode = buildMode == 'release'; - // Build APK in buildMode and check that devDependencyPlugin is included/excluded in the APK as expected. - await inDirectory(flutterProject.rootPath, () async { - await flutter( - 'build', - options: ['apk', '--$buildMode', '--target-platform=android-arm'], + utils.section('Query the dependencies of the app built with $buildMode'); + + final String appDependencies = await utils.eval(gradlewExecutable, [ + 'app:dependencies', + '--configuration', + '${buildMode}RuntimeClasspath', + ], workingDirectory: flutterProject.androidPath); + + if (isTestingReleaseMode) { + utils.section( + 'Check that the release build includes Flutter embedding as a direct dependency', ); - final File apk = File( - path.join( - flutterProject.rootPath, - 'build', - 'app', - 'outputs', - 'flutter-apk', - 'app-$buildMode.apk', - ), - ); - if (!apk.existsSync()) { - throw TaskResult.failure("Expected ${apk.path} to exist, but it doesn't."); - } - - // We expect the APK to include the devDependencyPlugin except in release mode. - final bool isTestingReleaseMode = buildMode == 'release'; - final bool apkIncludesDevDependency = await checkApkContainsMethodsFromLibrary( - apk, - devDependencyPluginOrg, - ); - final bool apkIncludesDevDependencyAsExpected = - isTestingReleaseMode ? !apkIncludesDevDependency : apkIncludesDevDependency; - if (!apkIncludesDevDependencyAsExpected) { + if (!appDependencies.contains(stringToMatchFlutterEmbedding)) { + // We expect dev_dependency_plugin to not be included in the dev dependency, but the Flutter + // embedding should still be a dependency of the app project (regardless of the fact + // that the app does not depend on any plugins that support Android, which would cause the + // Flutter embedding to be included as a transitive dependency). throw TaskResult.failure( - 'Expected to${isTestingReleaseMode ? ' not' : ''} find dev_dependency_plugin in APK built with debug mode but did${isTestingReleaseMode ? '' : ' not'}.', + 'Expected to find the Flutter embedding as a dependency of the release app build, but did not.', ); } - }); + } + + utils.section( + 'Check that the $buildMode build includes/excludes the dev dependency plugin as expected', + ); + + // Ensure that release builds have no reference to the dev dependency plugin and make sure + // that it is included with expected transitive dependencies for debug, profile builds. + final bool appIncludesDevDependencyAsExpected = + isTestingReleaseMode + ? !appDependencies.contains(regExpToMatchDevDependencyPlugin) + : appDependencies.contains( + regExpToMatchDevDependencyPluginWithTransitiveDependencies, + ); + if (!appIncludesDevDependencyAsExpected) { + throw TaskResult.failure( + 'Expected to${isTestingReleaseMode ? ' not' : ''} find dev_dependency_plugin as a dependency of the app built in $buildMode mode but did${isTestingReleaseMode ? '' : ' not'}.', + ); + } } }); return TaskResult.success(null); diff --git a/packages/flutter_tools/gradle/src/main/groovy/flutter.groovy b/packages/flutter_tools/gradle/src/main/groovy/flutter.groovy index 4d0168ad31..c843546f3d 100644 --- a/packages/flutter_tools/gradle/src/main/groovy/flutter.groovy +++ b/packages/flutter_tools/gradle/src/main/groovy/flutter.groovy @@ -590,11 +590,13 @@ class FlutterPlugin implements Plugin { } // The embedding is set as an API dependency in a Flutter plugin. // Therefore, don't make the app project depend on the embedding if there are Flutter - // plugins. + // plugin dependencies. In release mode, dev dependencies are stripped, so we do not + // consider those in the check. // This prevents duplicated classes when using custom build types. That is, a custom build // type like profile is used, and the plugin and app projects have API dependencies on the // embedding. - if (!isFlutterAppProject() || getPluginList(project).size() == 0) { + List> pluginsThatIncludeFlutterEmbeddingAsTransitiveDependency = flutterBuildMode == "release" ? getPluginListWithoutDevDependencies(project) : getPluginList(project); + if (!isFlutterAppProject() || pluginsThatIncludeFlutterEmbeddingAsTransitiveDependency.size() == 0) { addApiDependencies(project, buildType.name, "io.flutter:flutter_embedding_$flutterBuildMode:$engineVersion") } @@ -988,6 +990,24 @@ class FlutterPlugin implements Plugin { return pluginList } + /** + * Gets the list of plugins (as map) that support the Android platform and are dependencies of the + * Android project excluding dev dependencies. + * + * The map value contains either the plugins `name` (String), + * its `path` (String), or its `dependencies` (List). + * See [NativePluginLoader#getPlugins] in packages/flutter_tools/gradle/src/main/groovy/native_plugin_loader.groovy + */ + private List> getPluginListWithoutDevDependencies(Project project) { + List> pluginListWithoutDevDependencies = [] + for (Map plugin in getPluginList(project)) { + if (!plugin.dev_dependency) { + pluginListWithoutDevDependencies += plugin + } + } + return pluginListWithoutDevDependencies + } + // TODO(54566, 48918): Remove in favor of [getPluginList] only, see also // https://github.com/flutter/flutter/blob/1c90ed8b64d9ed8ce2431afad8bc6e6d9acc4556/packages/flutter_tools/lib/src/flutter_plugins.dart#L212 /** Gets the plugins dependencies from `.flutter-plugins-dependencies`. */