diff --git a/lib/src/commands/packages/commands/check/commands/licenses.dart b/lib/src/commands/packages/commands/check/commands/licenses.dart index 7499512ae..a5e23dc1c 100644 --- a/lib/src/commands/packages/commands/check/commands/licenses.dart +++ b/lib/src/commands/packages/commands/check/commands/licenses.dart @@ -69,8 +69,9 @@ const _defaultDetectionThreshold = 0.95; /// Defines a [Map] with dependencies as keys and their licenses as values. /// -/// If a dependency's license failed to be retrieved its license will be `null`. -typedef _DependencyLicenseMap = Map?>; +/// If a dependency's license failed to be retrieved its license will be +/// [SpdxLicense.$unknown]. +typedef _DependencyLicenseMap = Map>; /// Defines a [Map] with banned dependencies as keys and their banned licenses /// as values. @@ -212,41 +213,19 @@ class PackagesCheckLicensesCommand extends Command { usageException('Too many arguments'); } - final target = _argResults.rest.length == 1 ? _argResults.rest[0] : '.'; + final target = _argResults.rest.firstOrNull ?? '.'; final targetPath = path.normalize(Directory(target).absolute.path); + final targetDirectory = Directory(targetPath); - final config = VeryGoodConfig.load(Directory(targetPath), logger: _logger); + final config = VeryGoodConfig.load(targetDirectory, logger: _logger); if (config == null) return ExitCode.config.code; final options = PackagesCheckLicensesOptions.parse( _argResults, config: config, ); + _validateLicenseOptions(options); - final allowedLicenses = options.allowedLicenses; - final forbiddenLicenses = options.forbiddenLicenses; - - if (allowedLicenses.isNotEmpty && forbiddenLicenses.isNotEmpty) { - usageException( - '''Cannot specify both ${styleItalic.wrap('allowed')} and ${styleItalic.wrap('forbidden')} options.''', - ); - } - - final invalidLicenses = _invalidLicenses([ - ...allowedLicenses, - ...forbiddenLicenses, - ]); - if (invalidLicenses.isNotEmpty) { - final documentationLink = link( - uri: licenseDocumentationUri, - message: 'documentation', - ); - _logger.warn( - '''Some licenses failed to be recognized: ${invalidLicenses.stringify()}. Refer to the $documentationLink for a list of valid licenses.''', - ); - } - - final targetDirectory = Directory(targetPath); if (!targetDirectory.existsSync()) { _logger.err( '''Could not find directory at $targetPath. Specify a valid path to a Dart or Flutter project.''', @@ -259,15 +238,7 @@ class PackagesCheckLicensesCommand extends Command { final pubspecLockFile = File(path.join(targetPath, pubspecLockBasename)); if (!pubspecLockFile.existsSync()) { progress.cancel(); - if (declaresWorkspaceResolution(targetDirectory)) { - _logger.err( - 'Could not find a $pubspecLockBasename in $targetPath.\n' - 'This package resolves as part of a Pub workspace. ' - 'Run the command from the workspace root instead.', - ); - } else { - _logger.err('Could not find a $pubspecLockBasename in $targetPath'); - } + _logger.err(_missingPubspecLockMessage(targetDirectory)); return ExitCode.noInput.code; } @@ -278,29 +249,12 @@ class PackagesCheckLicensesCommand extends Command { return ExitCode.noInput.code; } - final resolveWorkspace = - resolveWorkspaceOverride ?? resolveWorkspaceDependencies; - final workspaceDeps = resolveWorkspace(targetDirectory, logger: _logger); - - final filteredDependencies = pubspecLock.packages.where((dependency) { - if (!dependency.isPubHosted) return false; - - if (options.skippedPackages.contains(dependency.name)) return false; - - final dependencyType = workspaceDeps == null - ? dependency.type - : workspaceDeps[dependency.name] ?? PubspecDependencyType.transitive; - return (options.dependencyTypes.contains('direct-main') && - dependencyType == PubspecDependencyType.directMain) || - (options.dependencyTypes.contains('direct-dev') && - dependencyType == PubspecDependencyType.directDev) || - (options.dependencyTypes.contains('transitive') && - dependencyType == PubspecDependencyType.transitive) || - (options.dependencyTypes.contains('direct-overridden') && - dependencyType == PubspecDependencyType.directOverridden); - }); - - if (filteredDependencies.isEmpty) { + final dependencies = _dependenciesToCheck( + pubspecLock, + targetDirectory: targetDirectory, + options: options, + ); + if (dependencies.isEmpty) { progress.cancel(); _logger.info( '''No hosted dependencies found in $targetPath of type: ${options.dependencyTypes.stringify()}.''', @@ -317,101 +271,21 @@ class PackagesCheckLicensesCommand extends Command { return ExitCode.noInput.code; } - final licenses = ?>{}; - final detectLicense = detectLicenseOverride ?? detector.detectLicense; - for (final dependency in filteredDependencies) { - progress.update( - '''Collecting licenses from ${licenses.length + 1} out of ${filteredDependencies.length} ${filteredDependencies.length == 1 ? 'package' : 'packages'}''', - ); - - final dependencyName = dependency.name; - final cachePackageEntry = packageConfig.packages.firstWhereOrNull( - (package) => package.name == dependencyName, + final _DependencyLicenseMap licenses; + try { + licenses = await _collectLicenses( + dependencies, + packageConfig: packageConfig, + options: options, + progress: progress, ); - if (cachePackageEntry == null) { - final errorMessage = - '''[$dependencyName] Could not find cached package path. Consider running `dart pub get` or `flutter pub get` to generate a new `package_config.json`.'''; - if (!options.ignoreRetrievalFailures) { - progress.cancel(); - _logger.err(errorMessage); - return ExitCode.noInput.code; - } - - _logger.err('\n$errorMessage'); - licenses[dependencyName] = {SpdxLicense.$unknown.value}; - continue; - } - - final packagePath = path.normalize(cachePackageEntry.root.toFilePath()); - final packageDirectory = Directory(packagePath); - if (!packageDirectory.existsSync()) { - final errorMessage = - '''[$dependencyName] Could not find package directory at $packagePath.'''; - if (!options.ignoreRetrievalFailures) { - progress.cancel(); - _logger.err(errorMessage); - return ExitCode.noInput.code; - } - - _logger.err('\n$errorMessage'); - licenses[dependencyName] = {SpdxLicense.$unknown.value}; - continue; - } - - final licenseFile = File(path.join(packagePath, 'LICENSE')); - if (!licenseFile.existsSync()) { - licenses[dependencyName] = {SpdxLicense.$unknown.value}; - continue; - } - - final licenseFileContent = licenseFile.readAsStringSync(); - - late final detector.Result detectorResult; - try { - detectorResult = await detectLicense( - licenseFileContent, - _defaultDetectionThreshold, - ); - } on Exception catch (e) { - final errorMessage = - '''[$dependencyName] Failed to detect license from $packagePath: $e'''; - if (!options.ignoreRetrievalFailures) { - progress.cancel(); - _logger.err(errorMessage); - return ExitCode.software.code; - } - - _logger.err('\n$errorMessage'); - licenses[dependencyName] = {SpdxLicense.$unknown.value}; - continue; - } - - final rawLicense = detectorResult.matches - // Accessing license is necessary to get the identifier of the license - // ignore: invalid_use_of_visible_for_testing_member - .map((match) => match.license.identifier) - .toSet(); - licenses[dependencyName] = { - ...rawLicense, - // If there are no matches, we add the unknown license - if (rawLicense.isEmpty) SpdxLicense.$unknown.value, - }; + } on _LicenseRetrievalFailure catch (failure) { + progress.cancel(); + _logger.err(failure.message); + return failure.exitCode.code; } - late final _BannedDependencyLicenseMap? bannedDependencies; - if (allowedLicenses.isNotEmpty) { - bannedDependencies = _bannedDependencies( - licenses: licenses, - isAllowed: allowedLicenses.contains, - ); - } else if (forbiddenLicenses.isNotEmpty) { - bannedDependencies = _bannedDependencies( - licenses: licenses, - isAllowed: (license) => !forbiddenLicenses.contains(license), - ); - } else { - bannedDependencies = null; - } + final bannedDependencies = _bannedDependenciesFor(licenses, options); progress.complete( _composeReport( @@ -428,6 +302,176 @@ class PackagesCheckLicensesCommand extends Command { return ExitCode.success.code; } + + /// Rejects combining allowed and forbidden licenses, and warns about any + /// license that is not a recognized SPDX identifier. + void _validateLicenseOptions(PackagesCheckLicensesOptions options) { + final PackagesCheckLicensesOptions(:allowedLicenses, :forbiddenLicenses) = + options; + + if (allowedLicenses.isNotEmpty && forbiddenLicenses.isNotEmpty) { + usageException( + '''Cannot specify both ${styleItalic.wrap('allowed')} and ${styleItalic.wrap('forbidden')} options.''', + ); + } + + final invalidLicenses = _invalidLicenses([ + ...allowedLicenses, + ...forbiddenLicenses, + ]); + if (invalidLicenses.isEmpty) return; + + final documentationLink = link( + uri: licenseDocumentationUri, + message: 'documentation', + ); + _logger.warn( + '''Some licenses failed to be recognized: ${invalidLicenses.stringify()}. Refer to the $documentationLink for a list of valid licenses.''', + ); + } + + /// The hosted dependencies in [pubspecLock] whose licenses should be + /// checked according to [options]. + /// + /// When [targetDirectory] is part of a Pub workspace, dependency types are + /// resolved from the workspace instead of the [pubspecLock]. + List _dependenciesToCheck( + PubspecLock pubspecLock, { + required Directory targetDirectory, + required PackagesCheckLicensesOptions options, + }) { + final resolveWorkspace = + resolveWorkspaceOverride ?? resolveWorkspaceDependencies; + final workspaceDeps = resolveWorkspace(targetDirectory, logger: _logger); + + PubspecDependencyType typeOf(PubspecLockPackage dependency) => + workspaceDeps == null + ? dependency.type + : workspaceDeps[dependency.name] ?? PubspecDependencyType.transitive; + + return pubspecLock.packages + .where( + (dependency) => + dependency.isPubHosted && + !options.skippedPackages.contains(dependency.name) && + options.dependencyTypes.contains(typeOf(dependency).optionName), + ) + .toList(); + } + + /// Retrieves the licenses of every dependency in [dependencies]. + /// + /// Throws a [_LicenseRetrievalFailure] when a license fails to be retrieved, + /// unless [PackagesCheckLicensesOptions.ignoreRetrievalFailures] is set, in + /// which case the failure is logged and the license is reported as unknown. + Future<_DependencyLicenseMap> _collectLicenses( + List dependencies, { + required package_config.PackageConfig packageConfig, + required PackagesCheckLicensesOptions options, + required Progress progress, + }) async { + final licenses = >{}; + final detectLicense = detectLicenseOverride ?? detector.detectLicense; + final packageWord = dependencies.length == 1 ? 'package' : 'packages'; + + for (final PubspecLockPackage(:name) in dependencies) { + progress.update( + '''Collecting licenses from ${licenses.length + 1} out of ${dependencies.length} $packageWord''', + ); + + try { + licenses[name] = await _retrieveLicenses( + name, + packageConfig: packageConfig, + detectLicense: detectLicense, + ); + } on _LicenseRetrievalFailure catch (failure) { + if (!options.ignoreRetrievalFailures) rethrow; + + _logger.err('\n${failure.message}'); + licenses[name] = {SpdxLicense.$unknown.value}; + } + } + + return licenses; + } +} + +/// Signals that the license of a dependency failed to be retrieved, carrying a +/// human friendly [message] and the [exitCode] to return when not ignored. +class _LicenseRetrievalFailure(final String message, final ExitCode exitCode) + implements Exception; + +/// Retrieves the licenses of the package named [dependencyName] from its +/// cached `LICENSE` file. +/// +/// Returns an unknown license when the package has no `LICENSE` file, or when +/// no license is detected in it. +/// +/// Throws a [_LicenseRetrievalFailure] when the cached package cannot be found +/// or its license fails to be detected. +Future> _retrieveLicenses( + String dependencyName, { + required package_config.PackageConfig packageConfig, + required Future Function(String, double) detectLicense, +}) async { + final cachePackageEntry = packageConfig.packages.firstWhereOrNull( + (package) => package.name == dependencyName, + ); + if (cachePackageEntry == null) { + throw _LicenseRetrievalFailure( + '''[$dependencyName] Could not find cached package path. Consider running `dart pub get` or `flutter pub get` to generate a new `package_config.json`.''', + ExitCode.noInput, + ); + } + + final packagePath = path.normalize(cachePackageEntry.root.toFilePath()); + if (!Directory(packagePath).existsSync()) { + throw _LicenseRetrievalFailure( + '''[$dependencyName] Could not find package directory at $packagePath.''', + ExitCode.noInput, + ); + } + + final licenseFile = File(path.join(packagePath, 'LICENSE')); + if (!licenseFile.existsSync()) return {SpdxLicense.$unknown.value}; + + final licenseFileContent = licenseFile.readAsStringSync(); + + final detector.Result detectorResult; + try { + detectorResult = await detectLicense( + licenseFileContent, + _defaultDetectionThreshold, + ); + } on Exception catch (e) { + throw _LicenseRetrievalFailure( + '''[$dependencyName] Failed to detect license from $packagePath: $e''', + ExitCode.software, + ); + } + + final rawLicense = detectorResult.matches + // Accessing license is necessary to get the identifier of the license + // ignore: invalid_use_of_visible_for_testing_member + .map((match) => match.license.identifier) + .toSet(); + return { + ...rawLicense, + // If there are no matches, we add the unknown license + if (rawLicense.isEmpty) SpdxLicense.$unknown.value, + }; +} + +/// The error message reported when the `pubspec.lock` file is missing from +/// [targetDirectory]. +String _missingPubspecLockMessage(Directory targetDirectory) { + final targetPath = targetDirectory.path; + return declaresWorkspaceResolution(targetDirectory) + ? 'Could not find a $pubspecLockBasename in $targetPath.\n' + 'This package resolves as part of a Pub workspace. ' + 'Run the command from the workspace root instead.' + : 'Could not find a $pubspecLockBasename in $targetPath'; } /// Attempts to parse a [PubspecLock] file in the given [path]. @@ -490,11 +534,7 @@ _BannedDependencyLicenseMap? _bannedDependencies({ required bool Function(String license) isAllowed, }) { _BannedDependencyLicenseMap? bannedDependencies; - for (final dependency in licenses.entries) { - final name = dependency.key; - final license = dependency.value; - if (license == null) continue; - + for (final MapEntry(key: name, value: license) in licenses.entries) { for (final licenseType in license) { if (isAllowed(licenseType)) continue; @@ -507,6 +547,33 @@ _BannedDependencyLicenseMap? _bannedDependencies({ return bannedDependencies; } +/// Returns the banned dependencies in [licenses] according to the allowed or +/// forbidden licenses in [options]. +/// +/// Returns `null` when neither allowed nor forbidden licenses are specified, +/// or when no dependency is banned. +_BannedDependencyLicenseMap? _bannedDependenciesFor( + _DependencyLicenseMap licenses, + PackagesCheckLicensesOptions options, +) { + final PackagesCheckLicensesOptions(:allowedLicenses, :forbiddenLicenses) = + options; + + if (allowedLicenses.isNotEmpty) { + return _bannedDependencies( + licenses: licenses, + isAllowed: allowedLicenses.contains, + ); + } + if (forbiddenLicenses.isNotEmpty) { + return _bannedDependencies( + licenses: licenses, + isAllowed: (license) => !forbiddenLicenses.contains(license), + ); + } + return null; +} + /// Composes a human friendly [String] to report the result of the retrieved /// licenses. /// @@ -517,38 +584,23 @@ String _composeReport({ required _BannedDependencyLicenseMap? bannedDependencies, ReporterOutputFormat? reporterOutputFormat, }) { - final bannedLicenseTypes = bannedDependencies?.values.fold({}, ( - previousValue, - licenses, - ) { - if (licenses.isEmpty) return previousValue; - return previousValue..addAll(licenses); - }); + final bannedLicenseTypes = + bannedDependencies?.values.expand((licenses) => licenses).toSet() ?? + const {}; - final licenseTypes = licenses.values.fold([], ( - previousValue, - licenses, - ) { - if (licenses == null) return previousValue; - return previousValue..addAll(licenses); - }); + final licenseTypes = licenses.values.expand((licenses) => licenses).toList(); + final totalLicenseCount = licenseTypes.length; final licenseCount = {}; for (final license in licenseTypes) { licenseCount.update(license, (value) => value + 1, ifAbsent: () => 1); } - final totalLicenseCount = licenseCount.values.fold( - 0, - (previousValue, count) => previousValue + count, - ); - final formattedLicenseTypes = licenseTypes.toSet().map((license) { - final colorWrapper = - bannedLicenseTypes != null && bannedLicenseTypes.contains(license) + final formattedLicenseTypes = licenseCount.entries.map((entry) { + final MapEntry(key: license, value: count) = entry; + final colorWrapper = bannedLicenseTypes.contains(license) ? red.wrap : green.wrap; - - final count = licenseCount[license]; final formattedCount = darkGray.wrap('($count)'); return '${colorWrapper(license)} $formattedCount'; @@ -559,25 +611,34 @@ String _composeReport({ final suffix = formattedLicenseTypes.isEmpty ? '' : ' of type: ${formattedLicenseTypes.toList().stringify()}'; + final listing = _composeLicenseListing(licenses, reporterOutputFormat); - final licenseBuilder = StringBuffer(); - if (reporterOutputFormat case final ReporterOutputFormat outputFormat) { - licenseBuilder.write('\n'); - for (final license in licenses.entries) { - if (license.value case final Set dependencyLicenses) { - for (final dependencyLicense in dependencyLicenses) { - licenseBuilder.writeln( - outputFormat.formatLicense( - packageName: license.key, - licenseName: dependencyLicense, - ), - ); - } - } + return '''Retrieved $totalLicenseCount $licenseWord from ${licenses.length} $packageWord$suffix.$listing'''; +} + +/// Lists every license of every dependency in [licenses] using +/// [reporterOutputFormat], one per line. +/// +/// Returns an empty [String] when no [reporterOutputFormat] is given. +String _composeLicenseListing( + _DependencyLicenseMap licenses, + ReporterOutputFormat? reporterOutputFormat, +) { + if (reporterOutputFormat == null) return ''; + + final listing = StringBuffer('\n'); + for (final MapEntry(key: packageName, value: dependencyLicenses) + in licenses.entries) { + for (final licenseName in dependencyLicenses) { + listing.writeln( + reporterOutputFormat.formatLicense( + packageName: packageName, + licenseName: licenseName, + ), + ); } } - - return '''Retrieved $totalLicenseCount $licenseWord from ${licenses.length} $packageWord$suffix.$licenseBuilder'''; + return listing.toString(); } String _composeBannedReport(_BannedDependencyLicenseMap bannedDependencies) { diff --git a/lib/src/pubspec/pubspec.dart b/lib/src/pubspec/pubspec.dart index 22abbced5..f02ea44c9 100644 --- a/lib/src/pubspec/pubspec.dart +++ b/lib/src/pubspec/pubspec.dart @@ -23,14 +23,14 @@ enum PubspecDependencyType { /// See also: /// /// * [Dart's dependency documentation](https://dart.dev/tools/pub/dependencies) - directMain._('direct main'), + directMain._('direct main', 'direct-main'), /// Another package that your package needs during development. /// /// See also: /// /// * [Dart's developer dependency documentation](https://dart.dev/tools/pub/dependencies#dev-dependencies) - directDev._('direct dev'), + directDev._('direct dev', 'direct-dev'), /// A dependency that your package indirectly uses because one of its /// dependencies requires it. @@ -38,7 +38,7 @@ enum PubspecDependencyType { /// See also: /// /// * [Dart's transitive dependency documentation](https://dart.dev/tools/pub/glossary#transitive-) - transitive._('transitive'), + transitive._('transitive', 'transitive'), /// A dependency that your package overrides that is not already a /// `direct main` or `direct dev` dependency. @@ -46,9 +46,9 @@ enum PubspecDependencyType { /// See also: /// /// * [Dart's dependency override documentation](https://dart.dev/tools/pub/dependencies#dependency-overrides) - directOverridden._('direct overridden'); + directOverridden._('direct overridden', 'direct-overridden'); - new _(this.value); + new _(this.value, this.optionName); /// Parses a [PubspecDependencyType] from its `pubspec.lock` textual form. /// @@ -70,6 +70,10 @@ enum PubspecDependencyType { /// The textual representation of the [PubspecDependencyType] as it appears in /// the `dependency` field of a `pubspec.lock` file. final String value; + + /// The `--dependency-type` option value of the `packages check licenses` + /// command that selects this dependency type. + final String optionName; } /// Tolerantly parses a [Pubspec] from [pubspecFile]. diff --git a/lib/src/pubspec_workspace/pubspec_workspace.dart b/lib/src/pubspec_workspace/pubspec_workspace.dart index 6cb09e32a..f8ef0b525 100644 --- a/lib/src/pubspec_workspace/pubspec_workspace.dart +++ b/lib/src/pubspec_workspace/pubspec_workspace.dart @@ -19,10 +19,13 @@ import 'dart:io'; import 'package:glob/glob.dart'; import 'package:glob/list_local_fs.dart'; import 'package:mason_logger/mason_logger.dart'; +import 'package:meta/meta.dart'; import 'package:path/path.dart' as path; import 'package:very_good_cli/src/pubspec/pubspec.dart'; import 'package:yaml/yaml.dart'; +part 'workspace_dependency_collector.dart'; + /// The basename of a pubspec file. const _pubspecBasename = 'pubspec.yaml'; @@ -66,9 +69,13 @@ final _globCharacters = RegExp(r'[*?\[\]{}]'); /// back to the lock's own classification (non-workspace behavior). A present /// but unparseable root pubspec logs a warning via [logger] before returning /// `null`. [logger] also receives a warning for every skipped member. +/// +/// The workspace is walked by [collector], which defaults to a fresh +/// [WorkspaceDependencyCollector] reporting to [logger]. Map? resolveWorkspaceDependencies( Directory rootDirectory, { required Logger logger, + @visibleForTesting WorkspaceDependencyCollector? collector, }) { final rootPubspecFile = File(path.join(rootDirectory.path, _pubspecBasename)); @@ -87,71 +94,10 @@ Map? resolveWorkspaceDependencies( final workspace = rootPubspec.workspace; if (workspace == null || workspace.isEmpty) return null; - final visited = {}; - final directDev = {}; - final directMain = {}; - final directOverridden = {}; - - void visit(Directory directory, Pubspec pubspec) { - if (!visited.add(directory.resolveSymbolicLinksSync())) return; - - directMain.addAll(pubspec.dependencies.keys); - directDev.addAll(pubspec.devDependencies.keys); - directOverridden.addAll(pubspec.dependencyOverrides.keys); - - for (final entry in pubspec.workspace ?? const []) { - final isLiteral = !_globCharacters.hasMatch(entry); - for (final memberDirectory in _expandMembers( - directory, - entry, - isLiteral: isLiteral, - logger: logger, - )) { - final memberPubspecFile = File( - path.join(memberDirectory.path, _pubspecBasename), - ); - final memberPubspec = _tryParsePubspecWithOverrides(memberDirectory); - if (memberPubspec == null) { - // Silently skip glob-matched directories that don't contain a - // pubspec.yaml — a common `packages/*` workspace should not warn - // for documentation or fixture folders sitting next to packages. - // For literal entries, or for glob-matched directories where a - // pubspec.yaml IS present but unparseable, keep the warning so - // real misconfigurations are still surfaced. - if (isLiteral || memberPubspecFile.existsSync()) { - logger.warn( - '''Skipping workspace member at ${memberDirectory.path}: missing or unparseable $_pubspecBasename.''', - ); - } - continue; - } - visit(memberDirectory, memberPubspec); - } - } - } - - visit(rootDirectory, rootPubspec); - - // Build highest precedence first so lower-precedence writes of the same name - // are no-ops: directMain > directDev > directOverridden. - final dependencies = {}; - - for (final name in directMain) { - dependencies[name] = PubspecDependencyType.directMain; - } - - for (final name in directDev) { - dependencies.putIfAbsent(name, () => PubspecDependencyType.directDev); - } - - for (final name in directOverridden) { - dependencies.putIfAbsent( - name, - () => PubspecDependencyType.directOverridden, - ); - } - - return dependencies; + final workspaceCollector = + (collector ?? WorkspaceDependencyCollector(logger: logger)) + ..visit(rootDirectory, rootPubspec); + return workspaceCollector.classify(); } /// Whether the package rooted at [directory] declares `resolution: workspace`, diff --git a/lib/src/pubspec_workspace/workspace_dependency_collector.dart b/lib/src/pubspec_workspace/workspace_dependency_collector.dart new file mode 100644 index 000000000..619f3a0d5 --- /dev/null +++ b/lib/src/pubspec_workspace/workspace_dependency_collector.dart @@ -0,0 +1,89 @@ +part of 'pubspec_workspace.dart'; + +/// Walks a Pub workspace depth-first and collects the dependency names each +/// visited package declares, grouped by declaration kind. +/// +/// A collector keeps track of the packages it has visited, so each instance +/// is meant to walk a single workspace. +class WorkspaceDependencyCollector({ + /// Receives a warning for every skipped workspace member. + required final Logger logger, +}) { + /// Creates a collector that reports skipped members to [logger]. + this; + + final _visited = {}; + final _directMain = {}; + final _directDev = {}; + final _directOverridden = {}; + + /// Records the dependencies of [pubspec] and recurses into its members. + /// + /// Each directory is visited at most once, keyed by its resolved path. + void visit(Directory directory, Pubspec pubspec) { + if (!_visited.add(directory.resolveSymbolicLinksSync())) return; + + _directMain.addAll(pubspec.dependencies.keys); + _directDev.addAll(pubspec.devDependencies.keys); + _directOverridden.addAll(pubspec.dependencyOverrides.keys); + + for (final entry in pubspec.workspace ?? const []) { + _visitEntry(directory, entry); + } + } + + /// Visits every member directory matched by a single `workspace:` [entry]. + void _visitEntry(Directory directory, String entry) { + final isLiteral = !_globCharacters.hasMatch(entry); + final memberDirectories = _expandMembers( + directory, + entry, + isLiteral: isLiteral, + logger: logger, + ); + for (final memberDirectory in memberDirectories) { + final memberPubspec = _parseMember(memberDirectory, isLiteral: isLiteral); + if (memberPubspec != null) visit(memberDirectory, memberPubspec); + } + } + + /// Parses the member pubspec at [memberDirectory], warning when it is + /// skipped. + /// + /// Glob-matched directories without a pubspec.yaml are skipped silently, so + /// a common `packages/*` workspace does not warn for documentation or + /// fixture folders sitting next to packages. Literal entries, and + /// glob-matched directories whose pubspec.yaml is present but unparseable, + /// keep the warning so real misconfigurations are still surfaced. + Pubspec? _parseMember(Directory memberDirectory, {required bool isLiteral}) { + final memberPubspec = _tryParsePubspecWithOverrides(memberDirectory); + final memberPubspecFile = File( + path.join(memberDirectory.path, _pubspecBasename), + ); + if (memberPubspec == null && + (isLiteral || memberPubspecFile.existsSync())) { + logger.warn( + '''Skipping workspace member at ${memberDirectory.path}: missing or unparseable $_pubspecBasename.''', + ); + } + return memberPubspec; + } + + /// Maps every collected name to its workspace-wide type. + /// + /// Builds highest precedence first so lower-precedence writes of the same + /// name are no-ops: directMain > directDev > directOverridden. + Map classify() { + final dependencies = {}; + for (final (names, type) in [ + (_directMain, PubspecDependencyType.directMain), + (_directDev, PubspecDependencyType.directDev), + (_directOverridden, PubspecDependencyType.directOverridden), + ]) { + for (final name in names) { + dependencies.putIfAbsent(name, () => type); + } + } + return dependencies; + } +} diff --git a/lib/src/very_good_config/very_good_config.dart b/lib/src/very_good_config/very_good_config.dart index 1b5ebdb59..e0e7a234a 100644 --- a/lib/src/very_good_config/very_good_config.dart +++ b/lib/src/very_good_config/very_good_config.dart @@ -14,6 +14,8 @@ import 'package:equatable/equatable.dart'; import 'package:json_annotation/json_annotation.dart'; import 'package:mason_logger/mason_logger.dart'; import 'package:path/path.dart' as path; +import 'package:very_good_cli/src/pubspec/pubspec.dart' + show PubspecDependencyType; part 'very_good_config.g.dart'; @@ -747,11 +749,8 @@ const collectCoverageFromAllowedValues = ['imports', 'all']; /// The dependency types accepted by `very_good packages check licenses`, shared /// between the CLI argument parser and the `very_good.yaml` validator so they /// cannot drift apart. -const dependencyTypeAllowedValues = [ - 'direct-main', - 'direct-dev', - 'direct-overridden', - 'transitive', +final List dependencyTypeAllowedValues = [ + for (final type in PubspecDependencyType.values) type.optionName, ]; /// The values accepted by the license `reporter` option, shared between the diff --git a/test/src/pubspec/pubspec_test.dart b/test/src/pubspec/pubspec_test.dart index cfbf2a043..d07138819 100644 --- a/test/src/pubspec/pubspec_test.dart +++ b/test/src/pubspec/pubspec_test.dart @@ -42,6 +42,21 @@ void main() { ); }); }); + + test('optionName matches the --dependency-type option values', () { + expect( + { + for (final type in PubspecDependencyType.values) + type: type.optionName, + }, + equals({ + PubspecDependencyType.directMain: 'direct-main', + PubspecDependencyType.directDev: 'direct-dev', + PubspecDependencyType.transitive: 'transitive', + PubspecDependencyType.directOverridden: 'direct-overridden', + }), + ); + }); }); group('tryParsePubspec', () { diff --git a/test/src/pubspec_workspace/pubspec_workspace_test.dart b/test/src/pubspec_workspace/pubspec_workspace_test.dart index 4f4c0d231..e6793c977 100644 --- a/test/src/pubspec_workspace/pubspec_workspace_test.dart +++ b/test/src/pubspec_workspace/pubspec_workspace_test.dart @@ -9,7 +9,19 @@ import 'package:very_good_cli/src/pubspec_workspace/pubspec_workspace.dart'; class _MockLogger extends Mock implements Logger; +class _MockWorkspaceDependencyCollector extends Mock + implements WorkspaceDependencyCollector; + +class _FakeDirectory extends Fake implements Directory; + +class _FakePubspec extends Fake implements Pubspec; + void main() { + setUpAll(() { + registerFallbackValue(_FakeDirectory()); + registerFallbackValue(_FakePubspec()); + }); + /// Writes a `pubspec.yaml` with [content] into a subdirectory [name] /// (which maybe a nested path) of [root], creating directories as needed. Directory writePubspec(Directory root, String name, String content) { @@ -735,6 +747,70 @@ environment: expect(result, isEmpty); }); + + test('walks the workspace with the injected collector', () { + writePubspec(tempDirectory, '.', ''' +name: root +environment: + sdk: ^3.11.0 +workspace: + - packages/a +'''); + final collector = _MockWorkspaceDependencyCollector(); + const dependencies = {'http': PubspecDependencyType.directMain}; + when(collector.classify).thenReturn(dependencies); + + final result = resolveWorkspaceDependencies( + tempDirectory, + logger: logger, + collector: collector, + ); + + expect(result, equals(dependencies)); + final visitedDirectory = + verify(() => collector.visit(captureAny(), any())).captured.single + as Directory; + expect(visitedDirectory.path, equals(tempDirectory.path)); + }); + }); + + group(WorkspaceDependencyCollector, () { + late Directory tempDirectory; + late Logger logger; + + setUp(() { + tempDirectory = Directory.systemTemp.createTempSync(); + addTearDown(() => tempDirectory.deleteSync(recursive: true)); + logger = _MockLogger(); + }); + + test('classify returns an empty map before any visit', () { + expect(WorkspaceDependencyCollector(logger: logger).classify(), isEmpty); + }); + + test('visits each directory at most once', () { + final pubspec = Pubspec( + 'root', + dependencies: {'http': HostedDependency()}, + devDependencies: {'test': HostedDependency()}, + dependencyOverrides: {'meta': HostedDependency()}, + ); + final collector = WorkspaceDependencyCollector(logger: logger) + ..visit(tempDirectory, pubspec) + ..visit( + tempDirectory, + Pubspec('root', dependencies: {'args': HostedDependency()}), + ); + + expect( + collector.classify(), + equals({ + 'http': PubspecDependencyType.directMain, + 'test': PubspecDependencyType.directDev, + 'meta': PubspecDependencyType.directOverridden, + }), + ); + }); }); group('declaresWorkspaceResolution', () {