Skip to content
Merged
2 changes: 1 addition & 1 deletion .cirrus.yml
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ task:
license_script: $PLUGIN_TOOL_COMMAND license-check
# The major and minor version here should match the lowest version
# analyzed in legacy_version_analyze.
pubspec_script: ./script/tool_runner.sh pubspec-check --min-min-flutter-version=3.0.0
pubspec_script: ./script/tool_runner.sh pubspec-check --min-min-flutter-version=3.0.0 --allow-dependencies=script/configs/allowed_unpinned_deps.yaml --allow-pinned-dependencies=script/configs/allowed_pinned_deps.yaml
readme_script:
- ./script/tool_runner.sh readme-check
# Re-run with --require-excerpts, skipping packages that still need
Expand Down
2 changes: 1 addition & 1 deletion packages/camera/camera_web/example/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,4 @@ dev_dependencies:
sdk: flutter
integration_test:
sdk: flutter
mocktail: ^0.3.0
mocktail: 0.3.0
Copy link
Member

@ditman ditman Mar 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

giphy

(My bad, this should have never happened in the first place)

4 changes: 2 additions & 2 deletions packages/go_router/example/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ environment:
flutter: ">=3.3.0"

dependencies:
adaptive_dialog: ^1.2.0
adaptive_dialog: 1.8.2
adaptive_navigation: ^0.0.4
collection: ^1.15.0
cupertino_icons: ^1.0.2
Expand All @@ -17,7 +17,7 @@ dependencies:
go_router:
path: ..
logging: ^1.0.0
provider: ^6.0.0
provider: 6.0.5
shared_preferences: ^2.0.11
url_launcher: ^6.0.7

Expand Down
2 changes: 1 addition & 1 deletion packages/go_router_builder/example/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ dependencies:
flutter:
sdk: flutter
go_router: ^6.0.0
provider: ^6.0.0
provider: 6.0.5

dev_dependencies:
build_runner: ^2.0.0
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ dependencies:
path: ../
http: ^0.13.5
js: ^0.6.4
jwt_decoder: ^2.0.1
jwt_decoder: 2.0.1

dev_dependencies:
build_runner: ^2.1.10 # To extract README excerpts only.
Expand Down
2 changes: 1 addition & 1 deletion packages/rfw/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,4 @@ dependencies:
dev_dependencies:
flutter_test:
sdk: flutter
lcov_parser: ^0.1.2
lcov_parser: 0.1.2
16 changes: 16 additions & 0 deletions script/configs/allowed_pinned_deps.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# The list of external dependencies that are allowed as unpinned dependencies.
# See https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#Dependencies
#
# All entries here should have an explanation for why they are here.

# TODO(stuartmorgan): Eliminate this in favor of standardizing on
# mockito.
- mocktail

# Legacy allowances, for dependencies that predate the tooling.
# TODO(stuartmorgan): Audit these. See
# https://github.com/flutter/flutter/issues/122713
- jwt_decoder
- lcov_parser
- adaptive_dialog
- provider
78 changes: 78 additions & 0 deletions script/configs/allowed_unpinned_deps.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
# The list of external dependencies that are allowed as pinned dependencies.
# See https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#Dependencies
#
# All entries here should have an explanation for why they are here, either
# via being part of one of the default-allowed groups, or a specific reason.

## TODO(stuartmorgan): Move these to pinned-only.
# Google-owned
- file_testing
# Dart-owned
- mockito

## Legacy allowances, for dependencies that predate the tooling.
# TODO(stuartmorgan): Audit these. See
# https://github.com/flutter/flutter/issues/122713
- equatable
- xml

## Explicit allowances

# Owned by individual Flutter Team members.
# Ideally we would not do this, since there's no clear plan for what
# would happen if the individuals left the Flutter Team, and the
# repositories may or may not meet Flutter's security standards. Be
# cautious about adding to this list.
- build_verify
- google_maps
- path_to_regexp
- source_gen_test
- win32

## Allowed by default

# Dart-team-owned packages
- analyzer
- args
- async
- build
- build_config
- build_runner
- collection
- convert
- crypto
- fake_async
- ffi
- gcloud
- html
- http
- intl
- js
- json_serializable
- lints
- logging
- markdown
- meta
- path
- shelf
- shelf_static
- source_gen
- stream_transform
- test
- test_api
- vm_service
- wasm
- yaml
# Google-owned packages
- adaptive_navigation
- file
- googleapis
- googleapis_auth
- json_annotation
- platform
- process
- quiver
- sanitize_html
- source_helper
- vector_math
- webkit_inspection_protocol
111 changes: 111 additions & 0 deletions script/tool/lib/src/pubspec_check_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@

import 'package:file/file.dart';
import 'package:git/git.dart';
import 'package:path/path.dart' as p;
import 'package:platform/platform.dart';
import 'package:pub_semver/pub_semver.dart';
import 'package:pubspec_parse/pubspec_parse.dart';
import 'package:yaml/yaml.dart';

import 'common/core.dart';
Expand Down Expand Up @@ -36,9 +38,24 @@ class PubspecCheckCommand extends PackageLoopingCommand {
help:
'The minimum Flutter version to allow as the minimum SDK constraint.',
);
argParser.addMultiOption(_allowDependenciesFlag,
help: 'Packages (comma separated) that are allowed as dependencies or '
'dev_dependencies.\n\n'
'Alternately, a list of one or more YAML files that contain a list '
'of allowed dependencies.',
defaultsTo: <String>[]);
argParser.addMultiOption(_allowPinnedDependenciesFlag,
help: 'Packages (comma separated) that are allowed as dependencies or '
'dev_dependencies only if pinned to an exact version.\n\n'
'Alternately, a list of one or more YAML files that contain a list '
'of allowed pinned dependencies.',
defaultsTo: <String>[]);
}

static const String _minMinFlutterVersionFlag = 'min-min-flutter-version';
static const String _allowDependenciesFlag = 'allow-dependencies';
static const String _allowPinnedDependenciesFlag =
'allow-pinned-dependencies';

// Section order for plugins. Because the 'flutter' section is critical
// information for plugins, and usually small, it goes near the top unlike in
Expand All @@ -62,6 +79,13 @@ class PubspecCheckCommand extends PackageLoopingCommand {
static const String _expectedIssueLinkFormat =
'https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A';

// The names of all published packages in the repository.
late final Set<String> _localPackages = <String>{};

// Packages on the explicit allow list.
late final Set<String> _allowedUnpinnedPackages = <String>{};
late final Set<String> _allowedPinnedPackages = <String>{};

@override
final String name = 'pubspec-check';

Expand All @@ -76,6 +100,40 @@ class PubspecCheckCommand extends PackageLoopingCommand {
PackageLoopingType get packageLoopingType =>
PackageLoopingType.includeAllSubpackages;

@override
Future<void> initializeRun() async {
// Find all local, published packages.
await for (final File pubspecFile in packagesDir.parent
.list(recursive: true, followLinks: false)
.where((FileSystemEntity entity) =>
entity is File && p.basename(entity.path) == 'pubspec.yaml')
.map((FileSystemEntity file) => file as File)) {
final Pubspec? pubspec = _tryParsePubspec(pubspecFile.readAsStringSync());
if (pubspec != null && pubspec.publishTo != 'none') {
_localPackages.add(pubspec.name);
}
}
// Load explicitly allowed packages.
_allowedUnpinnedPackages
.addAll(_getAllowedPackages(_allowDependenciesFlag));
_allowedPinnedPackages
.addAll(_getAllowedPackages(_allowPinnedDependenciesFlag));
}

Iterable<String> _getAllowedPackages(String flag) {
return getStringListArg(flag).expand<String>((String item) {
if (item.endsWith('.yaml')) {
final File file = packagesDir.fileSystem.file(item);
final Object? yaml = loadYaml(file.readAsStringSync());
if (yaml == null) {
return <String>[];
}
return (yaml as YamlList).toList().cast<String>();
}
return <String>[item];
});
}

@override
Future<PackageResult> runForPackage(RepositoryPackage package) async {
final File pubspec = package.pubspecFile;
Expand Down Expand Up @@ -139,6 +197,12 @@ class PubspecCheckCommand extends PackageLoopingCommand {
}
}

final String? dependenciesError = _checkDependencies(pubspec);
if (dependenciesError != null) {
printError('$indentation$dependenciesError');
passing = false;
}

// Ignore metadata that's only relevant for published packages if the
// packages is not intended for publishing.
if (pubspec.publishTo != 'none') {
Expand Down Expand Up @@ -426,4 +490,51 @@ class PubspecCheckCommand extends PackageLoopingCommand {
}
return result ?? Version.none;
}

// Validates the dependencies for a package, returning an error string if
// there are any that aren't allowed.
String? _checkDependencies(Pubspec pubspec) {
final Set<String> badDependencies = <String>{};
for (final Map<String, Dependency> dependencies
in <Map<String, Dependency>>[
pubspec.dependencies,
pubspec.devDependencies
]) {
for (final MapEntry<String, Dependency> entry in dependencies.entries) {
final String name = entry.key;
if (!_allowDependency(name, entry.value)) {
badDependencies.add(name);
}
}
}
if (badDependencies.isEmpty) {
return null;
}
return 'The following unexpected non-local dependencies were found:\n'
'${badDependencies.map((String name) => ' $name').join('\n')}\n'
'Please see https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#Dependencies '
'for more information and next steps.';
}

// Checks whether a given dependency is allowed.
bool _allowDependency(String name, Dependency dependency) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method name seems a little off from what I would expect. Something like _isDependancyAllowed or something similar.

As is I would expect this to add the dep to an allow list. Up to you if you agree.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, changed to _shouldAllowDependency

if (dependency is PathDependency || dependency is SdkDependency) {
return true;
}
if (_localPackages.contains(name) ||
_allowedUnpinnedPackages.contains(name)) {
return true;
}
if (dependency is HostedDependency &&
_allowedPinnedPackages.contains(name)) {
final VersionConstraint constraint = dependency.version;
if (constraint is VersionRange &&
constraint.min != null &&
constraint.max != null &&
constraint.min == constraint.max) {
return true;
}
}
return false;
}
}
Loading