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>
diff --git a/pkg/README.md b/pkg/README.md
new file mode 100644
index 0000000..474a204
--- /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 9a94f7d..ef86eeb 100644
--- a/pkg/analysis_server/pubspec.yaml
+++ b/pkg/analysis_server/pubspec.yaml
@@ -8,8 +8,10 @@
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 @@
html: any
intl: any
linter: any
- meta: any
+ meta:
+ path: ../meta
stream_channel: any
telemetry:
path: ../telemetry
@@ -29,7 +32,8 @@
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 a1980aa..da7989b 100644
--- a/pkg/analyzer_cli/pubspec.yaml
+++ b/pkg/analyzer_cli/pubspec.yaml
@@ -8,13 +8,15 @@
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 @@
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 ed67a42..7142157 100644
--- a/pkg/analyzer_utilities/pubspec.yaml
+++ b/pkg/analyzer_utilities/pubspec.yaml
@@ -6,7 +6,8 @@
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 b741a4b..a7b79b1 100644
--- a/pkg/build_integration/pubspec.yaml
+++ b/pkg/build_integration/pubspec.yaml
@@ -9,11 +9,8 @@
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 6a7ec65..8a61bf9 100644
--- a/pkg/compiler/pubspec.yaml
+++ b/pkg/compiler/pubspec.yaml
@@ -13,8 +13,10 @@
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 @@
# 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 @@
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 ecf8aed..d6347db 100644
--- a/pkg/dartdev/pubspec.yaml
+++ b/pkg/dartdev/pubspec.yaml
@@ -16,7 +16,8 @@
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 73cdcc9..155491d 100644
--- a/pkg/dev_compiler/pubspec.yaml
+++ b/pkg/dev_compiler/pubspec.yaml
@@ -19,7 +19,8 @@
path: ../front_end
kernel:
path: ../kernel
- meta: any
+ meta:
+ path: ../meta
path: any
source_maps: any
source_span: any
@@ -27,10 +28,12 @@
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 b1b5bd1..6a8f0bc 100644
--- a/pkg/expect/pubspec.yaml
+++ b/pkg/expect/pubspec.yaml
@@ -12,4 +12,5 @@
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 77ae47b..20a36d8 100644
--- a/pkg/front_end/pubspec.yaml
+++ b/pkg/front_end/pubspec.yaml
@@ -9,12 +9,16 @@
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 @@
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 dc4c5f73..3609233 100644
--- a/pkg/kernel/pubspec.yaml
+++ b/pkg/kernel/pubspec.yaml
@@ -8,7 +8,8 @@
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 ed3de13..0a382de 100644
--- a/pkg/nnbd_migration/pubspec.yaml
+++ b/pkg/nnbd_migration/pubspec.yaml
@@ -7,14 +7,18 @@
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 a011644..ffdaff1 100644
--- a/pkg/scrape/pubspec.yaml
+++ b/pkg/scrape/pubspec.yaml
@@ -2,11 +2,15 @@
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 c3ba85f..ca646aa 100644
--- a/pkg/telemetry/pubspec.yaml
+++ b/pkg/telemetry/pubspec.yaml
@@ -9,7 +9,8 @@
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 9a2d487..95341eb 100644
--- a/pkg/vm/pubspec.yaml
+++ b/pkg/vm/pubspec.yaml
@@ -11,9 +11,12 @@
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 e177485..ad73069 100644
--- a/tools/package_deps/bin/package_deps.dart
+++ b/tools/package_deps/bin/package_deps.dart
@@ -4,10 +4,6 @@
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 @@
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 @@
}
}
+ 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 @@
print('validating ${package.dir}'
'${package.publishable ? ' [publishable]' : ''}');
- if (!package.validate(logger)) {
+ if (!package.validate(logger, pkgPackages)) {
validateFailure = true;
}
@@ -59,19 +66,22 @@
// 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 @@
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 @@
@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 @@
_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 @@
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 @@
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);
+}
