From d9144bf8fce1ea59ae66f8476586883a10b2d41f Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Fri, 29 May 2020 11:55:02 -0700 Subject: [PATCH] [flutter_tools] rename output LICENSE file to NOTICES and support loading either (#57871) Work towards #16723 This is only safe to land after #58131 lands in google3. Only build NOTICES in asset manfiest, and load either LICENSE or NOTICES from pubspec dependencies. --- dev/devicelab/lib/framework/apk_utils.dart | 2 +- dev/devicelab/lib/tasks/perf_tests.dart | 2 +- packages/flutter_tools/lib/src/asset.dart | 49 +++++++++---------- .../test/general.shard/asset_bundle_test.dart | 4 +- .../build_system/targets/assets_test.dart | 2 +- .../general.shard/license_collector_test.dart | 15 ++++-- 6 files changed, 40 insertions(+), 34 deletions(-) diff --git a/dev/devicelab/lib/framework/apk_utils.dart b/dev/devicelab/lib/framework/apk_utils.dart index e272144f5b..63415a6024 100644 --- a/dev/devicelab/lib/framework/apk_utils.dart +++ b/dev/devicelab/lib/framework/apk_utils.dart @@ -11,7 +11,7 @@ import 'package:flutter_devicelab/framework/utils.dart'; final List flutterAssets = [ 'assets/flutter_assets/AssetManifest.json', - 'assets/flutter_assets/LICENSE', + 'assets/flutter_assets/NOTICES', 'assets/flutter_assets/fonts/MaterialIcons-Regular.ttf', 'assets/flutter_assets/packages/cupertino_icons/assets/CupertinoIcons.ttf', ]; diff --git a/dev/devicelab/lib/tasks/perf_tests.dart b/dev/devicelab/lib/tasks/perf_tests.dart index 473563c001..40811b0f97 100644 --- a/dev/devicelab/lib/tasks/perf_tests.dart +++ b/dev/devicelab/lib/tasks/perf_tests.dart @@ -544,7 +544,7 @@ class CompileTest { final _UnzipListEntry libflutter = fileToMetadata['lib/armeabi-v7a/libflutter.so']; final _UnzipListEntry libapp = fileToMetadata['lib/armeabi-v7a/libapp.so']; - final _UnzipListEntry license = fileToMetadata['assets/flutter_assets/LICENSE']; + final _UnzipListEntry license = fileToMetadata['assets/flutter_assets/NOTICES']; return { 'libflutter_uncompressed_bytes': libflutter.uncompressedSize, diff --git a/packages/flutter_tools/lib/src/asset.dart b/packages/flutter_tools/lib/src/asset.dart index d7d1fc109c..ce49e34c30 100644 --- a/packages/flutter_tools/lib/src/asset.dart +++ b/packages/flutter_tools/lib/src/asset.dart @@ -82,9 +82,9 @@ class ManifestAssetBundle implements AssetBundle { DateTime _lastBuildTimestamp; - static const String _assetManifestJson = 'AssetManifest.json'; - static const String _fontSetMaterial = 'material'; - static const String _license = 'LICENSE'; + static const String _kAssetManifestJson = 'AssetManifest.json'; + static const String _kFontSetMaterial = 'material'; + static const String _kNoticeFile = 'NOTICES'; @override bool wasBuiltOnce() => _lastBuildTimestamp != null; @@ -149,7 +149,7 @@ class ManifestAssetBundle implements AssetBundle { // device. _lastBuildTimestamp = DateTime.now(); if (flutterManifest.isEmpty) { - entries[_assetManifestJson] = DevFSStringContent('{}'); + entries[_kAssetManifestJson] = DevFSStringContent('{}'); return 0; } @@ -254,7 +254,7 @@ class ManifestAssetBundle implements AssetBundle { final List<_Asset> materialAssets = <_Asset>[ if (flutterManifest.usesMaterialDesign && includeDefaultFonts) - ..._getMaterialAssets(_fontSetMaterial), + ..._getMaterialAssets(_kFontSetMaterial), ]; for (final _Asset asset in materialAssets) { assert(asset.assetFileExists); @@ -285,9 +285,9 @@ class ManifestAssetBundle implements AssetBundle { globals.fs.file('DOES_NOT_EXIST_RERUN_FOR_WILDCARD$suffix').absolute); } - _setIfChanged(_assetManifestJson, assetManifest); + _setIfChanged(_kAssetManifestJson, assetManifest); _setIfChanged(kFontManifestJson, fontManifest); - _setIfChanged(_license, licenses); + _setIfChanged(_kNoticeFile, licenses); return 0; } @@ -395,28 +395,23 @@ List<_Asset> _getMaterialAssets(String fontSet) { } -/// Processes dependencies into a string representing the license file. +/// Processes dependencies into a string representing the NOTICES file. /// -/// Reads the LICENSE file from each package in the .packages file, splitting -/// each one into each component license (so that we can de-dupe if possible). -/// If provided with a pubspec.yaml file, only direct depedencies are included -/// in the resulting LICENSE file. +/// Reads the NOTICES or LICENSE file from each package in the .packages file, +/// splitting each one into each component license so that it can be de-duped +/// if possible. If the NOTICES file exists, it is preferred over the LICENSE +/// file. /// /// Individual licenses inside each LICENSE file should be separated by 80 /// hyphens on their own on a line. /// -/// If a LICENSE file contains more than one component license, then each -/// component license must start with the names of the packages to which the -/// component license applies, with each package name on its own line, and the -/// list of package names separated from the actual license text by a blank -/// line. (The packages need not match the names of the pub package. For +/// If a LICENSE or NOTICES file contains more than one component license, +/// then each component license must start with the names of the packages to +/// which the component license applies, with each package name on its own line +/// and the list of package names separated from the actual license text by a +/// blank line. The packages need not match the names of the pub package. For /// example, a package might itself contain code from multiple third-party -/// sources, and might need to include a license for each one.) -// Note: this logic currently has a bug, in that we collect LICENSE information -// for dev_dependencies and transitive dev_dependencies. These are not actually -// compiled into the released application and don't need to be included. Unfortunately, -// the pubspec.yaml alone does not have enough information to determine which -// dependencies are transitive of dev_dependencies, so a simple filter isn't sufficient. +/// sources, and might need to include a license for each one. class LicenseCollector { LicenseCollector({ @required FileSystem fileSystem @@ -440,7 +435,11 @@ class LicenseCollector { if (packageUri == null || packageUri.scheme != 'file') { continue; } - final File file = _fileSystem.file(packageUri.resolve('../LICENSE')); + // First check for NOTICES, then fallback to LICENSE + File file = _fileSystem.file(packageUri.resolve('../NOTICES')); + if (!file.existsSync()) { + file = _fileSystem.file(packageUri.resolve('../LICENSE')); + } if (!file.existsSync()) { continue; } @@ -527,7 +526,7 @@ List> _parseFonts( }) { return >[ if (manifest.usesMaterialDesign && includeDefaultFonts) - ..._getMaterialFonts(ManifestAssetBundle._fontSetMaterial), + ..._getMaterialFonts(ManifestAssetBundle._kFontSetMaterial), if (packageName == null) ...manifest.fontsDescriptor else diff --git a/packages/flutter_tools/test/general.shard/asset_bundle_test.dart b/packages/flutter_tools/test/general.shard/asset_bundle_test.dart index a50127d524..20f7bc030d 100644 --- a/packages/flutter_tools/test/general.shard/asset_bundle_test.dart +++ b/packages/flutter_tools/test/general.shard/asset_bundle_test.dart @@ -211,14 +211,14 @@ assets: as DevFSStringContent; final DevFSStringContent fontManifest = bundle.entries['FontManifest.json'] as DevFSStringContent; - final DevFSStringContent license = bundle.entries['LICENSE'] + final DevFSStringContent license = bundle.entries['NOTICES'] as DevFSStringContent; await bundle.build(manifestPath: 'pubspec.yaml'); expect(assetManifest, bundle.entries['AssetManifest.json']); expect(fontManifest, bundle.entries['FontManifest.json']); - expect(license, bundle.entries['LICENSE']); + expect(license, bundle.entries['NOTICES']); }, overrides: { FileSystem: () => MemoryFileSystem.test(), ProcessManager: () => FakeProcessManager.any(), diff --git a/packages/flutter_tools/test/general.shard/build_system/targets/assets_test.dart b/packages/flutter_tools/test/general.shard/build_system/targets/assets_test.dart index fe19fec197..a4b801837c 100644 --- a/packages/flutter_tools/test/general.shard/build_system/targets/assets_test.dart +++ b/packages/flutter_tools/test/general.shard/build_system/targets/assets_test.dart @@ -89,7 +89,7 @@ flutter: expect(fileSystem.file('${environment.buildDir.path}/flutter_assets/AssetManifest.json'), exists); expect(fileSystem.file('${environment.buildDir.path}/flutter_assets/FontManifest.json'), exists); - expect(fileSystem.file('${environment.buildDir.path}/flutter_assets/LICENSE'), exists); + expect(fileSystem.file('${environment.buildDir.path}/flutter_assets/NOTICES'), exists); // See https://github.com/flutter/flutter/issues/35293 expect(fileSystem.file('${environment.buildDir.path}/flutter_assets/assets/foo/bar.png'), exists); // See https://github.com/flutter/flutter/issues/46163 diff --git a/packages/flutter_tools/test/general.shard/license_collector_test.dart b/packages/flutter_tools/test/general.shard/license_collector_test.dart index a1f5c319d4..4709c1e95b 100644 --- a/packages/flutter_tools/test/general.shard/license_collector_test.dart +++ b/packages/flutter_tools/test/general.shard/license_collector_test.dart @@ -249,12 +249,16 @@ void main() { }); testWithoutContext('processes dependent licenses according to instructions', () async { - fileSystem.file('foo/LICENSE') + fileSystem.file('foo/NOTICES') ..createSync(recursive: true) ..writeAsStringSync(_kMitLicense); - fileSystem.file('bar/LICENSE') + fileSystem.file('bar/NOTICES') ..createSync(recursive: true) ..writeAsStringSync(_kApacheLicense); + // NOTICES is preferred over LICENSE + fileSystem.file('bar/LICENSE') + ..createSync(recursive: true) + ..writeAsStringSync('SHOULD NOT BE INCLUDED'); fileSystem.file('fizz/LICENSE') ..createSync(recursive: true) ..writeAsStringSync(_kMitLicense); // intentionally a duplicate @@ -292,14 +296,17 @@ void main() { expect(result.combinedLicenses, contains(_kApacheLicense)); expect(result.combinedLicenses, contains(_kMitLicense)); + // String from LICENSE file was not included when NOTICES exists. + expect(result.combinedLicenses, isNot(contains('SHOULD NOT BE INCLUDED'))); + // Licenses are de-duplicated correctly. expect(result.combinedLicenses.split(LicenseCollector.licenseSeparator), hasLength(2)); // All input licenses included in result. final Iterable filePaths = result.dependencies.map((File file) => file.path); expect(filePaths, unorderedEquals([ - '/foo/LICENSE', - '/bar/LICENSE', + '/foo/NOTICES', + '/bar/NOTICES', '/fizz/LICENSE' ])); });