Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add a "pub workspace" to the root of the engine repository #53539
Add a "pub workspace" to the root of the engine repository #53539
Changes from 12 commits
54d1ca6
2068ba1
fcde54c
50fa853
144ec4a
d78b88a
9f49e49
ff2aa36
7812873
a467fb6
901b47f
4f99625
d1ba509
b4c4186
9485af8
2638753
627d84c
46853e1
339abd3
3ed2f04
d889220
2ba6436
eb2303a
ef6605d
2d727a6
8ee0c85
d2aa032
d1a92b4
d4fa1df
7703083
e0c31a8
784b9d8
77f04a8
da0d118
9ee12d0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
These comments are phenomenal, excellent work!
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.
You could enable: https://dart.dev/tools/linter-rules/sort_pub_dependencies
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.
Added a TODO for now, thanks!
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.
I see pyyaml in the DEPS. Is that not usable?
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.
Sounds good, I used the same pattern as
gen_dart_package_config.py
and went with: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.
We should only be fetching dependencies from the network from the DEPS file.
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.
Right, pubspec files with
resolution: workspace
do not fetch dependencies at all.This script will throw saying "no package_config.json", hence this change.
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.
Ah, sorry. My comment was in reference to the discussion of pyyaml and using the wheel.
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.
For my understanding, is it possible for a pubspec that uses the workspace to add a dependency on a package that isn't in the workspace? Like can a workspace-using pubspec add a new dependency and pull a package from pub that isn't declared in the workspace?
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.
Hmm... I'm actually not sure. I assume no, but let me give it a shot.
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.
Short answer: Yes, it's possible. It ends up adding it to the root
.dart_tool/package_config.json
.Long answer: Our script to protect against pub dependencies still works, as it catches the root package config:
... and then:
I do think the UX of allowing package-specific dependencies and silently adding them to the root pubspec is confusing, but not a blocker for this effort. I'll file an issue on dart-lang/pub and see what the team thinks in terms of UX.
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.
Done in dart-lang/pub#4328.
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.
Thanks for checking!