From 8cba879f46cb6626f10c1b570f0cf826e7ffd7a6 Mon Sep 17 00:00:00 2001 From: Devon Carew Date: Wed, 2 Dec 2020 17:27:18 +0000 Subject: [PATCH] Add additional validations to the pkg/ package pubspecs. TEST=these are additional validations that we run on the bots Redux of https://dart-review.googlesource.com/c/sdk/+/161040 Change-Id: Ia32ced5d48fbfeafacfa9e51dc4774d2e9425091 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/174601 Reviewed-by: Konstantin Shcheglov Commit-Queue: Devon Carew --- pkg/README.md | 43 +++++++ pkg/analysis_server/pubspec.yaml | 12 +- pkg/analyzer_cli/pubspec.yaml | 10 +- pkg/analyzer_utilities/pubspec.yaml | 3 +- pkg/build_integration/pubspec.yaml | 7 +- pkg/compiler/pubspec.yaml | 11 +- pkg/dartdev/pubspec.yaml | 3 +- pkg/dev_compiler/pubspec.yaml | 9 +- pkg/expect/pubspec.yaml | 3 +- pkg/front_end/pubspec.yaml | 22 ++-- pkg/kernel/pubspec.yaml | 3 +- pkg/nnbd_migration/pubspec.yaml | 12 +- pkg/scrape/pubspec.yaml | 6 +- pkg/telemetry/pubspec.yaml | 3 +- pkg/vm/pubspec.yaml | 9 +- tools/package_deps/bin/package_deps.dart | 143 ++++++++++++++++++++--- 16 files changed, 233 insertions(+), 66 deletions(-) create mode 100644 pkg/README.md diff --git a/pkg/README.md b/pkg/README.md new file mode 100644 index 00000000000..474a204e418 --- /dev/null +++ b/pkg/README.md @@ -0,0 +1,43 @@ +# Package validation + +The packages in `pkg/` are automatically validated on the LUCI CI bots. The +validation is largely done by the `tools/package_deps` package; it can be tested +locally via: + +``` +dart tools/package_deps/bin/package_deps.dart +``` + +## Packages which are published + +There are several packages developed in `pkg/` which are published to pub. +Validation of these packages is particularly important because the pub tools are +not used for these packages during development; we get our dependency versions +from the DEPS file. Its very easy for the dependencies specified in a package's +pubspec file to get out of date wrt the packages and versions actually used. + +In order to better ensure we're publishing correct packages, we validate some +properties of the pubspec files on our CI system. These validations include: + +- that the dependencies listed in the pubspec are used in the package +- that all the packages used by the source are listed in the pubspec +- that we don't use relative path deps to pkg/ or third_party/ packages + +## Packages which are not published + +For packages in pkg/ which we do not intend to be published, we put the +following comment in the pubspec.yaml file: + +``` +# This package is not intended for consumption on pub.dev. DO NOT publish. +publish_to: none +``` + +These pubspecs are still validated by the package validation tool. The contents +are more informational as the pubspecs for these packages are not consumed by +the pub tool or ecosystem. + +We validate: +- that the dependencies listed in the pubspec are used in the package +- that all the packages used by the source are listed in the pubspec +- that a reference to a pkg/ package is done via a relative path dependency diff --git a/pkg/analysis_server/pubspec.yaml b/pkg/analysis_server/pubspec.yaml index 9a94f7dc690..ef86eeb3f37 100644 --- a/pkg/analysis_server/pubspec.yaml +++ b/pkg/analysis_server/pubspec.yaml @@ -8,8 +8,10 @@ environment: dependencies: _fe_analyzer_shared: path: ../_fe_analyzer_shared - analyzer: any - analyzer_plugin: any + analyzer: + path: ../analyzer + analyzer_plugin: + path: ../analyzer_plugin args: any cli_util: any collection: any @@ -19,7 +21,8 @@ dependencies: html: any intl: any linter: any - meta: any + meta: + path: ../meta stream_channel: any telemetry: path: ../telemetry @@ -29,7 +32,8 @@ dependencies: yaml: any dev_dependencies: - analyzer_utilities: any + analyzer_utilities: + path: ../analyzer_utilities http: any logging: any matcher: any diff --git a/pkg/analyzer_cli/pubspec.yaml b/pkg/analyzer_cli/pubspec.yaml index a1980aad548..da7989b614e 100644 --- a/pkg/analyzer_cli/pubspec.yaml +++ b/pkg/analyzer_cli/pubspec.yaml @@ -8,13 +8,15 @@ environment: sdk: "^2.7.0" dependencies: - analyzer: any + analyzer: + path: ../analyzer args: '>=0.13.0 <2.0.0' bazel_worker: ^0.1.0 collection: ^1.14.1 convert: any linter: ^0.1.16 - meta: any + meta: + path: ../meta path: any pub_semver: ^1.4.2 yaml: any @@ -24,7 +26,3 @@ dev_dependencies: protobuf: ^0.13.0 test_reflective_loader: ^0.1.8 test: ^1.0.0 - -dependency_overrides: - analyzer: - path: ../analyzer diff --git a/pkg/analyzer_utilities/pubspec.yaml b/pkg/analyzer_utilities/pubspec.yaml index ed67a420b60..714215717c4 100644 --- a/pkg/analyzer_utilities/pubspec.yaml +++ b/pkg/analyzer_utilities/pubspec.yaml @@ -6,7 +6,8 @@ environment: sdk: '>=2.1.0 <3.0.0' dependencies: - analyzer: any + analyzer: + path: ../analyzer html: any path: any test: any diff --git a/pkg/build_integration/pubspec.yaml b/pkg/build_integration/pubspec.yaml index b741a4b982a..a7b79b18de1 100644 --- a/pkg/build_integration/pubspec.yaml +++ b/pkg/build_integration/pubspec.yaml @@ -9,11 +9,8 @@ environment: sdk: '>=2.1.0 <3.0.0' dependencies: - front_end: any + front_end: + path: ../front_end dev_dependencies: test: any - -dependency_overrides: - front_end: - path: ../front_end diff --git a/pkg/compiler/pubspec.yaml b/pkg/compiler/pubspec.yaml index 6a7ec657c66..8a61bf926df 100644 --- a/pkg/compiler/pubspec.yaml +++ b/pkg/compiler/pubspec.yaml @@ -13,8 +13,10 @@ dependencies: collection: any crypto: any dart2js_info: any - front_end: any - kernel: any + front_end: + path: ../front_end + kernel: + path: ../kernel # Unpublished packages that can be used via path dependency _fe_analyzer_shared: @@ -30,7 +32,8 @@ dev_dependencies: # Published packages - repo version ensured via dependency_overrides args: any http: any - js: any + js: + path: ../js package_config: any path: any source_maps: any @@ -54,7 +57,7 @@ dependency_overrides: front_end: path: ../front_end kernel: - path: ../../pkg/kernel + path: ../kernel meta: path: ../meta diff --git a/pkg/dartdev/pubspec.yaml b/pkg/dartdev/pubspec.yaml index ecf8aed519a..d6347db5c53 100644 --- a/pkg/dartdev/pubspec.yaml +++ b/pkg/dartdev/pubspec.yaml @@ -16,7 +16,8 @@ dependencies: path: ../dart2native dart_style: any intl: any - meta: any + meta: + path: ../meta nnbd_migration: path: ../nnbd_migration path: ^1.0.0 diff --git a/pkg/dev_compiler/pubspec.yaml b/pkg/dev_compiler/pubspec.yaml index 73cdcc9e1d0..155491df850 100644 --- a/pkg/dev_compiler/pubspec.yaml +++ b/pkg/dev_compiler/pubspec.yaml @@ -19,7 +19,8 @@ dependencies: path: ../front_end kernel: path: ../kernel - meta: any + meta: + path: ../meta path: any source_maps: any source_span: any @@ -27,10 +28,12 @@ dependencies: path: ../vm dev_dependencies: - analyzer: any + analyzer: + path: ../analyzer expect: path: ../expect - js: any + js: + path: ../js modular_test: path: ../modular_test package_config: any diff --git a/pkg/expect/pubspec.yaml b/pkg/expect/pubspec.yaml index b1b5bd14835..6a8f0bcc7f2 100644 --- a/pkg/expect/pubspec.yaml +++ b/pkg/expect/pubspec.yaml @@ -12,4 +12,5 @@ environment: sdk: '>=2.0.0' dependencies: - meta: any + meta: + path: ../meta diff --git a/pkg/front_end/pubspec.yaml b/pkg/front_end/pubspec.yaml index 77ae47b40a1..20a36d80d37 100644 --- a/pkg/front_end/pubspec.yaml +++ b/pkg/front_end/pubspec.yaml @@ -9,12 +9,16 @@ environment: sdk: '>=2.6.0 <3.0.0' dependencies: - _fe_analyzer_shared: any - kernel: any - package_config: any + _fe_analyzer_shared: + path: ../_fe_analyzer_shared + kernel: + path: ../kernel + package_config: + path: ../../third_party/pkg_tested/package_config dev_dependencies: - analyzer: any + analyzer: + path: ../analyzer args: '>=0.13.0 <2.0.0' async_helper: path: ../async_helper @@ -39,13 +43,3 @@ dev_dependencies: path: ../vm_service web_socket_channel: ^1.0.4 yaml: any - -dependency_overrides: - _fe_analyzer_shared: - path: ../_fe_analyzer_shared/ - analyzer: - path: ../analyzer - kernel: - path: ../kernel/ - package_config: - path: ../../third_party/pkg_tested/package_config/ diff --git a/pkg/kernel/pubspec.yaml b/pkg/kernel/pubspec.yaml index dc4c5f732c4..36092330e6a 100644 --- a/pkg/kernel/pubspec.yaml +++ b/pkg/kernel/pubspec.yaml @@ -8,7 +8,8 @@ publish_to: none environment: sdk: '>=2.6.0 <3.0.0' dependencies: - meta: ^1.0.0 + meta: + path: ../meta dev_dependencies: args: '>=0.13.4 <2.0.0' expect: diff --git a/pkg/nnbd_migration/pubspec.yaml b/pkg/nnbd_migration/pubspec.yaml index ed3de13ee3e..0a382de71ee 100644 --- a/pkg/nnbd_migration/pubspec.yaml +++ b/pkg/nnbd_migration/pubspec.yaml @@ -7,14 +7,18 @@ environment: sdk: '>=2.6.0 <3.0.0' dependencies: - _fe_analyzer_shared: ^4.0.0 - analyzer: ^0.40.4 - analyzer_plugin: ^0.2.4 + _fe_analyzer_shared: + path: ../_fe_analyzer_shared + analyzer: + path: ../analyzer + analyzer_plugin: + path: ../analyzer_plugin args: ^1.4.4 cli_util: ^0.2.0 collection: ^1.14.11 crypto: ^2.0.6 - meta: ^1.1.6 + meta: + path: ../meta path: ^1.6.2 pub_semver: ^1.4.2 source_span: ^1.4.1 diff --git a/pkg/scrape/pubspec.yaml b/pkg/scrape/pubspec.yaml index a0116443030..ffdaff1d30b 100644 --- a/pkg/scrape/pubspec.yaml +++ b/pkg/scrape/pubspec.yaml @@ -2,11 +2,15 @@ name: scrape description: Helper package for analyzing the syntax of Dart programs. # This package is not intended for consumption on pub.dev. DO NOT publish. publish_to: none + environment: sdk: ^2.10.0 + dependencies: args: ^1.6.0 - analyzer: ^0.40.4 + analyzer: + path: ../analyzer path: ^1.7.0 + dev_dependencies: pedantic: ^1.9.2 diff --git a/pkg/telemetry/pubspec.yaml b/pkg/telemetry/pubspec.yaml index c3ba85f9df1..ca646aa6d53 100644 --- a/pkg/telemetry/pubspec.yaml +++ b/pkg/telemetry/pubspec.yaml @@ -9,7 +9,8 @@ environment: dependencies: http: ^0.12.0 - meta: ^1.0.2 + meta: + path: ../meta path: ^1.4.0 stack_trace: ^1.7.0 usage: ^3.2.0+1 diff --git a/pkg/vm/pubspec.yaml b/pkg/vm/pubspec.yaml index 9a2d487119d..95341eb527e 100644 --- a/pkg/vm/pubspec.yaml +++ b/pkg/vm/pubspec.yaml @@ -11,9 +11,12 @@ dependencies: build_integration: path: ../build_integration crypto: any - front_end: any - kernel: ^0.3.6 - meta: any + front_end: + path: ../front_end + kernel: + path: ../kernel + meta: + path: ../meta package_config: any dev_dependencies: diff --git a/tools/package_deps/bin/package_deps.dart b/tools/package_deps/bin/package_deps.dart index e17748598e4..ad73069c23e 100644 --- a/tools/package_deps/bin/package_deps.dart +++ b/tools/package_deps/bin/package_deps.dart @@ -4,10 +4,6 @@ import 'package:cli_util/cli_logging.dart'; import 'package:path/path.dart' as path; import 'package:yaml/yaml.dart' as yaml; -// TODO(devoncarew): Validate that publishable packages don't use relative sdk -// paths in their pubspecs. - -// TODO(devoncarew): Find unused entries in the DEPS file. const validateDEPS = false; void main(List arguments) { @@ -20,6 +16,15 @@ void main(List arguments) { exit(1); } + print('To run this script, execute:'); + print(''); + print(' dart tools/package_deps/bin/package_deps.dart'); + print(''); + print('See pkg/README.md for more information.'); + print(''); + print('----'); + print(''); + // locate all pkg/ packages final packages = []; for (var entity in Directory('pkg').listSync()) { @@ -31,6 +36,8 @@ void main(List arguments) { } } + List pkgPackages = packages.map((p) => p.packageName).toList(); + // Manually added directories (outside of pkg/). List alsoValidate = [ 'tools/package_deps', @@ -49,7 +56,7 @@ void main(List arguments) { print('validating ${package.dir}' '${package.publishable ? ' [publishable]' : ''}'); - if (!package.validate(logger)) { + if (!package.validate(logger, pkgPackages)) { validateFailure = true; } @@ -59,19 +66,22 @@ void main(List arguments) { // Read and display info about the sdk DEPS file. if (validateDEPS) { print('SDK DEPS'); + print(''); + var sdkDeps = SdkDeps(File('DEPS')); sdkDeps.parse(); - print(''); - print('packages:'); - for (var pkg in sdkDeps.pkgs) { - print(' package:$pkg'); + + List deps = [...sdkDeps.pkgs, ...sdkDeps.testedPkgs]..sort(); + for (var pkg in deps) { + final tested = sdkDeps.testedPkgs.contains(pkg); + print('package:$pkg${tested ? ' [tested]' : ''}'); } - print(''); - print('tested packages:'); - for (var pkg in sdkDeps.testedPkgs) { - print(' package:$pkg'); - } + // TODO(devoncarew): Validate that published packages solve against the + // versions brought in from the DEPS file. + + // TODO(devoncarew): Find unused entries in the DEPS file. + } if (validateFailure) { @@ -93,13 +103,16 @@ class Package implements Comparable { String get packageName => _packageName; Set _declaredDependencies; + List _declaredPubDeps; Set _declaredDevDependencies; + List _declaredDevPubDeps; List get regularDependencies => _regularDependencies.toList()..sort(); List get devDependencies => _devDependencies.toList()..sort(); bool _publishToNone; + bool get publishable => !_publishToNone; @override @@ -111,9 +124,9 @@ class Package implements Comparable { @override int compareTo(Package other) => dir.compareTo(other.dir); - bool validate(Logger logger) { + bool validate(Logger logger, List pkgPackages) { _parseImports(); - return _validatePubspecDeps(logger); + return _validatePubspecDeps(logger, pkgPackages); } void _parseImports() { @@ -155,21 +168,34 @@ class Package implements Comparable { _packageName = docContents['name']; _publishToNone = docContents['publish_to'] == 'none'; + _declaredPubDeps = []; if (docContents['dependencies'] != null) { _declaredDependencies = Set.from(docContents['dependencies'].keys); + + var deps = docContents['dependencies']; + for (var package in deps.keys) { + _declaredPubDeps.add(PubDep.parse(package, deps[package])); + } } else { _declaredDependencies = {}; } + + _declaredDevPubDeps = []; if (docContents['dev_dependencies'] != null) { _declaredDevDependencies = Set.from(docContents['dev_dependencies'].keys); + + var deps = docContents['dev_dependencies']; + for (var package in deps.keys) { + _declaredDevPubDeps.add(PubDep.parse(package, deps[package])); + } } else { _declaredDevDependencies = {}; } } - bool _validatePubspecDeps(Logger logger) { + bool _validatePubspecDeps(Logger logger, List pkgPackages) { var fail = false; if (dirName != packageName) { @@ -240,6 +266,47 @@ class Package implements Comparable { fail = true; } + // Validate that we don't have relative deps into third_party. + // TODO(devoncarew): This is currently just enforced for publishable + // packages. + if (publishable) { + for (PubDep dep in [..._declaredPubDeps, ..._declaredDevPubDeps]) { + if (dep is PathPubDep) { + var path = dep.path; + + if (path.contains('third_party/pkg_tested/') || + path.contains('third_party/pkg/')) { + out(' Prefer a semver dependency for packages brought in via DEPS:'); + out(' $dep'); + fail = true; + } + } + } + } + + // Validate that published packages don't use path deps. + if (publishable) { + for (PubDep dep in _declaredPubDeps) { + if (dep is PathPubDep) { + out(' Published packages should use semver deps:'); + out(' $dep'); + fail = true; + } + } + } + + // Validate that non-published packages use relative a (relative) path dep + // for pkg/ packages. + if (!publishable) { + for (PubDep dep in [..._declaredPubDeps, ..._declaredDevPubDeps]) { + if (pkgPackages.contains(dep.name) && dep is! PathPubDep) { + out(' Prefer a relative path dep for pkg/ packages:'); + out(' $dep'); + fail = true; + } + } + } + if (!fail) { print(' No issues.'); } @@ -361,3 +428,45 @@ class SdkDeps { testedPkgs.sort(); } } + +abstract class PubDep { + final String name; + + PubDep(this.name); + + String toString() => name; + + static PubDep parse(String name, Object dep) { + if (dep is String) { + return SemverPubDep(name, dep); + } else if (dep is Map) { + if (dep.containsKey('path')) { + return PathPubDep(name, dep['path']); + } else { + return UnhandledPubDep(name); + } + } else { + return UnhandledPubDep(name); + } + } +} + +class SemverPubDep extends PubDep { + final String value; + + SemverPubDep(String name, this.value) : super(name); + + String toString() => '$name: $value'; +} + +class PathPubDep extends PubDep { + final String path; + + PathPubDep(String name, this.path) : super(name); + + String toString() => '$name: $path'; +} + +class UnhandledPubDep extends PubDep { + UnhandledPubDep(String name) : super(name); +}