-
Notifications
You must be signed in to change notification settings - Fork 6k
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
Conversation
pubspec.yaml
Outdated
# (3) MIGRATING A NON-WORKSPACE PACKAGE TO USING THE WORKSPACE | ||
# ------------------------------------------------------------------------------ | ||
# Many packages in this repo are still using a pre-workspace style pubspec.yaml, | ||
# either with manuallu declared `dependency_overrides` (much of ./tools) or by |
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.
s/manuallu/manually
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!
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
# `DEPS` file: they get a "any" version constraint and are overridden in the | ||
# `dependency_overrides` section below. | ||
# | ||
# While not enforced by tooling, try to keep this list in alphabetical order. |
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!
The issue this was blocked on has been resolved and rolled in. Does this just need a rebase? |
I think we're still looking at some options for overrides. See dart-lang/pub/issues/4318 |
pubspec.yaml
Outdated
path: ./tools/pkg/engine_repo_tools | ||
expect: any | ||
file: any | ||
litetest: |
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.
move litetest
and process_fakes
to dependency_overrides
and the sub-packages don't need to include redundant paths.
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.
Great suggestion, done!
I am still blocked on a new enough Dart SDK that supports Revving here: https://dart-review.googlesource.com/c/sdk/+/373903 |
49b2ff0
to
9f49e49
Compare
Without this, I can't use `workspace` in the Flutter engine: flutter/engine#53539 Change-Id: I3b68f14b94f91deda329efd98042dcf63ddb80ba Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/373903 Commit-Queue: Ryan Macnak <[email protected]> Reviewed-by: Ryan Macnak <[email protected]>
69eace8
to
ff2aa36
Compare
pubspec.yaml
Outdated
# ------------------------------------------------------------------------------ | ||
# (1) ADDING A NEW DEPENDENCY | ||
# ------------------------------------------------------------------------------ | ||
# Dependencies need to be added either via the DEPS file, or by checking the are |
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.
by checking the are available
seems like a typo? Should this not be by checking they are available
, like in the last phrase of this paragraph?
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.
The linked pull request was merged. Are you still blocked on this? |
Nope, looks like I can rebase. |
auto label is removed for flutter/engine/53539, due to - The status or check suite Mac mac_host_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.
|
auto label is removed for flutter/engine/53539, due to - The status or check suite Linux linux_unopt has failed. Please fix the issues identified (or deflake) before re-applying this label. |
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.
still lgtm
auto label is removed for flutter/engine/53539, due to - The status or check suite Linux linux_unopt has failed. Please fix the issues identified (or deflake) before re-applying this label. |
auto label is removed for flutter/engine/53539, due to - The status or check suite Linux linux_unopt has failed. Please fix the issues identified (or deflake) before re-applying this label. |
…151977) flutter/engine@d58ba74...8bcf638 2024-07-18 [email protected] Add a "pub workspace" to the root of the engine repository (flutter/engine#53539) 2024-07-18 [email protected] Roll Skia from 9d391088f52c to 939e1dac9815 (1 revision) (flutter/engine#53990) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…3539) ... and use it in `engine_tool` to prove everything is working, i.e. on CI and elsewhere. Partial work towards flutter/flutter#147883. Supersedes and closes flutter#51782 (which was a prototype). See also: - https://flutter.dev/go/pub-workspace, the design doc on the feature. - dart-lang/pub-dev#7762, an example PR. I'll probably end up moving the inline docs in `pubspec.yaml` to a `docs/*.md` once that's a thing.
…lutter#53539)" This reverts commit 8c1dd2f.
…lutter#151977) flutter/engine@d58ba74...8bcf638 2024-07-18 [email protected] Add a "pub workspace" to the root of the engine repository (flutter/engine#53539) 2024-07-18 [email protected] Roll Skia from 9d391088f52c to 939e1dac9815 (1 revision) (flutter/engine#53990) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…lutter#151977) flutter/engine@d58ba74...8bcf638 2024-07-18 [email protected] Add a "pub workspace" to the root of the engine repository (flutter/engine#53539) 2024-07-18 [email protected] Roll Skia from 9d391088f52c to 939e1dac9815 (1 revision) (flutter/engine#53990) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
... and use it in
engine_tool
to prove everything is working, i.e. on CI and elsewhere.Partial work towards flutter/flutter#147883.
Supersedes and closes #51782 (which was a prototype).
See also:
I'll probably end up moving the inline docs in
pubspec.yaml
to adocs/*.md
once that's a thing.