Remove rewrapper prefix from compiler commands for clang-tidy #51001
Conversation
zanderso
force-pushed
the
clean-clang-tidy-cmds
branch
from
d8b3117 to
bb4468c
Compare
zanderso
changed the title
|
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. |
zanderso
force-pushed
the
clean-clang-tidy-cmds
branch
2 times, most recently
from
29901ee to
0a5844b
Compare
zanderso
force-pushed
the
clean-clang-tidy-cmds
branch
from
0a5844b to
261878d
Compare
|
Alrighty, this is passing the presubs now. |
| "--rbe", | ||
| "--no-goma" |
There was a problem hiding this comment.
Can these be inferrred from the gclient_variables so we don't have to duplicate this information?
There was a problem hiding this comment.
Like are there environment variables being set?
There was a problem hiding this comment.
@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.
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.
@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-gomain 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.
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.
SGTM. Thanks for the context.
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
LGTM modulo the question for the infra team to see if we can deduplicate rbe specification.
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)
3 participants
This PR also enables RBE for builds for clang-tidy.