Skip to content

Give error if global activated package depends on hooks #4612

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion lib/src/command/deps.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down
8 changes: 6 additions & 2 deletions lib/src/command/upgrade.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
38 changes: 38 additions & 0 deletions lib/src/global_packages.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -194,6 +211,10 @@ class GlobalPackages {
await entrypoint.acquireDependencies(SolveType.get);
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.
Expand Down Expand Up @@ -260,10 +281,27 @@ 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.
for (final package in result.packages) {
_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 =
originalLockFile != null && originalLockFile.samePackageIds(lockFile);

Expand Down
44 changes: 18 additions & 26 deletions lib/src/package_graph.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -23,9 +20,6 @@ class PackageGraph {
/// relevant in the current context.
final Map<String, Package> packages;

/// A map of transitive dependencies for each package.
Map<String, Set<Package>>? _transitiveDependencies;

PackageGraph(this.entrypoint, this.packages);

/// Creates a package graph using the data from [result].
Expand Down Expand Up @@ -55,28 +49,25 @@ 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<Package> transitiveDependencies(String package) {
if (package == entrypoint.workspaceRoot.name) {
return packages.values.toSet();
}
Set<Package> transitiveDependencies(
String package, {
required bool followDevDependenciesFromRoot,
}) {
final result = <Package>{};

if (_transitiveDependencies == null) {
final graph = mapMap<String, Package, String, Iterable<String>>(
packages,
value: (_, package) => package.dependencies.keys,
);
final closure = transitiveClosure(graph.keys, (n) => graph[n]!);
_transitiveDependencies =
mapMap<String, Set<String>, String, Set<Package>>(
closure,
value: (depender, names) {
final set = names.map((name) => packages[name]!).toSet();
set.add(packages[depender]!);
return set;
},
);
final stack = [package];
final visited = <String>{};
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 (followDevDependenciesFromRoot && current == package) {
stack.addAll(currentPackage.devDependencies.keys);
}
}
return _transitiveDependencies![package]!;
return result;
}

bool _isPackageFromImmutableSource(String package) {
Expand All @@ -98,6 +89,7 @@ class PackageGraph {

return transitiveDependencies(
package,
followDevDependenciesFromRoot: true,
).any((dep) => !_isPackageFromImmutableSource(dep.name));
}
}
2 changes: 1 addition & 1 deletion pubspec.lock
Original file line number Diff line number Diff line change
Expand Up @@ -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"
138 changes: 138 additions & 0 deletions test/global/activate/activate_package_with_hooks_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,138 @@
// 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',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use a different name for this, as this https://pub.dev/packages/hooks/versions/0.19.5 should never contain a hook.

'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_dev_hooks',
'pkgs/foo_no_hooks',
],
},
sdk: '^3.5.0',
),
dir('pkgs', [
dir('foo_hooks', [
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,
),
]),
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: '''
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'},
);
},
);

test('activating a hosted package gives error if package uses hooks in any'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test name needs to be updated. A hook in a dev dep is fine.

' 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: '''
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: '''
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', 'foo_hooks_in_dev_deps'],
environment: {'_PUB_TEST_SDK_VERSION': '3.5.0'},
);
});
}