-
Notifications
You must be signed in to change notification settings - Fork 236
feat: add workspace support for packages check licenses #1540
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
base: main
Are you sure you want to change the base?
Changes from 9 commits
2a58446
ce7e027
6177eb4
d625cb6
b8d019f
f32c5e9
7bb1147
ad7f11c
2cad6f0
9b9cfa2
f2760e4
33a4973
df0bda5
8ca2652
5cb355b
1242593
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ import 'package:package_config/package_config.dart' as package_config; | |
| import 'package:pana/src/license_detection/license_detector.dart' as detector; | ||
| import 'package:path/path.dart' as path; | ||
| import 'package:very_good_cli/src/pub_license/spdx_license.gen.dart'; | ||
| import 'package:very_good_cli/src/pubspec/pubspec.dart'; | ||
| import 'package:very_good_cli/src/pubspec_lock/pubspec_lock.dart'; | ||
|
|
||
| /// Overrides the [package_config.findPackageConfig] function for testing. | ||
|
|
@@ -35,6 +36,10 @@ Future<detector.Result> Function(String, double)? detectLicenseOverride; | |
| @visibleForTesting | ||
| const pubspecLockBasename = 'pubspec.lock'; | ||
|
|
||
| /// The basename of the pubspec file. | ||
| @visibleForTesting | ||
| const pubspecBasename = 'pubspec.yaml'; | ||
|
|
||
| /// The URI for the pub.dev license page for the given [packageName]. | ||
| @visibleForTesting | ||
| Uri pubLicenseUri(String packageName) => | ||
|
|
@@ -192,11 +197,48 @@ class PackagesCheckLicensesCommand extends Command<int> { | |
| return ExitCode.noInput.code; | ||
| } | ||
|
|
||
| // Check if this is a workspace root and collect dependencies accordingly | ||
| final pubspecFile = File(path.join(targetPath, pubspecBasename)); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: you've already computed |
||
| final pubspec = _tryParsePubspec(pubspecFile); | ||
|
|
||
| // Collect workspace dependencies if this is a workspace root | ||
| final workspaceDependencies = _collectWorkspaceDependencies( | ||
| pubspec: pubspec, | ||
| targetDirectory: targetDirectory, | ||
| dependencyTypes: dependencyTypes, | ||
| ); | ||
|
|
||
| final filteredDependencies = pubspecLock.packages.where((dependency) { | ||
| if (!dependency.isPubHosted) return false; | ||
|
|
||
| if (skippedPackages.contains(dependency.name)) return false; | ||
|
|
||
| // If we have workspace dependencies, use them for filtering direct deps | ||
| if (workspaceDependencies != null) { | ||
| // For direct-main and direct-dev, check against workspace dependencies | ||
| if (dependencyTypes.contains('direct-main') || | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This outer |
||
| dependencyTypes.contains('direct-dev')) { | ||
| if (workspaceDependencies.contains(dependency.name)) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| // For transitive and direct-overridden, still use pubspec.lock types | ||
| final dependencyType = dependency.type; | ||
| if (dependencyTypes.contains('transitive') && | ||
| dependencyType == PubspecLockPackageDependencyType.transitive) { | ||
| return true; | ||
| } | ||
| if (dependencyTypes.contains('direct-overridden') && | ||
| dependencyType == | ||
| PubspecLockPackageDependencyType.directOverridden) { | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| // Non-workspace: use the original filtering logic | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Subtle behavioral divergence worth calling out in a comment or doc: in non-workspace mode, |
||
| final dependencyType = dependency.type; | ||
| return (dependencyTypes.contains('direct-main') && | ||
| dependencyType == PubspecLockPackageDependencyType.directMain) || | ||
|
|
@@ -531,6 +573,72 @@ extension on List<Object> { | |
| } | ||
| } | ||
|
|
||
| /// Attempts to parse a [Pubspec] from a file. | ||
| /// | ||
| /// Returns `null` if the file doesn't exist or cannot be parsed. | ||
| Pubspec? _tryParsePubspec(File pubspecFile) { | ||
| if (!pubspecFile.existsSync()) return null; | ||
| try { | ||
| return Pubspec.fromFile(pubspecFile); | ||
| } on PubspecParseException catch (_) { | ||
| return null; | ||
| } | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here, maybe it's better if it's part of pubspec library |
||
|
|
||
| /// Collects dependencies from a workspace. | ||
| /// | ||
| /// If [pubspec] is not a workspace root, returns `null`. | ||
| /// Otherwise, returns a set of dependency names collected from all workspace | ||
| /// members based on the requested [dependencyTypes]. | ||
| Set<String>? _collectWorkspaceDependencies({ | ||
| required Pubspec? pubspec, | ||
| required Directory targetDirectory, | ||
| required List<String> dependencyTypes, | ||
| }) { | ||
| if (pubspec == null || !pubspec.isWorkspaceRoot) return null; | ||
|
|
||
| final dependencies = <String>{}; | ||
|
|
||
| // Collect dependencies from the root pubspec itself | ||
| if (dependencyTypes.contains('direct-main')) { | ||
| dependencies.addAll(pubspec.dependencies); | ||
| } | ||
| if (dependencyTypes.contains('direct-dev')) { | ||
| dependencies.addAll(pubspec.devDependencies); | ||
| } | ||
|
|
||
| // Collect dependencies from workspace members | ||
| final members = pubspec.resolveWorkspaceMembers(targetDirectory); | ||
| for (final memberDirectory in members) { | ||
| final memberPubspecFile = File( | ||
| path.join(memberDirectory.path, pubspecBasename), | ||
| ); | ||
| final memberPubspec = _tryParsePubspec(memberPubspecFile); | ||
| if (memberPubspec == null) continue; | ||
|
|
||
| if (dependencyTypes.contains('direct-main')) { | ||
| dependencies.addAll(memberPubspec.dependencies); | ||
| } | ||
| if (dependencyTypes.contains('direct-dev')) { | ||
| dependencies.addAll(memberPubspec.devDependencies); | ||
| } | ||
|
|
||
| // Handle nested workspaces recursively | ||
| if (memberPubspec.isWorkspaceRoot) { | ||
| final nestedDeps = _collectWorkspaceDependencies( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no cycle protection here. If a member's |
||
| pubspec: memberPubspec, | ||
| targetDirectory: memberDirectory, | ||
| dependencyTypes: dependencyTypes, | ||
| ); | ||
| if (nestedDeps != null) { | ||
| dependencies.addAll(nestedDeps); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return dependencies; | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This helper is purely about workspace traversal and doesn't depend on anything in this command — it would fit better in |
||
|
|
||
| /// Format type for listing all licenses via --reporter option. | ||
| enum ReporterOutputFormat { | ||
| /// List all licenses separated by a dash. | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,206 @@ | ||||||||||||||||||
| /// A simple parser for pubspec.yaml files. | ||||||||||||||||||
| /// | ||||||||||||||||||
| /// This is used by the `packages check licenses` command to detect workspace | ||||||||||||||||||
| /// configurations and collect dependencies from workspace members. | ||||||||||||||||||
| library; | ||||||||||||||||||
|
|
||||||||||||||||||
| import 'dart:collection'; | ||||||||||||||||||
| import 'dart:io'; | ||||||||||||||||||
|
|
||||||||||||||||||
| import 'package:glob/glob.dart'; | ||||||||||||||||||
| import 'package:glob/list_local_fs.dart'; | ||||||||||||||||||
| import 'package:path/path.dart' as path; | ||||||||||||||||||
| import 'package:yaml/yaml.dart'; | ||||||||||||||||||
|
|
||||||||||||||||||
| /// {@template PubspecParseException} | ||||||||||||||||||
| /// Thrown when a [Pubspec] fails to parse. | ||||||||||||||||||
| /// {@endtemplate} | ||||||||||||||||||
| class PubspecParseException implements Exception { | ||||||||||||||||||
| /// {@macro PubspecParseException} | ||||||||||||||||||
| const PubspecParseException([this.message]); | ||||||||||||||||||
|
|
||||||||||||||||||
| /// The error message. | ||||||||||||||||||
| final String? message; | ||||||||||||||||||
|
|
||||||||||||||||||
| @override | ||||||||||||||||||
| String toString() => message != null | ||||||||||||||||||
| ? 'PubspecParseException: $message' | ||||||||||||||||||
| : 'PubspecParseException'; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /// {@template Pubspec} | ||||||||||||||||||
| /// A representation of a pubspec.yaml file. | ||||||||||||||||||
| /// {@endtemplate} | ||||||||||||||||||
| class Pubspec { | ||||||||||||||||||
| const Pubspec._({ | ||||||||||||||||||
| required this.name, | ||||||||||||||||||
| required this.dependencies, | ||||||||||||||||||
| required this.devDependencies, | ||||||||||||||||||
| required this.workspace, | ||||||||||||||||||
| required this.resolution, | ||||||||||||||||||
| }); | ||||||||||||||||||
|
|
||||||||||||||||||
| /// Parses a [Pubspec] from a string. | ||||||||||||||||||
| /// | ||||||||||||||||||
| /// Throws a [PubspecParseException] if the string cannot be parsed. | ||||||||||||||||||
| factory Pubspec.fromString(String content) { | ||||||||||||||||||
| late final YamlMap yaml; | ||||||||||||||||||
| try { | ||||||||||||||||||
| yaml = loadYaml(content) as YamlMap; | ||||||||||||||||||
| // loadYaml throws TypeError when it fails to cast content as a YamlMap. | ||||||||||||||||||
| // YamlException is thrown when the content is not valid YAML. | ||||||||||||||||||
| // We need to catch both to provide a meaningful exception. | ||||||||||||||||||
| // ignore: avoid_catching_errors | ||||||||||||||||||
| } on TypeError catch (_) { | ||||||||||||||||||
| throw const PubspecParseException('Failed to parse YAML content'); | ||||||||||||||||||
| } on YamlException catch (_) { | ||||||||||||||||||
| throw const PubspecParseException('Failed to parse YAML content'); | ||||||||||||||||||
| } | ||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
since it's the same exception being thrown |
||||||||||||||||||
|
|
||||||||||||||||||
| final name = yaml['name'] as String? ?? ''; | ||||||||||||||||||
|
|
||||||||||||||||||
| final dependencies = _parseDependencies(yaml['dependencies']); | ||||||||||||||||||
| final devDependencies = _parseDependencies(yaml['dev_dependencies']); | ||||||||||||||||||
|
|
||||||||||||||||||
| final workspaceValue = yaml['workspace']; | ||||||||||||||||||
| List<String>? workspace; | ||||||||||||||||||
| if (workspaceValue is YamlList) { | ||||||||||||||||||
| workspace = workspaceValue.cast<String>().toList(); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| final resolutionValue = yaml['resolution']; | ||||||||||||||||||
| PubspecResolution? resolution; | ||||||||||||||||||
| if (resolutionValue is String) { | ||||||||||||||||||
| resolution = PubspecResolution.tryParse(resolutionValue); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| return Pubspec._( | ||||||||||||||||||
| name: name, | ||||||||||||||||||
| dependencies: UnmodifiableListView(dependencies), | ||||||||||||||||||
| devDependencies: UnmodifiableListView(devDependencies), | ||||||||||||||||||
| workspace: workspace != null ? UnmodifiableListView(workspace) : null, | ||||||||||||||||||
| resolution: resolution, | ||||||||||||||||||
| ); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /// Parses a [Pubspec] from a file. | ||||||||||||||||||
| /// | ||||||||||||||||||
| /// Throws a [PubspecParseException] if the file cannot be read or parsed. | ||||||||||||||||||
| factory Pubspec.fromFile(File file) { | ||||||||||||||||||
| if (!file.existsSync()) { | ||||||||||||||||||
| throw PubspecParseException('File not found: ${file.path}'); | ||||||||||||||||||
| } | ||||||||||||||||||
| return Pubspec.fromString(file.readAsStringSync()); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /// The name of the package. | ||||||||||||||||||
| final String name; | ||||||||||||||||||
|
|
||||||||||||||||||
| /// The direct main dependencies. | ||||||||||||||||||
| final UnmodifiableListView<String> dependencies; | ||||||||||||||||||
|
|
||||||||||||||||||
| /// The direct dev dependencies. | ||||||||||||||||||
| final UnmodifiableListView<String> devDependencies; | ||||||||||||||||||
|
|
||||||||||||||||||
| /// The workspace member paths, if this is a workspace root. | ||||||||||||||||||
| /// | ||||||||||||||||||
| /// This is `null` if this pubspec is not a workspace root. | ||||||||||||||||||
| final UnmodifiableListView<String>? workspace; | ||||||||||||||||||
|
|
||||||||||||||||||
| /// The resolution mode for this package. | ||||||||||||||||||
| /// | ||||||||||||||||||
| /// This is `null` if no resolution is specified (typical for standalone | ||||||||||||||||||
| /// packages or workspace roots). | ||||||||||||||||||
| final PubspecResolution? resolution; | ||||||||||||||||||
|
|
||||||||||||||||||
| /// Whether this pubspec is a workspace root. | ||||||||||||||||||
| bool get isWorkspaceRoot => workspace != null; | ||||||||||||||||||
|
|
||||||||||||||||||
| /// Whether this pubspec is a workspace member. | ||||||||||||||||||
| bool get isWorkspaceMember => resolution == PubspecResolution.workspace; | ||||||||||||||||||
|
|
||||||||||||||||||
| /// Resolves workspace member paths to actual directories. | ||||||||||||||||||
| /// | ||||||||||||||||||
| /// This handles glob patterns in workspace paths (e.g., `packages/*`). | ||||||||||||||||||
| /// The [rootDirectory] should be the directory containing this pubspec. | ||||||||||||||||||
| /// | ||||||||||||||||||
| /// Returns an empty list if this is not a workspace root. | ||||||||||||||||||
| List<Directory> resolveWorkspaceMembers(Directory rootDirectory) { | ||||||||||||||||||
| if (workspace == null) return []; | ||||||||||||||||||
|
|
||||||||||||||||||
| final members = <Directory>[]; | ||||||||||||||||||
| for (final pattern in workspace!) { | ||||||||||||||||||
| if (_isGlobPattern(pattern)) { | ||||||||||||||||||
| // Handle glob patterns | ||||||||||||||||||
| final glob = Glob(pattern); | ||||||||||||||||||
| final matches = glob.listSync(root: rootDirectory.path); | ||||||||||||||||||
| for (final match in matches) { | ||||||||||||||||||
| if (match is Directory) { | ||||||||||||||||||
| final pubspecFile = File(path.join(match.path, 'pubspec.yaml')); | ||||||||||||||||||
| if (pubspecFile.existsSync()) { | ||||||||||||||||||
| members.add(Directory(match.path)); | ||||||||||||||||||
| } | ||||||||||||||||||
| } else if (match is File && | ||||||||||||||||||
| path.basename(match.path) == 'pubspec.yaml') { | ||||||||||||||||||
| members.add(Directory(match.parent.path)); | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
| } else { | ||||||||||||||||||
| // Handle direct path | ||||||||||||||||||
| final memberPath = path.join(rootDirectory.path, pattern); | ||||||||||||||||||
| final memberDir = Directory(memberPath); | ||||||||||||||||||
| if (memberDir.existsSync()) { | ||||||||||||||||||
| members.add(memberDir); | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| return members; | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /// Parses dependency names from a YAML dependencies map. | ||||||||||||||||||
| List<String> _parseDependencies(Object? value) { | ||||||||||||||||||
| if (value == null) return []; | ||||||||||||||||||
| if (value is! YamlMap) return []; | ||||||||||||||||||
|
|
||||||||||||||||||
| return value.keys.cast<String>().toList(); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /// Checks if a path pattern contains glob characters. | ||||||||||||||||||
| bool _isGlobPattern(String pattern) { | ||||||||||||||||||
| return pattern.contains('*') || | ||||||||||||||||||
| pattern.contains('?') || | ||||||||||||||||||
| pattern.contains('[') || | ||||||||||||||||||
| pattern.contains('{'); | ||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hand-rolling glob detection is fragile — e.g. negation patterns starting with |
||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /// {@template PubspecResolution} | ||||||||||||||||||
| /// The resolution mode for a pubspec. | ||||||||||||||||||
| /// {@endtemplate} | ||||||||||||||||||
| enum PubspecResolution { | ||||||||||||||||||
| /// This package is a workspace member and should resolve with the workspace | ||||||||||||||||||
| /// root. | ||||||||||||||||||
| workspace._('workspace'), | ||||||||||||||||||
|
|
||||||||||||||||||
| /// This package uses external resolution (e.g., Dart SDK packages). | ||||||||||||||||||
| external._('external'), | ||||||||||||||||||
| ; | ||||||||||||||||||
|
|
||||||||||||||||||
| const PubspecResolution._(this.value); | ||||||||||||||||||
|
|
||||||||||||||||||
| /// Tries to parse a [PubspecResolution] from a string. | ||||||||||||||||||
| /// | ||||||||||||||||||
| /// Returns `null` if the string is not a valid resolution value. | ||||||||||||||||||
| static PubspecResolution? tryParse(String value) { | ||||||||||||||||||
| for (final resolution in PubspecResolution.values) { | ||||||||||||||||||
| if (resolution.value == value) { | ||||||||||||||||||
| return resolution; | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
| return null; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /// The string representation as it appears in pubspec.yaml. | ||||||||||||||||||
| final String value; | ||||||||||||||||||
| } | ||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we have a library for pubspecs, maybe it's more natural for these variables to part of it