Fix dependency_overrides validator in workspaces (#4564)
diff --git a/lib/src/package_graph.dart b/lib/src/package_graph.dart index 9cd77a2..01ee7ca 100644 --- a/lib/src/package_graph.dart +++ b/lib/src/package_graph.dart
@@ -37,15 +37,14 @@ SolveResult result, ) { final packages = { - for (final id in result.packages) - id.name: - id.isRoot - ? entrypoint.workspaceRoot - : Package( - result.pubspecs[id.name]!, - entrypoint.cache.getDirectory(id), - [], - ), + for (final package in entrypoint.workspaceRoot.transitiveWorkspace) + package.name: package, + for (final id in result.packages.where((p) => !p.isRoot)) + id.name: Package( + result.pubspecs[id.name]!, + entrypoint.cache.getDirectory(id), + [], + ), }; return PackageGraph(entrypoint, packages);
diff --git a/lib/src/validator/dependency_override.dart b/lib/src/validator/dependency_override.dart index 8f88631..cc7e2ab 100644 --- a/lib/src/validator/dependency_override.dart +++ b/lib/src/validator/dependency_override.dart
@@ -4,26 +4,33 @@ import 'dart:async'; -import 'package:collection/collection.dart'; - import '../validator.dart'; -/// A validator that validates a package's dependencies overrides (or the -/// absence thereof). +/// Complains (with a hint) if any of the transitive dependencies of a package's +/// non-dev dependencies are overridden anywhere in the workspace. class DependencyOverrideValidator extends Validator { @override Future<void> validate() async { - final overridden = MapKeySet( - context.entrypoint.workspaceRoot.allOverridesInWorkspace, - ); - final dev = MapKeySet(package.devDependencies); - if (overridden.difference(dev).isNotEmpty) { - final overridesFile = - package.pubspec.dependencyOverridesFromOverridesFile - ? package.pubspecOverridesPath - : package.pubspecPath; + final graph = await context.entrypoint.packageGraph; + final transitiveNonDevDependencies = <String>{}; + final toVisit = [package.name]; + while (toVisit.isNotEmpty) { + final next = toVisit.removeLast(); + if (transitiveNonDevDependencies.add(next)) { + toVisit.addAll(graph.packages[next]!.dependencies.keys); + } + } - hints.add(''' + for (final workspacePackage + in context.entrypoint.workspaceRoot.transitiveWorkspace) { + for (final override + in workspacePackage.pubspec.dependencyOverrides.keys) { + if (transitiveNonDevDependencies.contains(override)) { + final overridesFile = + workspacePackage.pubspec.dependencyOverridesFromOverridesFile + ? workspacePackage.pubspecOverridesPath + : workspacePackage.pubspecPath; + hints.add(''' Non-dev dependencies are overridden in $overridesFile. This indicates you are not testing your package against the same versions of its @@ -32,6 +39,8 @@ This might be necessary for packages with cyclic dependencies. Please be extra careful when publishing.'''); + } + } } } }
diff --git a/pubspec.lock b/pubspec.lock index 6df6ff8..b7ad4bf 100644 --- a/pubspec.lock +++ b/pubspec.lock
@@ -5,23 +5,18 @@ dependency: transitive description: name: _fe_analyzer_shared - sha256: "88399e291da5f7e889359681a8f64b18c5123e03576b01f32a6a276611e511c3" + sha256: dc27559385e905ad30838356c5f5d574014ba39872d732111cd07ac0beff4c57 url: "https://pub.dev" source: hosted - version: "78.0.0" - _macros: - dependency: transitive - description: dart - source: sdk - version: "0.3.3" + version: "80.0.0" analyzer: dependency: "direct main" description: name: analyzer - sha256: "62899ef43d0b962b056ed2ebac6b47ec76ffd003d5f7c4e4dc870afe63188e33" + sha256: "192d1c5b944e7e53b24b5586db760db934b177d4147c42fbca8c8c5f1eb8d11e" url: "https://pub.dev" source: hosted - version: "7.1.0" + version: "7.3.0" args: dependency: "direct main" description: @@ -190,14 +185,6 @@ url: "https://pub.dev" source: hosted version: "1.3.0" - macros: - dependency: transitive - description: - name: macros - sha256: "1d9e801cd66f7ea3663c45fc708450db1fa57f988142c64289142c9b7ee80656" - url: "https://pub.dev" - source: hosted - version: "0.1.3-main.0" matcher: dependency: transitive description:
diff --git a/pubspec.yaml b/pubspec.yaml index 1f40063..1aebf3e 100644 --- a/pubspec.yaml +++ b/pubspec.yaml
@@ -4,7 +4,7 @@ sdk: ^3.7.0 dependencies: - analyzer: 7.1.0 + analyzer: 7.3.0 args: ^2.7.0 async: ^2.11.0 cli_util: ^0.4.1
diff --git a/test/validator/dependency_override_test.dart b/test/validator/dependency_override_test.dart index 8f877da..92b534d 100644 --- a/test/validator/dependency_override_test.dart +++ b/test/validator/dependency_override_test.dart
@@ -2,6 +2,7 @@ // for details. All rights reserved. Use of this source code is governed by a // BSD-style license that can be found in the LICENSE file. +import 'package:path/path.dart' as p; import 'package:test/test.dart'; import '../descriptor.dart' as d; @@ -9,7 +10,7 @@ import 'utils.dart'; void main() { - test('should consider a package valid if it has dev dependency ' + test('should consider a package valid if it overrides dev dependency ' 'overrides', () async { final server = await servePackages(); server.serve('foo', '3.0.0'); @@ -27,8 +28,28 @@ await expectValidation(); }); - group('should consider a package invalid if', () { - test('it has only non-dev dependency overrides', () async { + test('should consider a package valid ' + 'if it has any dependency overrides on non-dependency', () async { + final server = await servePackages(); + server.serve('foo', '3.0.0'); + server.serve('bar', '3.0.0'); + + await d.validPackage().create(); + await d.dir(appPath, [ + d.validPubspec( + extras: { + 'dev_dependencies': {'foo': '^1.0.0'}, + 'dependency_overrides': {'foo': '^3.0.0', 'bar': '^3.0.0'}, + }, + ), + ]).create(); + + await expectValidation(); + }); + + test( + 'should consider a package invalid if it has override of direct dependency', + () async { final server = await servePackages(); server.serve('foo', '3.0.0'); await d.validPackage().create(); @@ -45,46 +66,84 @@ await expectValidationHint( 'Non-dev dependencies are overridden in pubspec.yaml.', ); - }); - test('it has a pubspec_overrides.yaml', () async { - final server = await servePackages(); - server.serve('foo', '3.0.0'); - await d.validPackage().create(); + }, + ); - await d.dir(appPath, [ + test('should consider a package invalid if it ' + 'has override of transitive dependency', () async { + final server = await servePackages(); + server.serve('foo', '1.0.0', deps: {'bar': '^3.0.0'}); + server.serve('bar', '3.0.0'); + + await d.validPackage().create(); + + await d.dir(appPath, [ + d.validPubspec( + extras: { + 'dependencies': {'foo': '^1.0.0'}, + 'dependency_overrides': {'bar': '^3.0.0'}, + }, + ), + ]).create(); + + await expectValidationHint( + 'Non-dev dependencies are overridden in pubspec.yaml.', + ); + }); + + test('reports correctly about a pubspec_overrides.yaml', () async { + final server = await servePackages(); + server.serve('foo', '3.0.0'); + await d.validPackage().create(); + + await d.dir(appPath, [ + d.validPubspec( + extras: { + 'dependencies': {'foo': '^1.0.0'}, + }, + ), + d.pubspecOverrides({ + 'dependency_overrides': {'foo': '3.0.0'}, + }), + ]).create(); + + await expectValidationHint( + 'Non-dev dependencies are overridden in pubspec_overrides.yaml.', + ); + }); + + test('Detects overrides from outside work-package', () async { + final server = await servePackages(); + server.serve('foo', '3.0.0'); + await d.validPackage().create(); + + await d.dir(appPath, [ + d.libPubspec( + 'workspace', + '1.2.3', + extras: { + 'workspace': ['a'], + 'dependency_overrides': {'foo': '^3.0.0'}, + }, + sdk: '^3.5.0', + ), + d.dir('a', [ + ...d.validPackage().contents, d.validPubspec( extras: { + 'environment': {'sdk': '^3.5.0'}, + 'resolution': 'workspace', 'dependencies': {'foo': '^1.0.0'}, }, ), - d.pubspecOverrides({ - 'dependency_overrides': {'foo': '3.0.0'}, - }), - ]).create(); + ]), + ]).create(); - await expectValidationHint( - 'Non-dev dependencies are overridden in pubspec_overrides.yaml.', - ); - }); - - test('it has any non-dev dependency overrides', () async { - final server = await servePackages(); - server.serve('foo', '3.0.0'); - server.serve('bar', '3.0.0'); - - await d.validPackage().create(); - await d.dir(appPath, [ - d.validPubspec( - extras: { - 'dev_dependencies': {'foo': '^1.0.0'}, - 'dependency_overrides': {'foo': '^3.0.0', 'bar': '^3.0.0'}, - }, - ), - ]).create(); - - await expectValidationHint( - 'Non-dev dependencies are overridden in pubspec.yaml.', - ); - }); + final s = p.separator; + await expectValidationHint( + 'Non-dev dependencies are overridden in ..${s}pubspec.yaml.', + workingDirectory: p.join(d.sandbox, appPath, 'a'), + environment: {'_PUB_TEST_SDK_VERSION': '3.5.0'}, + ); }); }
diff --git a/test/validator/utils.dart b/test/validator/utils.dart index 3a8fb88..6e79f11 100644 --- a/test/validator/utils.dart +++ b/test/validator/utils.dart
@@ -58,11 +58,13 @@ String hint, { int count = 1, Map<String, String> environment = const {}, + String? workingDirectory, }) async { final s = count == 1 ? '' : 's'; await expectValidation( message: allOf([contains(hint), contains('and $count hint$s')]), environment: environment, + workingDirectory: workingDirectory, ); }