From f9f42bee9959f4f304f51dbe5c060db406b4c096 Mon Sep 17 00:00:00 2001 From: Camille Simon <43054281+camsim99@users.noreply.github.com> Date: Wed, 4 Dec 2024 13:13:07 -0500 Subject: [PATCH] [Android] Removes dev dependency plugins from release builds (#158026) Removes plugins that are `dev_dependency`s from being dependencies of release builds of Flutter Android app projects. Fixes https://github.com/flutter/flutter/issues/157949. ## 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. - [x] 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 --- .ci.yaml | 19 ++++++ TESTOWNERS | 1 + ..._builds_exclude_dev_dependencies_test.dart | 60 +++++++++++++++++++ .../tasks/gradle_desugar_classes_test.dart | 2 +- .../tasks/gradle_plugin_light_apk_test.dart | 5 +- dev/devicelab/lib/framework/apk_utils.dart | 44 ++++++++++---- .../gradle/src/main/groovy/flutter.groovy | 51 +++++++++++----- .../main/groovy/native_plugin_loader.groovy | 20 ++++++- 8 files changed, 171 insertions(+), 31 deletions(-) create mode 100644 dev/devicelab/bin/tasks/android_release_builds_exclude_dev_dependencies_test.dart diff --git a/.ci.yaml b/.ci.yaml index 2a915617e1..8d9d98fae1 100644 --- a/.ci.yaml +++ b/.ci.yaml @@ -1077,6 +1077,25 @@ targets: ["devicelab", "hostonly", "linux"] task_name: linux_desktop_impeller + - name: Linux android_release_builds_exclude_dev_dependencies_test + recipe: devicelab/devicelab_drone + timeout: 60 + bringup: true + properties: + dependencies: >- + [ + {"dependency": "android_sdk", "version": "version:35v1"}, + {"dependency": "chrome_and_driver", "version": "version:125.0.6422.141"}, + {"dependency": "open_jdk", "version": "version:17"} + ] + tags: > + ["devicelab", "hostonly", "linux"] + task_name: android_release_builds_exclude_dev_dependencies_test + runIf: + - dev/** + - bin/** + - .ci.yaml + - name: Linux run_release_test_linux recipe: devicelab/devicelab_drone timeout: 60 diff --git a/TESTOWNERS b/TESTOWNERS index 76c8b2bef2..77699dcfdb 100644 --- a/TESTOWNERS +++ b/TESTOWNERS @@ -230,6 +230,7 @@ ## Host only DeviceLab tests /dev/devicelab/bin/tasks/animated_complex_opacity_perf_macos__e2e_summary.dart @cbracken @flutter/desktop +/dev/devicelab/bin/tasks/android_release_builds_exclude_dev_dependencies_test.dart @camsim99 @flutter/android /dev/devicelab/bin/tasks/basic_material_app_macos__compile.dart @cbracken @flutter/desktop /dev/devicelab/bin/tasks/build_aar_module_test.dart @andrewkolos @flutter/tool /dev/devicelab/bin/tasks/build_ios_framework_module_test.dart @jmagman @flutter/tool 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 new file mode 100644 index 0000000000..c8e6e30530 --- /dev/null +++ b/dev/devicelab/bin/tasks/android_release_builds_exclude_dev_dependencies_test.dart @@ -0,0 +1,60 @@ +// Copyright 2014 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'dart:async'; +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:path/path.dart' as path; + +Future main() async { + await task(() async { + try { + await runProjectTest((FlutterProject flutterProject) async { + // 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'; + await FlutterPluginProject.create(tempDir, 'dev_dependency_plugin', options: ['--platforms=android', '--org=$devDependencyPluginOrg']); + + // Add devDependencyPlugin as dependency of flutterProject. + await flutterProject.addPlugin('dev_dependency_plugin', options: ['--path', path.join(tempDir.path, 'dev_dependency_plugin')]); + + final List buildModesToTest = ['debug', 'profile', 'release']; + for (final String buildMode in buildModesToTest) { + section('APK does contain methods from dev dependency in $buildMode mode'); + + // 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', + ]); + + final File apk = File(path.join(flutterProject.rootPath, 'build', 'app', 'outputs', 'flutter-apk', 'app-debug.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) { + return TaskResult.failure('Expected to${isTestingReleaseMode ? ' not' : ''} find dev_dependency_plugin in APK built with debug mode but did${isTestingReleaseMode ? '' : ' not'}.'); + } + }); + } + }); + return TaskResult.success(null); + } on TaskResult catch (taskResult) { + return taskResult; + } catch (e) { + return TaskResult.failure(e.toString()); + } + }); +} diff --git a/dev/devicelab/bin/tasks/gradle_desugar_classes_test.dart b/dev/devicelab/bin/tasks/gradle_desugar_classes_test.dart index ec6cec85ad..2d6062ed35 100644 --- a/dev/devicelab/bin/tasks/gradle_desugar_classes_test.dart +++ b/dev/devicelab/bin/tasks/gradle_desugar_classes_test.dart @@ -16,7 +16,7 @@ Future main() async { await runProjectTest((FlutterProject flutterProject) async { section('APK contains plugin classes'); await flutterProject.setMinSdkVersion(20); - flutterProject.addPlugin('google_maps_flutter', value: '^2.2.1'); + await flutterProject.addPlugin('google_maps_flutter:^2.2.1'); await inDirectory(flutterProject.rootPath, () async { await flutter('build', options: [ diff --git a/dev/devicelab/bin/tasks/gradle_plugin_light_apk_test.dart b/dev/devicelab/bin/tasks/gradle_plugin_light_apk_test.dart index 332e8ac431..bf66697bc9 100644 --- a/dev/devicelab/bin/tasks/gradle_plugin_light_apk_test.dart +++ b/dev/devicelab/bin/tasks/gradle_plugin_light_apk_test.dart @@ -210,8 +210,7 @@ Future main() async { }); section('Configure'); - project.addPlugin('plugin_under_test', - value: '${Platform.lineTerminator} path: ${pluginDir.path}'); + await project.addPlugin('plugin_under_test', options: ['--path', pluginDir.path]); await project.addCustomBuildType('local', initWith: 'debug'); await project.getPackages(); @@ -229,7 +228,7 @@ Future main() async { section('gradlew assembleLocal (plugin with custom build type)'); await project.addCustomBuildType('local', initWith: 'debug'); section('Add plugin'); - project.addPlugin('path_provider'); + await project.addPlugin('path_provider'); await project.getPackages(); await project.runGradleTask('assembleLocal'); diff --git a/dev/devicelab/lib/framework/apk_utils.dart b/dev/devicelab/lib/framework/apk_utils.dart index d0ce8ee289..c2aa90e4dc 100644 --- a/dev/devicelab/lib/framework/apk_utils.dart +++ b/dev/devicelab/lib/framework/apk_utils.dart @@ -192,6 +192,17 @@ class ApkExtractor { _extracted = true; } + /// Returns true if APK contains classes from library with given [libraryName]. + Future containsLibrary(String libraryName) async { + await _extractDex(); + for (final String className in _classes) { + if (className.startsWith(libraryName)) { + return true; + } + } + return false; + } + /// Returns true if the APK contains a given class. Future containsClass(String className) async { await _extractDex(); @@ -218,6 +229,14 @@ Future getAndroidManifest(String apk) async { ); } +/// Checks that the [apk] includes any classes from a particularly library with +/// given [libraryName] in the [apk] and returns true if so, false otherwise. +Future checkApkContainsMethodsFromLibrary(File apk, String libraryName) async { + final ApkExtractor extractor = ApkExtractor(apk); + final bool apkContainsMethodsFromLibrary = await extractor.containsLibrary(libraryName); + return apkContainsMethodsFromLibrary; +} + /// Checks that the classes are contained in the APK, throws otherwise. Future checkApkContainsClasses(File apk, List classes) async { final ApkExtractor extractor = ApkExtractor(apk); @@ -272,16 +291,17 @@ android { } /// Adds a plugin to the pubspec. - /// In pubspec, each dependency is expressed as key, value pair joined by a colon `:`. - /// such as `plugin_a`:`^0.0.1` or `plugin_a`:`\npath: /some/path`. - void addPlugin(String plugin, { String value = '' }) { - final File pubspec = File(path.join(rootPath, 'pubspec.yaml')); - String content = pubspec.readAsStringSync(); - content = content.replaceFirst( - '${Platform.lineTerminator}dependencies:${Platform.lineTerminator}', - '${Platform.lineTerminator}dependencies:${Platform.lineTerminator} $plugin: $value${Platform.lineTerminator}', - ); - pubspec.writeAsStringSync(content, flush: true); + /// + /// If a particular version of the [plugin] is desired, it should be included + /// in the name as it would be in the `flutter pub add` command, e.g. + /// `google_maps_flutter:^2.2.1`. + /// + /// Include all other desired options for running `flutter pub add` to + /// [options], e.g. `['--path', 'path/to/plugin']`. + Future addPlugin(String plugin, {List options = const []}) async { + await inDirectory(Directory(rootPath), () async { + await flutter('pub', options: ['add', plugin, ...options]); + }); } Future setMinSdkVersion(int sdkVersion) async { @@ -367,9 +387,9 @@ class FlutterPluginProject { final Directory parent; final String name; - static Future create(Directory directory, String name) async { + static Future create(Directory directory, String name, {List options = const ['--platforms=ios,android']}) async { await inDirectory(directory, () async { - await flutter('create', options: ['--template=plugin', '--platforms=ios,android', name]); + await flutter('create', options: ['--template=plugin', ...options, name]); }); return FlutterPluginProject(directory, name); } diff --git a/packages/flutter_tools/gradle/src/main/groovy/flutter.groovy b/packages/flutter_tools/gradle/src/main/groovy/flutter.groovy index 3a1590feb7..f12a3b3555 100644 --- a/packages/flutter_tools/gradle/src/main/groovy/flutter.groovy +++ b/packages/flutter_tools/gradle/src/main/groovy/flutter.groovy @@ -769,10 +769,18 @@ class FlutterPlugin implements Plugin { // Apply the "flutter" Gradle extension to plugins so that they can use it's vended // compile/target/min sdk values. pluginProject.extensions.create("flutter", FlutterExtension) + // Add plugin dependency to the app project. - project.dependencies { - api(pluginProject) + project.android.buildTypes.each { buildType -> + String flutterBuildMode = buildModeFor(buildType) + if (flutterBuildMode != "release" || !pluginObject.dev_dependency) { + // Only add dependency on dev dependencies in non-release builds. + project.dependencies { + api(pluginProject) + } + } } + Closure addEmbeddingDependencyToPlugin = { buildType -> String flutterBuildMode = buildModeFor(buildType) // In AGP 3.5, the embedding must be added as an API implementation, @@ -784,6 +792,12 @@ class FlutterPlugin implements Plugin { if (!pluginProject.hasProperty("android")) { return } + if (flutterBuildMode == "release" && pluginObject.dev_dependency) { + // This plugin is a dev dependency and will not be included in + // the release build, so no need to add the embedding + // dependency to it. + return + } // Copy build types from the app to the plugin. // This allows to build apps with plugins and custom build types or flavors. pluginProject.android.buildTypes { @@ -944,20 +958,29 @@ class FlutterPlugin implements Plugin { if (pluginProject == null) { return } - def dependencies = pluginObject.dependencies - assert(dependencies instanceof List) - dependencies.each { pluginDependencyName -> - if (pluginDependencyName.empty) { + + project.android.buildTypes.each { buildType -> + String flutterBuildMode = buildModeFor(buildType) + if (flutterBuildMode == "release" && pluginObject.dev_dependency) { + // This plugin is a dev dependency will not be included in the + // release build, so no need to add its dependencies. return } - Project dependencyProject = project.rootProject.findProject(":$pluginDependencyName") - if (dependencyProject == null) { - return - } - // Wait for the Android plugin to load and add the dependency to the plugin project. - pluginProject.afterEvaluate { - pluginProject.dependencies { - implementation(dependencyProject) + def dependencies = pluginObject.dependencies + assert(dependencies instanceof List) + dependencies.each { pluginDependencyName -> + if (pluginDependencyName.empty) { + return + } + Project dependencyProject = project.rootProject.findProject(":$pluginDependencyName") + if (dependencyProject == null) { + return + } + // Wait for the Android plugin to load and add the dependency to the plugin project. + pluginProject.afterEvaluate { + pluginProject.dependencies { + implementation(dependencyProject) + } } } } diff --git a/packages/flutter_tools/gradle/src/main/groovy/native_plugin_loader.groovy b/packages/flutter_tools/gradle/src/main/groovy/native_plugin_loader.groovy index 1ca726ac60..177fb87bbf 100644 --- a/packages/flutter_tools/gradle/src/main/groovy/native_plugin_loader.groovy +++ b/packages/flutter_tools/gradle/src/main/groovy/native_plugin_loader.groovy @@ -19,9 +19,10 @@ class NativePluginLoader { * "path": "/path/to/plugin-a", * "dependencies": ["plugin-b", "plugin-c"], * "native_build": true + * "dev_dependency": false * } * - * Therefore the map value can either be a `String`, a `List` or a `boolean`. + * Therefore the map value can either be a `String`, a `List` or a `Boolean`. */ List> getPlugins(File flutterSourceDirectory) { List> nativePlugins = [] @@ -40,6 +41,7 @@ class NativePluginLoader { assert(androidPlugin.name instanceof String) assert(androidPlugin.path instanceof String) assert(androidPlugin.dependencies instanceof List) + assert(androidPlugin.dev_dependency instanceof Boolean) // Skip plugins that have no native build (such as a Dart-only implementation // of a federated plugin). def needsBuild = androidPlugin.containsKey(nativeBuildKey) ? androidPlugin[nativeBuildKey] : true @@ -66,18 +68,28 @@ class NativePluginLoader { // "path": "/path/to/plugin-a", // "dependencies": ["plugin-b", "plugin-c"], // "native_build": true + // "dev_dependency": false // }, // { // "name": "plugin-b", // "path": "/path/to/plugin-b", // "dependencies": ["plugin-c"], // "native_build": true + // "dev_dependency": false // }, // { // "name": "plugin-c", // "path": "/path/to/plugin-c", // "dependencies": [], // "native_build": true + // "dev_dependency": false + // }, + // { + // "name": "plugin-d", + // "path": "/path/to/plugin-d", + // "dependencies": [], + // "native_build": true + // "dev_dependency": true // }, // ], // }, @@ -93,12 +105,18 @@ class NativePluginLoader { // { // "name": "plugin-c", // "dependencies": [] + // }, + // { + // "name": "plugin-d", + // "dependencies": [] // } // ] // } // This means, `plugin-a` depends on `plugin-b` and `plugin-c`. // `plugin-b` depends on `plugin-c`. // `plugin-c` doesn't depend on anything. + // `plugin-d` also doesn't depend on anything, but it is a dev + // dependency to the Flutter project, so it is marked as such. if (parsedFlutterPluginsDependencies) { return parsedFlutterPluginsDependencies }