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 <scheglov@google.com>
Commit-Queue: Devon Carew <devoncarew@google.com>

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

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

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

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

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

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
 

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

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

pkg/expect/pubspec.yaml

@@ -12,4 +12,5 @@ environment:
   sdk: '>=2.0.0'
 
 dependencies:
-  meta: any
+  meta:
+    path: ../meta

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/

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:

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

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

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

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:

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<String> arguments) {
@@ -20,6 +16,15 @@ void main(List<String> 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 = <Package>[];
   for (var entity in Directory('pkg').listSync()) {
@@ -31,6 +36,8 @@ void main(List<String> arguments) {
     }
   }
 
+  List<String> pkgPackages = packages.map((p) => p.packageName).toList();
+
   // Manually added directories (outside of pkg/).
   List<String> alsoValidate = [
     'tools/package_deps',
@@ -49,7 +56,7 @@ void main(List<String> 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<String> 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<String> 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<Package> {
 
   String get packageName => _packageName;
   Set<String> _declaredDependencies;
+  List<PubDep> _declaredPubDeps;
   Set<String> _declaredDevDependencies;
+  List<PubDep> _declaredDevPubDeps;
 
   List<String> get regularDependencies => _regularDependencies.toList()..sort();
 
   List<String> get devDependencies => _devDependencies.toList()..sort();
 
   bool _publishToNone;
+
   bool get publishable => !_publishToNone;
 
   @override
@@ -111,9 +124,9 @@ class Package implements Comparable<Package> {
   @override
   int compareTo(Package other) => dir.compareTo(other.dir);
 
-  bool validate(Logger logger) {
+  bool validate(Logger logger, List<String> pkgPackages) {
     _parseImports();
-    return _validatePubspecDeps(logger);
+    return _validatePubspecDeps(logger, pkgPackages);
   }
 
   void _parseImports() {
@@ -155,21 +168,34 @@ class Package implements Comparable<Package> {
     _packageName = docContents['name'];
     _publishToNone = docContents['publish_to'] == 'none';
 
+    _declaredPubDeps = [];
     if (docContents['dependencies'] != null) {
       _declaredDependencies =
           Set<String>.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<String>.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<String> pkgPackages) {
     var fail = false;
 
     if (dirName != packageName) {
@@ -240,6 +266,47 @@ class Package implements Comparable<Package> {
       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);
+}