[about] Add excludePackages parameter to license display APIs#186632
[about] Add excludePackages parameter to license display APIs#186632xlash5 wants to merge 6 commits into
Conversation
Tool side: LicenseCollector.obtainLicenses() now accepts an optional allowedPackageNames parameter to filter which packages' licenses are collected into NOTICES.Z. The set is built from transitive dependencies, excluding dev-only packages (unless includeAssetsFromDevDependencies is set, as for flutter test). Framework side: All license-displaying entry points (AboutListTile, showAboutDialog, showAdaptiveAboutDialog, AboutDialog, showLicensePage, LicensePage) gain an optional excludePackages parameter. Packages listed are removed at display time via _LicenseData.filtered(), which also drops orphaned licenses. Fixes flutter#100746
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request introduces an excludePackages parameter to several widgets and functions, including AboutListTile, AboutDialog, and LicensePage, allowing specific packages to be omitted from the license list. The LicenseCollector in flutter_tools is also updated to support package filtering. Feedback was provided to optimize the _LicenseData.filtered method from O(N^2) to O(N) complexity and to ensure data consistency for the firstPackage property.
| _LicenseData filtered(Set<String> excludePackages) { | ||
| final _LicenseData result = _LicenseData() | ||
| ..firstPackage = firstPackage; | ||
| for (final String package in packages) { | ||
| if (excludePackages.contains(package)) { | ||
| continue; | ||
| } | ||
| result.packages.add(package); | ||
| final List<int> bindings = packageLicenseBindings[package]!; | ||
| result.packageLicenseBindings[package] = <int>[]; | ||
| for (final int licenseIndex in bindings) { | ||
| final LicenseEntry license = licenses[licenseIndex]; | ||
| int newIndex = result.licenses.indexOf(license); | ||
| if (newIndex == -1) { | ||
| newIndex = result.licenses.length; | ||
| result.licenses.add(license); | ||
| } | ||
| result.packageLicenseBindings[package]!.add(newIndex); | ||
| } | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
The filtered method can be optimized to avoid indexOf inside a nested loop. Using a map to track license indices reduces this to firstPackage property should be cleared if it is among the excluded packages to maintain data consistency.
_LicenseData filtered(Set<String> excludePackages) {
final _LicenseData result = _LicenseData();
if (firstPackage != null && !excludePackages.contains(firstPackage)) {
result.firstPackage = firstPackage;
}
final Map<int, int> oldToNewIndex = <int, int>{};
for (final String package in packages) {
if (excludePackages.contains(package)) {
continue;
}
result.packages.add(package);
final List<int> bindings = packageLicenseBindings[package]!;
final List<int> newBindings = <int>[];
for (final int licenseIndex in bindings) {
int? newIndex = oldToNewIndex[licenseIndex];
if (newIndex == null) {
newIndex = result.licenses.length;
result.licenses.add(licenses[licenseIndex]);
oldToNewIndex[licenseIndex] = newIndex;
}
newBindings.add(newIndex);
}
result.packageLicenseBindings[package] = newBindings;
}
return result;
}…ency Replace indexOf inside the inner loop with a Map<LicenseEntry, int> to track already-seen licenses, reducing complexity from O(N^2) to O(N). Clear firstPackage when it is among the excluded packages to maintain data consistency.
|
I am not sure this is something we are able to do, let me inquire. |
|
Guidance is: As long as this only excludes development-only dependencies (tools used to build software but which do not get compiled into the software being built in any way), then that is totally fine. Only libraries that actually get incorporated into the distributed software are in scope for open source attribution. So, excluding other packages or transitive dependencies cannot be allowed, but excluding only dev dependencies should be ok. |
Per review feedback, the excludePackages API on the framework side (AboutListTile, showAboutDialog, showAdaptiveAboutDialog, AboutDialog, LicensePage, etc.) gives users the ability to exclude arbitrary packages, not just dev dependencies. This contradicts the guidance that only development-only dependencies should be excluded. The tool-side automatic filtering (allowedPackageNames in LicenseCollector based on transitiveDependencies) handles this correctly at build time and is kept as-is.
The previous revert commit left minor formatting differences (multi-line constructor calls, extra blank lines). This commit applies the exact master versions so these two files are byte-for-byte identical to upstream/master.
| ); | ||
| final DevFSByteContent assetManifestBinary = _createAssetManifestBinary(assetManifest); | ||
| final fontManifest = DevFSStringContent(json.encode(fonts)); | ||
| final Set<String> allowedPackageNames = transitiveDependencies |
There was a problem hiding this comment.
I think we explicitly need to test this behavior to ensure we're actually only omitting dev dependencies. The existing tests only check that the filtering mechanism works, not that we're filtering the correct dependencies.
There was a problem hiding this comment.
Yes we should make sure we do not leave a potential gap to deviate from the guidance above and only have the option to exclude dev dependencies.
|
Can you take a look at the failing checks here? |
The method chain in asset.dart was indented 6 spaces instead of the required 8. The testWithoutContext call in license_collector_test.dart needed wrapping with a trailing comma and proper re-indentation of cascade chains.
…licenses The existing allowedPackageNames tests only verified the filtering mechanism works, not that the correct set of dependencies is being filtered. Add two tests that exercise the exact production logic from asset.dart:637-640: 1. Ensures dev-exclusive deps (isExclusiveDevDependency: true) are excluded while regular deps are included. 2. Ensures all deps are included when includeAssetsFromDevDependencies is true (test runner path).
|
@Piinks checks are looking fine now. Also added additional tests for allowedPackageNames as you suggested |
Fixes #100746
Summary
Two complementary changes to exclude dev dependencies from the OSS license acknowledgement page.
Tool Side (Build-Time)
LicenseCollector.obtainLicenses()now accepts an optionalallowedPackageNamesparameter.ManifestAssetBundle.build()builds the set fromtransitiveDependencies, filtering out exclusive dev dependencies (unlessincludeAssetsFromDevDependenciesis true, as forflutter test).Framework Side (Run-Time)
All license-displaying entry points gain an optional
excludePackagesparameter:AboutListTileshowAboutDialog()/showAdaptiveAboutDialog()AboutDialog/AboutDialog.adaptiveshowLicensePage()LicensePageThe parameter flows down to
_PackagesViewwhich applies_LicenseData.filtered()after loading, removing excluded packages and their orphaned licenses.Tests
Tool Side (license_collector_test.dart)
allowedPackageNames filters packagesnull allowedPackageNames includes all packagesempty allowedPackageNames excludes all packagesallowedPackageNames excludes additional licenses for non-allowed packagesFramework Side (about_test.dart)
LicensePage excludePackages hides packagesLicensePage excludePackages null includes allshowLicensePage passes excludePackages to LicensePageAboutDialog excludePackages flows to license pageAboutListTile excludePackages flows to dialog