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
Remove rewrapper prefix from compiler commands for clang-tidy #51001
Conversation
d8b3117
to
bb4468c
Compare
Hmm. Not ready yet. |
This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again. |
29901ee
to
0a5844b
Compare
0a5844b
to
261878d
Compare
Alrighty, this is passing the presubs now. |
"--rbe", | ||
"--no-goma" |
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.
Can these be inferrred from the gclient_variables
so we don't have to duplicate this information?
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.
Like are there environment variables being set?
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 can correct me if I'm not remembering right, but I believe that the LUCI recipes interpret and act on the contents of this parameter list. For example, the only way to keep a recipe from trying to start up goma is by including --no-goma
in this list.
The local developer workflow for RBE will make more sense, hopefully.
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.
Yea, if it's not possible, no need to hold back this PR. This is just a footgun that you have to specify this twice. What does it mean to say use_rbe
but then not pass in --rbe
for example? We could issue a follow up pr for the infra team.
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 can correct me if I'm not remembering right, but I believe that the LUCI recipes interpret and act on the contents of this parameter list. For example, the only way to keep a recipe from trying to start up goma is by including
--no-goma
in this list.The local developer workflow for RBE will make more sense, hopefully.
That's true. But one optimization we can do is have recipes to auto append --rbe
and --no-goma
if use_rbe
exists in the gclient_variables
. This should remove tons of duplication here from config.json files. flutter/flutter#144269 to optimize.
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.
Please no more implicit configuration in the recipes! The implicit configuration of Goma and LTO in the recipes is super-confusing, and one of my goals with RBE is explicitly not to repeat those mistakes. If we need tooling to streamline the local dev workflow, then we can put that logic in the engine repo and not in recipes.
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.
SGTM. Thanks for the context.
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 modulo the question for the infra team to see if we can deduplicate rbe specification.
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
64a375de9c [Windows] Make the engine own a map of views (flutter/engine#51017) 73480bf11d Rename some classes in the engine_build_configs package (flutter/engine#51016) 2756f14a46 Remove rewrapper prefix from compiler commands for clang-tidy (flutter/engine#51001) 0f727a5b31 Improve, test, and fix a bug related to `adb logcat` filtering. (flutter/engine#51012) ab4d6db3f1 Move vulkan-deps to //flutter/third_party/vulkan-deps (flutter/engine#51013)
This PR also enables RBE for builds for clang-tidy.