feat: add workspace support for packages check licenses#1540
feat: add workspace support for packages check licenses#1540realmeylisdev wants to merge 19 commits into
Conversation
When a pubspec.yaml declares a workspace property, the command now recursively collects dependencies from all workspace members and checks their licenses using the root pubspec.lock. This enables license checking in monorepo projects that use Dart's pub workspace feature. Closes VeryGoodOpenSource#1273
|
@realmeylisdev seems like there are some conflicts that needs to be resolved |
Keep both workspace license support functions and ReporterOutputFormat enum from main.
|
now done. |
|
@marcossevilla could we get this PR reviewed which was de-prioritized last time ? #1444 (comment) |
hi @samitsv, I'll take a look during this week, thanks for pushing this! also mentioning @brianegan as he was having this issue too, a review or test to check if it's fixing your issue is also appreciated 👍 |
|
any update on this @marcossevilla |
|
any update on this @marcossevilla . Would be great to have this merged soon. |
|
hey @samitsv, we're currently trying the branch against internal repos to verify the changes, I'll leave an update once we finish testing |
|
@marcossevilla Functional testing done on my end! PR worked well for a workspace monorepo. Sorry for the delay here -- deadlines. |
| @visibleForTesting | ||
| const pubspecLockBasename = 'pubspec.lock'; | ||
|
|
||
| /// The basename of the pubspec file. | ||
| @visibleForTesting | ||
| const pubspecBasename = 'pubspec.yaml'; |
There was a problem hiding this comment.
if we have a library for pubspecs, maybe it's more natural for these variables to part of it
| /// 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; | ||
| } | ||
| } |
There was a problem hiding this comment.
same here, maybe it's better if it's part of pubspec library
| } on TypeError catch (_) { | ||
| throw const PubspecParseException('Failed to parse YAML content'); | ||
| } on YamlException catch (_) { | ||
| throw const PubspecParseException('Failed to parse YAML content'); | ||
| } |
There was a problem hiding this comment.
| } on TypeError catch (_) { | |
| throw const PubspecParseException('Failed to parse YAML content'); | |
| } on YamlException catch (_) { | |
| throw const PubspecParseException('Failed to parse YAML content'); | |
| } | |
| } on Exception catch (_) { | |
| throw const PubspecParseException('Failed to parse YAML content'); | |
| } |
since it's the same exception being thrown
| import 'package:very_good_cli/src/pubspec/pubspec.dart'; | ||
|
|
||
| void main() { | ||
| group('$Pubspec', () { |
There was a problem hiding this comment.
| group('$Pubspec', () { | |
| group(Pubspec, () { |
| }); | ||
| }); | ||
|
|
||
| group('$PubspecParseException', () { |
There was a problem hiding this comment.
| group('$PubspecParseException', () { | |
| group(PubspecParseException, () { |
| }); | ||
| }); | ||
|
|
||
| group('$PubspecResolution', () { |
There was a problem hiding this comment.
| group('$PubspecResolution', () { | |
| group(PubspecResolution, () { |
Address review feedback on PR VeryGoodOpenSource#1540: - Move `pubspecBasename` constant from licenses.dart to pubspec library - Replace private `_tryParsePubspec` helper with `Pubspec.tryParse` static - Simplify `Pubspec.fromString` by replacing the throwing `as` cast with an `is!` check, removing the `TypeError` catch and lint suppression - Pass class types directly to `group()` in pubspec_test.dart
|
@realmeylisdev CI is failing, also found we could just use https://pub.dev/packages/pubspec_parse instead of creating a pubspec library |
CI Failure AnalysisThe Root CauseThe docs_site e2e test generates a Docusaurus project and runs This is an upstream Docusaurus/webpack incompatibility in the PR Code Verification
|
|
hey @realmeylisdev, we need to use |
|
hey @realmeylisdev, are you able to work on the changes? otherwise we could close the PR and the team will tackle this issue separately |
Replaces the hand-rolled Pubspec parser with `package:pubspec_parse`, which is already a direct dependency. Keeps only the workspace helpers (`tryParsePubspec`, `PubspecWorkspace` extension, `resolveWorkspaceMembers`) that pubspec_parse does not provide. Addresses review feedback on VeryGoodOpenSource#1540.
|
Hey @marcossevilla — swapped the custom parser for The two failing e2e checks ( Ready for another look. |
|
Can we please get this reviewed :) @marcossevilla |
|
@realmeylisdev CI should be fixed, you can update your branch to get the latest changes |
done. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
@vgvbot perform a code review focused on verifying nits and code readability |
|
Code review submitted. |
![very-good-code-bot[bot]](https://avatars.githubusercontent.com/in/3144111?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR adds workspace support to packages check licenses, which is a useful feature with reasonable test coverage. Overall the approach (detect a workspace root, walk member pubspecs, build a Set of declared direct deps to filter pubspec.lock by name) is workable and backwards-compatible. A few concerns worth addressing:
Concerns
_collectWorkspaceDependenciesrecurses into nested workspaces with no cycle protection. A pubspec that lists its own directory (or a parent) inworkspace:would cause unbounded recursion.Pubspec.parse(..., lenient: true)accepts many malformed pubspecs without throwing. The malformed-pubspec test only passes because the YAML itself is unparsable; a structurally invalid but YAML-valid pubspec would silently parse with empty/default dep maps and contribute nothing — i.e. workspace members would be silently dropped.PubspecWorkspace.isWorkspaceMemberis dead production code (only referenced by tests). Either drop it or use it to validate that resolved members declareresolution: workspace.- The new workspace-aware filter and the helper that drives it (
_collectWorkspaceDependencies) live inlicenses.darteven though the logic is generic workspace-walking; moving it intolib/src/pubspec/pubspec.dart(or a sibling) would keep the command file focused and let other commands reuse it. - The branch around
dependencyTypes.contains('direct-main') || dependencyTypes.contains('direct-dev')is redundant onceworkspaceDependencieswas already filtered by those types — the outer if can be removed for clarity, or the membership Sets should be tracked per-type to actually enforce the distinction. - Test gaps: no coverage for (a) a dep declared as
direct-mainin one member anddirect-devin another (relevant since the merged Set loses that distinction), (b)_isGlobPattern's?/[branches, (c) cycle protection, and (d) a workspace member that also declaresresolution: workspacemismatch. _isGlobPatternis a hand-rolled heuristic; pkg:glob can be queried more directly, and patterns like!packages/foo(negation) won't be classified as glob.
Positives
- Clean separation between
pubspec.darthelpers and the command layer. - Backwards-compatibility is preserved: when no
pubspec.yamlis present or it isn't a workspace root, the original filtering path is used unchanged. - Good integration test coverage for the common workspace shapes (direct-main/dev, transitive, overridden, nested, and non-workspace).
|
|
||
| // Handle nested workspaces recursively | ||
| if (memberPubspec.isWorkspaceRoot) { | ||
| final nestedDeps = _collectWorkspaceDependencies( |
There was a problem hiding this comment.
There is no cycle protection here. If a member's pubspec.yaml lists the parent (or itself) under workspace: — e.g. via a misconfigured glob like .. — this recursion will not terminate. Consider passing a Set<String> of canonicalized directory paths and short-circuiting if memberDirectory.absolute.path has already been visited.
| // 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') || |
There was a problem hiding this comment.
This outer if is redundant given the way workspaceDependencies is built — the Set only contains names whose declared type is in dependencyTypes, so the inner contains already encodes the filter. Either drop this branch (and just check workspaceDependencies.contains(dependency.name)) or, if you want to actually enforce direct-main vs direct-dev separately, change _collectWorkspaceDependencies to return per-type Sets. As written it implies a distinction that isn't really being made.
| return false; | ||
| } | ||
|
|
||
| // Non-workspace: use the original filtering logic |
There was a problem hiding this comment.
Subtle behavioral divergence worth calling out in a comment or doc: in non-workspace mode, direct-main is decided by pubspec.lock's recorded type; in workspace mode, it's decided by which members declare it. For overlapping packages (a dep that's direct dev in pubspec.lock but appears in some member's dependencies:) the two paths can disagree. Probably fine, but users will notice.
| bool get isWorkspaceRoot => workspace != null; | ||
|
|
||
| /// Whether this pubspec is a workspace member. | ||
| bool get isWorkspaceMember => resolution == _workspaceResolution; |
There was a problem hiding this comment.
isWorkspaceMember isn't referenced anywhere in lib/; only the tests call it. Either delete it (YAGNI), or wire it into resolveWorkspaceMembers/_collectWorkspaceDependencies to warn/skip when a resolved member doesn't declare resolution: workspace — that would catch real misconfigurations rather than just being inert.
| return Pubspec.parse( | ||
| file.readAsStringSync(), | ||
| sourceUrl: file.uri, | ||
| lenient: true, |
There was a problem hiding this comment.
lenient: true causes Pubspec.parse to accept many structurally invalid pubspecs without throwing. That means tryParsePubspec only returns null for true YAML-level errors — an otherwise well-formed YAML doc with bad dependencies: shape will parse to a Pubspec with empty maps and we'll silently skip its deps. The malformed-pubspec integration test exercises the YAML-error path; consider either dropping lenient: true (so structural errors are surfaced as well), or logging a warning when a workspace member fails to parse so users aren't left wondering why a member's deps are missing.
| final members = <Directory>[]; | ||
| for (final pattern in patterns) { | ||
| if (_isGlobPattern(pattern)) { | ||
| final matches = Glob(pattern).listSync(root: root.path); |
There was a problem hiding this comment.
Workspace patterns under pub use POSIX-style separators, but on Windows users may write packages\app (or it may end up that way after normalization). Glob is POSIX-only — consider normalizing patterns to POSIX before constructing the Glob, or document the constraint.
| } | ||
|
|
||
| return dependencies; | ||
| } |
There was a problem hiding this comment.
This helper is purely about workspace traversal and doesn't depend on anything in this command — it would fit better in lib/src/pubspec/pubspec.dart (or a sibling) alongside resolveWorkspaceMembers, keeping the licenses command focused. That also makes it easier to unit-test directly rather than only through the integration tests.
|
|
||
| expect(result, equals(ExitCode.success.code)); | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Missing test for the case where the same dep name appears as direct-main in one workspace member and direct-dev in another. Since _collectWorkspaceDependencies merges into a single Set<String>, this is exactly the case where the lost distinction could matter — worth a test that pins down the intended behavior.
| test('returns null when the file contains invalid YAML', () { | ||
| final file = File(path.join(tempDirectory.path, pubspecBasename)) | ||
| ..writeAsStringSync('invalid: yaml: content: ['); | ||
| expect(tryParsePubspec(file), isNull); |
There was a problem hiding this comment.
This test passes because 'invalid: yaml: content: [' is unparseable at the YAML level. Given lenient: true, it does not actually exercise the case of a YAML-valid but pubspec-invalid file (e.g. name: foo\ndependencies: not-a-map). Worth adding a case for that to confirm whatever behavior you want — currently it would parse successfully and return an effectively-empty Pubspec.
| } | ||
|
|
||
| // Check if this is a workspace root and collect dependencies accordingly | ||
| final pubspecFile = File(path.join(targetPath, pubspecBasename)); |
There was a problem hiding this comment.
Minor: you've already computed targetPath and targetDirectory. path.join(targetDirectory.path, pubspecBasename) would avoid recomputing the join from targetPath and keeps the single source of truth (targetDirectory). Same comment applies to pubspecLockFile above.
| /// Attempts to read and parse a [Pubspec] from [file]. | ||
| /// | ||
| /// Returns `null` when the file does not exist or cannot be parsed. | ||
| Pubspec? tryParsePubspec(File file) { |
There was a problem hiding this comment.
additional to the bot's comments, I'd say all the pubspec related methods can be in the existing PubspecWorkspace extension since we're just extending pubspec_parse
- Move resolveWorkspaceMembers and dependency collection into the PubspecWorkspace extension on Pubspec, keeping pubspec helpers in the pubspec library. - Add cycle protection to collectWorkspaceDependencies via a visited set of canonical paths to prevent unbounded recursion when a member references a parent. - Drop the redundant outer dependency-type guard in licenses.dart now that workspaceDependencies is already filtered at collection time. - Use targetDirectory.path consistently when joining the pubspec and pubspec.lock paths. - Cover the merged-membership case (dep listed as direct-main in one member and direct-dev in another) and circular workspace references with new tests; rename groups to track the renamed APIs.
|
hey @realmeylisdev, will you be able to apply the comments on this PR? thanks! |
- Drop `lenient: true` from `tryParsePubspec` so structurally invalid
pubspecs (missing `name`, malformed dep maps) fall through to the
exception handler instead of silently parsing with empty dep maps.
- Remove the unused `PubspecWorkspace.isWorkspaceMember` getter and
`_workspaceResolution` constant; only tests referenced it.
- Drop the hand-rolled `_isGlobPattern` heuristic and route every
workspace pattern through `Glob.listSync`. Literal paths match as
exact entries, so the previous direct-path branch (which skipped
the `pubspec.yaml` check) becomes unnecessary.
- Cover the new behavior: structurally invalid pubspecs, `?`/`[...]`/
`{...}` glob shapes, literal paths with non-metachar symbols, and
workspace members that omit `resolution: workspace`.
|
@marcossevilla applied the remaining bot review nits in d5668ab:
Ready for another look. |
|
@vgvbot check the changes from your last review, resolve the comments that have been addressed and leave any comments if necessary |
|
Code review submitted. |
There was a problem hiding this comment.
Overall
This PR adds workspace support to packages check licenses, which fills a real gap for monorepo users. The approach — walk member pubspecs to build a union of direct deps, then intersect with the workspace's pubspec.lock — is reasonable. Test coverage is broad, covering globs (*, ?, [...], {...}), nested workspaces, malformed members, and backwards compatibility.
Main concerns
- Duplicated filter logic in
licenses.dart. The new workspace branch (lines 212–227) re-implements thetransitive/direct-overriddenchecks that already exist in the non-workspace branch (lines 234–238). The two paths now have to stay in sync. - Dual signal/data return in
collectWorkspaceDependencies.nullmeans "not a workspace," a non-nullSetmeans "workspace deps." SinceisWorkspaceRootalready exists as a separate getter, this overloading is unnecessary and makes the call site read awkwardly (workspaceDependencies != nullis really asking "are we in workspace mode?"). - Redundant traversal on nested workspaces. When a member is itself a workspace root, its
dependencies/devDependenciesare added by the outer loop and by the recursive call's prologue.Setdedupes, but the data flow is harder to follow than it should be. dependency_overridesis silently ignored when collecting workspace deps. Members that override deps will rely on the lock file'sdirect-overriddenmarker — which is fine in practice but worth either documenting in the dartdoc or handling explicitly.- Unnecessary churn at line 183: changing
targetPathtotargetDirectory.pathdoesn't change behavior (they're the same string) and bloats the diff.
Smaller notes
- Several test fixtures collide on the pubspec
namefield (e.g._workspaceMemberSharedPubspecContentis used forappDir, where it producesname: shared), which makes the test scenarios harder to parse. tryParsePubspeccatchesExceptiononly;Pubspec.parsepaths go throughParsedYamlException/YamlExceptionwhich subclassFormatException, so this is correct — but the doc comment could state that explicitly.- Cross-platform:
Glob(pattern).listSync(root: root.path)will use the platformpath.Contextby default. Workspace globs in real pubspecs always use/, which works, but worth a quick sanity check on Windows CI before shipping.
|
|
||
| final pubspecLockFile = File(path.join(targetPath, pubspecLockBasename)); | ||
| final pubspecLockFile = File( | ||
| path.join(targetDirectory.path, pubspecLockBasename), |
There was a problem hiding this comment.
targetDirectory.path and targetPath are the same string here (targetDirectory = Directory(targetPath) two lines above). This rename adds diff noise without changing behavior — consider reverting so the diff stays focused on the workspace feature.
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
The transitive and direct-overridden checks in this workspace branch duplicate the ones in the non-workspace branch below (lines 234–238). Now both paths need to be kept in sync whenever a new dependency-type is added. Consider folding this into a single filter that uses workspaceDependencies only to augment direct-main/direct-dev matching, e.g.:
final bool inWorkspaceDeps = workspaceDependencies?.contains(dependency.name) ?? false;
return (inWorkspaceDeps) ||
(workspaceDependencies == null && dependencyTypes.contains('direct-main') && dependencyType == ...directMain) ||
(workspaceDependencies == null && dependencyTypes.contains('direct-dev') && dependencyType == ...directDev) ||
(dependencyTypes.contains('transitive') && dependencyType == ...transitive) ||
(dependencyTypes.contains('direct-overridden') && dependencyType == ...directOverridden);That keeps the transitive/overridden handling in one place.
| final workspaceDependencies = pubspec?.collectWorkspaceDependencies( | ||
| root: targetDirectory, | ||
| dependencyTypes: dependencyTypes, | ||
| ); |
There was a problem hiding this comment.
Two coupled signals are being smuggled through workspaceDependencies: (a) whether the project is a workspace root and (b) the actual dep set. Consider making collectWorkspaceDependencies return a Set<String> unconditionally (empty when not a workspace) and read the boolean directly via pubspec?.isWorkspaceRoot ?? false. That removes the != null guard at the call site and the mixed-purpose null return in the extension.
| } | ||
| if (dependencyTypes.contains('direct-dev')) { | ||
| deps.addAll(memberPubspec.devDependencies.keys); | ||
| } |
There was a problem hiding this comment.
When memberPubspec is itself a workspace root, its dependencies/devDependencies get added here (lines 86–91), and then the recursive call at line 94 re-adds them via lines 72–77 of the recursion. The Set deduplicates so there's no correctness bug, but the data flow is confusing — readers have to convince themselves the duplicate add is harmless. Consider either skipping the direct add when memberPubspec.isWorkspaceRoot (let the recursion handle it) or having the recursive call only walk deeper members (i.e. drop lines 72–77 from the recursive path).
| } | ||
| if (dependencyTypes.contains('direct-dev')) { | ||
| deps.addAll(memberPubspec.devDependencies.keys); | ||
| } |
There was a problem hiding this comment.
dependencyOverrides is never inspected. In workspace mode, the filter relies on the lock file's direct-overridden marker to surface overrides, but a reader of this extension can't tell that from the code or doc. Worth either: (a) adding dependencyOverrides.keys here under a 'direct-overridden' branch for symmetry with the non-workspace path, or (b) calling out the intentional limitation in the dartdoc on collectWorkspaceDependencies.
|
|
||
| final members = <Directory>[]; | ||
| for (final pattern in patterns) { | ||
| final matches = Glob(pattern).listSync(root: root.path); |
There was a problem hiding this comment.
Glob(pattern).listSync(root: root.path) uses the platform's default path.Context. Pub workspace globs are always POSIX-style (forward slashes) per the pub spec, but on Windows the default Context is windows. Worth either pinning Glob(pattern, context: p.posix) for portability, or at minimum verifying behavior on Windows CI before release. (Same applies to test fixtures using packages/app literals.)
| } on Exception { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Catching Exception is correct here (Pubspec.parse throws ParsedYamlException/YamlException, both FormatExceptions), but the dartdoc currently just says "cannot be parsed" — it's worth being explicit that this swallows all parse failures silently. Callers (notably collectWorkspaceDependencies) skip these members without logging, which could make broken member pubspecs invisible. Consider returning a richer signal or at least documenting that malformed members are silently skipped.
| path.join(tempDirectory.path, 'packages', 'app'), | ||
| )..createSync(recursive: true); | ||
| File(path.join(appDir.path, 'pubspec.yaml')).writeAsStringSync( | ||
| _workspaceMemberSharedPubspecContent, |
There was a problem hiding this comment.
This is confusing: _workspaceMemberSharedPubspecContent is named shared inside, but it's being written to appDir. So appDir/pubspec.yaml declares name: shared, and sharedDir/pubspec.yaml also declares name: shared (via _workspaceMemberCliCompletionDevDepPubspecContent, which also uses name: shared). Pub would reject duplicate names in a real workspace. Consider using a fixture whose name matches its directory, so the scenario reads naturally.
|
|
||
| dev_dependencies: | ||
| very_good_analysis: ^5.0.0 | ||
| '''; |
There was a problem hiding this comment.
_workspaceMemberWithDevDepsPubspecContent declares both dependencies: (http) and dev_dependencies: (very_good_analysis). The name suggests dev-only deps. Consider renaming to _workspaceMemberWithMixedDepsPubspecContent (or similar) so it's obvious why this fixture is used in the dev-dependency-filter test.
| dependencyTypes: ['direct-main'], | ||
| ), | ||
| returnsNormally, | ||
| ); |
There was a problem hiding this comment.
returnsNormally only proves termination; it doesn't verify what was collected. Since this test exists to prove the cycle-prevention logic (seen.add(...)), assert on the returned set too — e.g. expect it to contain http from the cycling member but not enter an infinite loop. Otherwise this passes vacuously even if the early-return logic at line 68 of pubspec.dart regressed and started looping forever in a future change.
5 participants
Summary
This PR adds workspace support to the
packages check licensescommand, enabling license checking in monorepo projects that use Dart's pub workspace feature.Reopens #1444 (branch was restored per request).
Closes #1273
Changes
New Files
lib/src/pubspec/pubspec.dart- Pubspec parser for detecting workspace configurationstest/src/pubspec/pubspec_test.dart- 19 tests for the Pubspec parserModified Files
lib/src/commands/packages/commands/check/commands/licenses.dart- Added workspace detection and dependency collectiontest/src/commands/packages/commands/check/commands/licenses_test.dart- Added 3 workspace support testsHow It Works
pubspec.yamlcontains aworkspaceproperty, it's detected as a workspace rootpubspec.yamlfilespackages/*are supported (Dart 3.11+)Test plan