From 30eb18c54693dfab51b92609dac419472946a32b Mon Sep 17 00:00:00 2001 From: Sigurd Meldgaard Date: Mon, 23 Jun 2025 12:26:19 +0000 Subject: [PATCH 1/3] Give error if globall activated package depends on hooks --- lib/src/global_packages.dart | 20 ++++ lib/src/package_graph.dart | 38 +++---- pubspec.lock | 2 +- .../activate_package_with_hooks_test.dart | 106 ++++++++++++++++++ 4 files changed, 140 insertions(+), 26 deletions(-) create mode 100644 test/global/activate/activate_package_with_hooks_test.dart diff --git a/lib/src/global_packages.dart b/lib/src/global_packages.dart index 25169eb03..63af9bd9d 100644 --- a/lib/src/global_packages.dart +++ b/lib/src/global_packages.dart @@ -192,6 +192,12 @@ class GlobalPackages { // Get the package's dependencies. await entrypoint.acquireDependencies(SolveType.get); + for (final package in (await entrypoint.packageGraph) + .transitiveDependencies(entrypoint.workPackage.name)) { + if (fileExists(p.join(package.dir, 'hooks', 'build.dart'))) { + fail('Cannot `global activate` packages with hooks.'); + } + } final activatedPackage = entrypoint.workPackage; final name = activatedPackage.name; _describeActive(name, cache); @@ -260,10 +266,24 @@ class GlobalPackages { } rethrow; } + // We want the entrypoint to be rooted at 'dep' not the dummy-package. result.packages.removeWhere((id) => id.name == 'pub global activate'); final lockFile = await result.downloadCachedPackages(cache); + + // Because we know that the dummy package never is a workspace we can + // iterate all packages. + // TODO(sigurdm): refactor PackageGraph to make it possible to query without + // loading entrypoint. + for (final package in result.packages) { + if (fileExists( + p.join(cache.getDirectory(package), 'hooks', 'build.dart'), + )) { + fail('Cannot `global activate` packages with hooks.'); + } + } + final sameVersions = originalLockFile != null && originalLockFile.samePackageIds(lockFile); diff --git a/lib/src/package_graph.dart b/lib/src/package_graph.dart index 01ee7caaa..1c9cb380e 100644 --- a/lib/src/package_graph.dart +++ b/lib/src/package_graph.dart @@ -2,14 +2,11 @@ // 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:graphs/graphs.dart'; - import 'entrypoint.dart'; import 'package.dart'; import 'solver.dart'; import 'source/cached.dart'; import 'source/sdk.dart'; -import 'utils.dart'; /// A holistic view of the entire transitive dependency graph for an entrypoint. class PackageGraph { @@ -23,9 +20,6 @@ class PackageGraph { /// relevant in the current context. final Map packages; - /// A map of transitive dependencies for each package. - Map>? _transitiveDependencies; - PackageGraph(this.entrypoint, this.packages); /// Creates a package graph using the data from [result]. @@ -56,27 +50,21 @@ class PackageGraph { /// dev and override. For any other package, it ignores dev and override /// dependencies. Set transitiveDependencies(String package) { - if (package == entrypoint.workspaceRoot.name) { - return packages.values.toSet(); - } + final result = {}; - if (_transitiveDependencies == null) { - final graph = mapMap>( - packages, - value: (_, package) => package.dependencies.keys, - ); - final closure = transitiveClosure(graph.keys, (n) => graph[n]!); - _transitiveDependencies = - mapMap, String, Set>( - closure, - value: (depender, names) { - final set = names.map((name) => packages[name]!).toSet(); - set.add(packages[depender]!); - return set; - }, - ); + final stack = [package]; + final visited = {}; + while (stack.isNotEmpty) { + final current = stack.removeLast(); + if (!visited.add(current)) continue; + final currentPackage = packages[current]!; + result.add(currentPackage); + stack.addAll(currentPackage.dependencies.keys); + if (current == package) { + stack.addAll(currentPackage.devDependencies.keys); + } } - return _transitiveDependencies![package]!; + return result; } bool _isPackageFromImmutableSource(String package) { diff --git a/pubspec.lock b/pubspec.lock index f24c3e1b5..049bbdf34 100644 --- a/pubspec.lock +++ b/pubspec.lock @@ -474,4 +474,4 @@ packages: source: hosted version: "2.2.2" sdks: - dart: ">=3.7.0 <4.0.0" + dart: ">=3.8.0 <4.0.0" diff --git a/test/global/activate/activate_package_with_hooks_test.dart b/test/global/activate/activate_package_with_hooks_test.dart new file mode 100644 index 000000000..486eea6d0 --- /dev/null +++ b/test/global/activate/activate_package_with_hooks_test.dart @@ -0,0 +1,106 @@ +// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file +// 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'; +import '../../test_pub.dart'; + +void main() { + test( + 'activating a package from path gives error if package uses hooks', + () async { + final server = await servePackages(); + server.serve( + 'hooks', + '1.0.0', + contents: [ + dir('hooks', [file('build.dart')]), + ], + ); + server.serve('no_hooks', '1.0.0'); + + await dir(appPath, [ + libPubspec( + 'foo', + '1.2.3', + extras: { + 'workspace': ['pkgs/foo_hooks', 'pkgs/foo_no_hooks'], + }, + sdk: '^3.5.0', + ), + dir('pkgs', [ + dir('foo_hooks', [ + libPubspec( + 'foo_hooks', + '1.1.1', + devDeps: {'hooks': '^1.0.0'}, + resolutionWorkspace: true, + ), + ]), + dir('foo_no_hooks', [ + libPubspec( + 'foo_no_hooks', + '1.1.1', + deps: {'no_hooks': '^1.0.0'}, + resolutionWorkspace: true, + ), + ]), + ]), + ]).create(); + + await runPub( + args: ['global', 'activate', '-spath', p.join('pkgs', 'foo_hooks')], + environment: {'_PUB_TEST_SDK_VERSION': '3.5.0'}, + error: 'Cannot `global activate` packages with hooks.', + exitCode: 1, + ); + + await runPub( + args: ['global', 'activate', '-spath', p.join('pkgs', 'foo_no_hooks')], + environment: {'_PUB_TEST_SDK_VERSION': '3.5.0'}, + ); + }, + ); + + test('activating a hosted package gives error if package uses hooks in any' + ' dependency', () async { + final server = await servePackages(); + server.serve( + 'hooks', + '1.0.0', + contents: [ + dir('hooks', [file('build.dart')]), + ], + ); + server.serve('foo_hooks', '1.1.1', deps: {'hooks': '^1.0.0'}); + server.serve( + 'foo_hooks_in_dev_deps', + '1.0.0', + pubspec: { + 'dev_dependencies': {'hooks': '^1.0.0'}, + }, + ); + + await runPub( + args: ['global', 'activate', 'hooks'], + environment: {'_PUB_TEST_SDK_VERSION': '3.5.0'}, + error: 'Cannot `global activate` packages with hooks.', + exitCode: 1, + ); + + await runPub( + args: ['global', 'activate', 'foo_hooks'], + environment: {'_PUB_TEST_SDK_VERSION': '3.5.0'}, + error: 'Cannot `global activate` packages with hooks.', + exitCode: 1, + ); + + await runPub( + args: ['global', 'activate', 'foo_hooks_in_dev_deps'], + environment: {'_PUB_TEST_SDK_VERSION': '3.5.0'}, + ); + }); +} From 476266704c1dfd7cf237e5569fd2980d38f9195c Mon Sep 17 00:00:00 2001 From: Sigurd Meldgaard Date: Mon, 23 Jun 2025 13:44:44 +0000 Subject: [PATCH 2/3] Improve --- lib/src/command/deps.dart | 7 ++- lib/src/command/upgrade.dart | 8 +++- lib/src/global_packages.dart | 44 +++++++++++++------ lib/src/package_graph.dart | 8 +++- .../activate_package_with_hooks_test.dart | 40 +++++++++++++++-- 5 files changed, 85 insertions(+), 22 deletions(-) diff --git a/lib/src/command/deps.dart b/lib/src/command/deps.dart index 32c0a0b6c..222a59414 100644 --- a/lib/src/command/deps.dart +++ b/lib/src/command/deps.dart @@ -417,7 +417,12 @@ class DepsCommand extends PubCommand { ], ]; return nonDevDependencies - .expand(graph.transitiveDependencies) + .expand( + (p) => graph.transitiveDependencies( + p, + followDevDependenciesFromRoot: false, + ), + ) .map((package) => package.name) .toSet(); } diff --git a/lib/src/command/upgrade.dart b/lib/src/command/upgrade.dart index 9d4ba459a..0e93a646a 100644 --- a/lib/src/command/upgrade.dart +++ b/lib/src/command/upgrade.dart @@ -121,8 +121,12 @@ class UpgradeCommand extends PubCommand { final graph = await entrypoint.packageGraph; return argResults.rest .expand( - (package) => - graph.transitiveDependencies(package).map((p) => p.name), + (package) => graph + .transitiveDependencies( + package, + followDevDependenciesFromRoot: true, + ) + .map((p) => p.name), ) .toSet() .toList(); diff --git a/lib/src/global_packages.dart b/lib/src/global_packages.dart index 63af9bd9d..b4b708ee5 100644 --- a/lib/src/global_packages.dart +++ b/lib/src/global_packages.dart @@ -174,6 +174,23 @@ class GlobalPackages { ); } + void _testForHooks(Package package, String activatedPackageName) { + final prelude = + (package.name == activatedPackageName) + ? 'Package $activatedPackageName uses hooks.' + : 'The dependency of $activatedPackageName, ' + '${package.name} uses hooks.'; + if (fileExists(p.join(package.dir, 'hooks', 'build.dart'))) { + fail(''' +$prelude + +You currently cannot `global activate` packages relying on hooks. + +Follow progress in https://github.com/dart-lang/sdk/issues/60889. +'''); + } + } + /// Makes the local package at [path] globally active. /// /// [executables] is the names of the executables that should have binstubs. @@ -192,14 +209,12 @@ class GlobalPackages { // Get the package's dependencies. await entrypoint.acquireDependencies(SolveType.get); - for (final package in (await entrypoint.packageGraph) - .transitiveDependencies(entrypoint.workPackage.name)) { - if (fileExists(p.join(package.dir, 'hooks', 'build.dart'))) { - fail('Cannot `global activate` packages with hooks.'); - } - } final activatedPackage = entrypoint.workPackage; final name = activatedPackage.name; + for (final package in (await entrypoint.packageGraph) + .transitiveDependencies(name, followDevDependenciesFromRoot: false)) { + _testForHooks(package, name); + } _describeActive(name, cache); // Write a lockfile that points to the local package. @@ -274,14 +289,17 @@ class GlobalPackages { // Because we know that the dummy package never is a workspace we can // iterate all packages. - // TODO(sigurdm): refactor PackageGraph to make it possible to query without - // loading entrypoint. for (final package in result.packages) { - if (fileExists( - p.join(cache.getDirectory(package), 'hooks', 'build.dart'), - )) { - fail('Cannot `global activate` packages with hooks.'); - } + _testForHooks( + // TODO(sigurdm): refactor PackageGraph to make it possible to query + // without loading the entrypoint. + Package( + result.pubspecs[package.name]!, + cache.getDirectory(package), + [], + ), + name, + ); } final sameVersions = diff --git a/lib/src/package_graph.dart b/lib/src/package_graph.dart index 1c9cb380e..07f066629 100644 --- a/lib/src/package_graph.dart +++ b/lib/src/package_graph.dart @@ -49,7 +49,10 @@ class PackageGraph { /// For the entrypoint this returns all packages in [packages], which includes /// dev and override. For any other package, it ignores dev and override /// dependencies. - Set transitiveDependencies(String package) { + Set transitiveDependencies( + String package, { + required bool followDevDependenciesFromRoot, + }) { final result = {}; final stack = [package]; @@ -60,7 +63,7 @@ class PackageGraph { final currentPackage = packages[current]!; result.add(currentPackage); stack.addAll(currentPackage.dependencies.keys); - if (current == package) { + if (followDevDependenciesFromRoot && current == package) { stack.addAll(currentPackage.devDependencies.keys); } } @@ -86,6 +89,7 @@ class PackageGraph { return transitiveDependencies( package, + followDevDependenciesFromRoot: true, ).any((dep) => !_isPackageFromImmutableSource(dep.name)); } } diff --git a/test/global/activate/activate_package_with_hooks_test.dart b/test/global/activate/activate_package_with_hooks_test.dart index 486eea6d0..b03dd5e38 100644 --- a/test/global/activate/activate_package_with_hooks_test.dart +++ b/test/global/activate/activate_package_with_hooks_test.dart @@ -27,7 +27,11 @@ void main() { 'foo', '1.2.3', extras: { - 'workspace': ['pkgs/foo_hooks', 'pkgs/foo_no_hooks'], + 'workspace': [ + 'pkgs/foo_hooks', + 'pkgs/foo_dev_hooks', + 'pkgs/foo_no_hooks', + ], }, sdk: '^3.5.0', ), @@ -36,6 +40,14 @@ void main() { libPubspec( 'foo_hooks', '1.1.1', + deps: {'hooks': '^1.0.0'}, + resolutionWorkspace: true, + ), + ]), + dir('foo_dev_hooks', [ + libPubspec( + 'foo_dev_hooks', + '1.1.1', devDeps: {'hooks': '^1.0.0'}, resolutionWorkspace: true, ), @@ -54,10 +66,20 @@ void main() { await runPub( args: ['global', 'activate', '-spath', p.join('pkgs', 'foo_hooks')], environment: {'_PUB_TEST_SDK_VERSION': '3.5.0'}, - error: 'Cannot `global activate` packages with hooks.', + error: ''' +The dependency of foo_hooks, hooks uses hooks. + +You currently cannot `global activate` packages relying on hooks. + +Follow progress in https://github.com/dart-lang/sdk/issues/60889.''', exitCode: 1, ); + await runPub( + args: ['global', 'activate', '-spath', p.join('pkgs', 'foo_dev_hooks')], + environment: {'_PUB_TEST_SDK_VERSION': '3.5.0'}, + ); + await runPub( args: ['global', 'activate', '-spath', p.join('pkgs', 'foo_no_hooks')], environment: {'_PUB_TEST_SDK_VERSION': '3.5.0'}, @@ -87,14 +109,24 @@ void main() { await runPub( args: ['global', 'activate', 'hooks'], environment: {'_PUB_TEST_SDK_VERSION': '3.5.0'}, - error: 'Cannot `global activate` packages with hooks.', + error: ''' +Package hooks uses hooks. + +You currently cannot `global activate` packages relying on hooks. + +Follow progress in https://github.com/dart-lang/sdk/issues/60889.''', exitCode: 1, ); await runPub( args: ['global', 'activate', 'foo_hooks'], environment: {'_PUB_TEST_SDK_VERSION': '3.5.0'}, - error: 'Cannot `global activate` packages with hooks.', + error: ''' +The dependency of foo_hooks, hooks uses hooks. + +You currently cannot `global activate` packages relying on hooks. + +Follow progress in https://github.com/dart-lang/sdk/issues/60889.''', exitCode: 1, ); From a7594393a662aab7605c076222811a2c021c0c6f Mon Sep 17 00:00:00 2001 From: Sigurd Meldgaard Date: Mon, 7 Jul 2025 13:12:22 +0000 Subject: [PATCH 3/3] Correct tests --- pubspec.lock | 16 +++++++++--- pubspec.yaml | 1 + .../activate_package_with_hooks_test.dart | 26 +++++++++---------- 3 files changed, 26 insertions(+), 17 deletions(-) diff --git a/pubspec.lock b/pubspec.lock index 049bbdf34..4bffed1ee 100644 --- a/pubspec.lock +++ b/pubspec.lock @@ -5,18 +5,18 @@ packages: dependency: transitive description: name: _fe_analyzer_shared - sha256: e55636ed79578b9abca5fecf9437947798f5ef7456308b5cb85720b793eac92f + sha256: c81659312e021e3b780a502206130ea106487b34793bce61e26dc0f9b84807af url: "https://pub.dev" source: hosted - version: "82.0.0" + version: "83.0.0" analyzer: dependency: "direct main" description: name: analyzer - sha256: "904ae5bb474d32c38fb9482e2d925d5454cda04ddd0e55d2e6826bc72f6ba8c0" + sha256: "9c35a79bf2a150b3ea0d40010fbbb45b5ebea143d47096e0f82fd922a324b49b" url: "https://pub.dev" source: hosted - version: "7.4.5" + version: "7.4.6" args: dependency: "direct main" description: @@ -257,6 +257,14 @@ packages: url: "https://pub.dev" source: hosted version: "2.2.0" + retry: + dependency: "direct main" + description: + name: retry + sha256: "822e118d5b3aafed083109c72d5f484c6dc66707885e07c0fbcb8b986bba7efc" + url: "https://pub.dev" + source: hosted + version: "3.1.2" shelf: dependency: "direct main" description: diff --git a/pubspec.yaml b/pubspec.yaml index 808dda239..73c43109b 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -20,6 +20,7 @@ dependencies: path: ^1.9.1 pool: ^1.5.1 pub_semver: ^2.2.0 + retry: ^3.1.2 shelf: ^1.4.2 source_span: ^1.10.1 stack_trace: ^1.12.1 diff --git a/test/global/activate/activate_package_with_hooks_test.dart b/test/global/activate/activate_package_with_hooks_test.dart index b03dd5e38..847663ab9 100644 --- a/test/global/activate/activate_package_with_hooks_test.dart +++ b/test/global/activate/activate_package_with_hooks_test.dart @@ -14,13 +14,13 @@ void main() { () async { final server = await servePackages(); server.serve( - 'hooks', + 'uses_hooks', '1.0.0', contents: [ dir('hooks', [file('build.dart')]), ], ); - server.serve('no_hooks', '1.0.0'); + server.serve('uses_no_hooks', '1.0.0'); await dir(appPath, [ libPubspec( @@ -40,7 +40,7 @@ void main() { libPubspec( 'foo_hooks', '1.1.1', - deps: {'hooks': '^1.0.0'}, + deps: {'uses_hooks': '^1.0.0'}, resolutionWorkspace: true, ), ]), @@ -48,7 +48,7 @@ void main() { libPubspec( 'foo_dev_hooks', '1.1.1', - devDeps: {'hooks': '^1.0.0'}, + devDeps: {'uses_hooks': '^1.0.0'}, resolutionWorkspace: true, ), ]), @@ -56,7 +56,7 @@ void main() { libPubspec( 'foo_no_hooks', '1.1.1', - deps: {'no_hooks': '^1.0.0'}, + deps: {'uses_no_hooks': '^1.0.0'}, resolutionWorkspace: true, ), ]), @@ -67,7 +67,7 @@ void main() { args: ['global', 'activate', '-spath', p.join('pkgs', 'foo_hooks')], environment: {'_PUB_TEST_SDK_VERSION': '3.5.0'}, error: ''' -The dependency of foo_hooks, hooks uses hooks. +The dependency of foo_hooks, uses_hooks uses hooks. You currently cannot `global activate` packages relying on hooks. @@ -87,30 +87,30 @@ Follow progress in https://github.com/dart-lang/sdk/issues/60889.''', }, ); - test('activating a hosted package gives error if package uses hooks in any' + test('activating a hosted package gives error if package uses hooks in direct' ' dependency', () async { final server = await servePackages(); server.serve( - 'hooks', + 'uses_hooks', '1.0.0', contents: [ dir('hooks', [file('build.dart')]), ], ); - server.serve('foo_hooks', '1.1.1', deps: {'hooks': '^1.0.0'}); + server.serve('foo_hooks', '1.1.1', deps: {'uses_hooks': '^1.0.0'}); server.serve( 'foo_hooks_in_dev_deps', '1.0.0', pubspec: { - 'dev_dependencies': {'hooks': '^1.0.0'}, + 'dev_dependencies': {'uses_hooks': '^1.0.0'}, }, ); await runPub( - args: ['global', 'activate', 'hooks'], + args: ['global', 'activate', 'uses_hooks'], environment: {'_PUB_TEST_SDK_VERSION': '3.5.0'}, error: ''' -Package hooks uses hooks. +Package uses_hooks uses hooks. You currently cannot `global activate` packages relying on hooks. @@ -122,7 +122,7 @@ Follow progress in https://github.com/dart-lang/sdk/issues/60889.''', args: ['global', 'activate', 'foo_hooks'], environment: {'_PUB_TEST_SDK_VERSION': '3.5.0'}, error: ''' -The dependency of foo_hooks, hooks uses hooks. +The dependency of foo_hooks, uses_hooks uses hooks. You currently cannot `global activate` packages relying on hooks.