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
Reverts "Implement dartPluginClass support for plugins #74469" #78623
Conversation
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.
LGTM
Some conflicts here. 😞 |
95c2115
to
1627ce5
Compare
@jonahwilliams Do you know how (or whether) to remove the "Mac dart_plugin_registry_test" presubmit check for this? |
For the try builders, you need to edit the JSON config here to say https://github.com/flutter/flutter/blob/master/dev/try_builders.json#L535 and maybe for prod builders just mark it as flaky? https://github.com/flutter/flutter/blob/master/dev/prod_builders.json#L904 How do we disable a prod builder? @godofredoc / @keyonghan |
@@ -532,13 +532,6 @@ | |||
"enabled": true, | |||
"run_if": ["dev/**", "packages/flutter_tools/**", "bin/**"] | |||
}, | |||
{ | |||
"name": "Mac dart_plugin_registry_test", |
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.
Do you know how (or whether) to remove the "Mac dart_plugin_registry_test" presubmit check for this?
I would have thought this would have done it...
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.
oh, I wonder if uses the JSON from HEAD?
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.
@keyonghan why would it show up in this PR as a pre-submit check if it was removed from try_builders?
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.
It should be reflected in the PR immediately.
There is some recent change about how builders are retrieved. /cc @CaseyHillers any idea?
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 just edited the try_builders.json
in #78888 and the two tests I added showed up...
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.
Was the try_builders.json change here in the first commit change of this PR? I am guessing if it was triggered first, then it will remain in this check run set. Though later changes (remove that test) only trigger other tests.
Marking it as flaky will hide it from tree status, but it will still be scheduled to run. |
We need to remove it for 2-3 weeks or so |
Then marking as flaky sounds a better way to go. |
"name": "Mac dart_plugin_registry_test", | ||
"repo": "flutter", | ||
"task_name": "dart_plugin_registry_test", | ||
"enabled": true, |
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.
Another option: you can set "enabled": false
if you want to reenable it back later.
I restored the entries to the json files, and marked as flaky in prod and disabled in try. |
/cc @blasten |
…r#74469" (flutter#78623)" This reverts commit 5efc716.
For #78554