feat: add workspace support for packages check licenses#1540
feat: add workspace support for packages check licenses#1540realmeylisdev wants to merge 16 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
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