From 2dad95b21c15e41902f2ea0645d613246b364bce Mon Sep 17 00:00:00 2001 From: Andrew Kolos Date: Tue, 15 Oct 2024 10:39:01 -0700 Subject: [PATCH] Fix flavor-conditional asset bundling for path dependencies (#156687) Fixes https://github.com/flutter/flutter/issues/155755 When building the asset bundle during, the `--flavor` option isn't considered when searching for assets from dependencies. This PR fixes that. It's possible that when initially implementing this feature, I thought that flavor-conditional assets didn't make sense for packages. While I still think that way regarding pub packages, using this feature makes a lot more sense for monorepo projects.
Pre-launch checklist
--- packages/flutter_tools/lib/src/asset.dart | 3 +- .../asset_bundle_package_test.dart | 99 +++++++++++++++++-- 2 files changed, 94 insertions(+), 8 deletions(-) diff --git a/packages/flutter_tools/lib/src/asset.dart b/packages/flutter_tools/lib/src/asset.dart index 97fced1cb02..549e5c37183 100644 --- a/packages/flutter_tools/lib/src/asset.dart +++ b/packages/flutter_tools/lib/src/asset.dart @@ -380,6 +380,7 @@ class ManifestAssetBundle implements AssetBundle { targetPlatform, packageName: package.name, attributedPackage: package, + flavor: flavor, ); if (packageAssets == null) { @@ -873,7 +874,7 @@ class ManifestAssetBundle implements AssetBundle { TargetPlatform? targetPlatform, { String? packageName, Package? attributedPackage, - String? flavor, + required String? flavor, }) { final Map<_Asset, List<_Asset>> result = <_Asset, List<_Asset>>{}; diff --git a/packages/flutter_tools/test/general.shard/asset_bundle_package_test.dart b/packages/flutter_tools/test/general.shard/asset_bundle_package_test.dart index 3554e0b15f3..37c3221d42a 100644 --- a/packages/flutter_tools/test/general.shard/asset_bundle_package_test.dart +++ b/packages/flutter_tools/test/general.shard/asset_bundle_package_test.dart @@ -27,9 +27,14 @@ void main() { return path.replaceAll('/', globals.fs.path.separator); } - void writePubspecFile(String path, String name, { List? assets }) { + void writePubspecFile( + String path, + String name, { + List? assets, + List<(String path, String flavor)>? flavoredAssets, + }) { String assetsSection; - if (assets == null) { + if (assets == null && flavoredAssets == null) { assetsSection = ''; } else { final StringBuffer buffer = StringBuffer(); @@ -38,11 +43,20 @@ flutter: assets: '''); - for (final String asset in assets) { + for (final String asset in (assets ?? [])) { buffer.write(''' - $asset '''); } + + for (final (String path, String flavor) in flavoredAssets ?? <(String, String)>[]) { + buffer.write(''' + - path: $path + flavors: + - $flavor +'''); + } + assetsSection = buffer.toString(); } @@ -57,7 +71,7 @@ $assetsSection '''); } -void writePackageConfigFile(Map packages) { + void writePackageConfigFile(Map packages) { globals.fs.directory('.dart_tool').childFile('package_config.json') ..createSync(recursive: true) ..writeAsStringSync( @@ -89,11 +103,15 @@ void writePackageConfigFile(Map packages) { Future buildAndVerifyAssets( List assets, List packages, - Map expectedAssetManifest - ) async { + Map expectedAssetManifest, { + String? flavor, + }) async { final AssetBundle bundle = AssetBundleFactory.instance.createBundle(); - await bundle.build(packageConfigPath: '.dart_tool/package_config.json'); + await bundle.build( + packageConfigPath: '.dart_tool/package_config.json', + flavor: flavor, + ); for (final String packageName in packages) { for (final String asset in assets) { @@ -532,6 +550,73 @@ void writePackageConfigFile(Map packages) { FileSystem: () => testFileSystem, ProcessManager: () => FakeProcessManager.any(), }); + + testUsingContext('Flavored assets are bundled when the app depends on a package', () async { + writePubspecFile( + 'pubspec.yaml', + 'test', + ); + writePackageConfigFile( + { + 'test_package': 'p/p/', + }, + ); + writePubspecFile( + 'p/p/pubspec.yaml', + 'test_package', + flavoredAssets: <(String, String)>[('assets/vanilla.txt', 'vanilla')], + ); + + final List assets = ['assets/vanilla.txt']; + writeAssets('p/p', assets); + + const Map expectedAssetManifest = { + 'packages/test_package/assets/vanilla.txt': >[ + {'asset': 'packages/test_package/assets/vanilla.txt'}, + ] + }; + + await buildAndVerifyAssets( + assets, + ['test_package'], + expectedAssetManifest, + flavor: 'vanilla', + ); + }, overrides: { + FileSystem: () => testFileSystem, + ProcessManager: () => FakeProcessManager.any(), + }); + }); + + testUsingContext('Asset paths can contain URL reserved characters', () async { + writePubspecFile('pubspec.yaml', 'test'); + writePackageConfigFile({'test_package': 'p/p/'}); + + final List assets = ['a/foo', 'a/foo [x]']; + writePubspecFile( + 'p/p/pubspec.yaml', + 'test_package', + assets: assets, + ); + + writeAssets('p/p/', assets); + const Map expectedAssetManifest = { + 'packages/test_package/a/foo': >[ + {'asset': 'packages/test_package/a/foo'} + ], + 'packages/test_package/a/foo [x]': >[ + {'asset': 'packages/test_package/a/foo [x]'} + ] + }; + + await buildAndVerifyAssets( + assets, + ['test_package'], + expectedAssetManifest, + ); + }, overrides: { + FileSystem: () => testFileSystem, + ProcessManager: () => FakeProcessManager.any(), }); testUsingContext('Asset paths can contain URL reserved characters', () async {