Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Remove rewrapper prefix from compiler commands for clang-tidy #51001

Merged
zanderso merged 1 commit into from
Feb 27, 2024
Merged

Remove rewrapper prefix from compiler commands for clang-tidy #51001

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 17 additions & 2 deletions ci/builders/linux_clang_tidy.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,18 @@
"device_type=none",
"os=Linux"
],
"gclient_variables": {
"use_rbe": true
},
"gn": [
"--android",
"--android-cpu",
"arm64",
"--no-lto",
"--target-dir",
"android_debug_arm64_clang_tidy"
"android_debug_arm64_clang_tidy",
"--rbe",
"--no-goma"
],
"name": "android_debug_arm64_clang_tidy",
"ninja": {
Expand All @@ -25,13 +30,18 @@
"device_type=none",
"os=Linux"
],
"gclient_variables": {
"use_rbe": true
},
"gn": [
"--runtime-mode",
"debug",
"--prebuilt-dart-sdk",
"--no-lto",
"--target-dir",
"host_debug_clang_tidy"
"host_debug_clang_tidy",
"--rbe",
"--no-goma"
Comment on lines +43 to +44
Copy link
Member

@gaaclarke gaaclarke Feb 27, 2024

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?

Copy link
Member

@gaaclarke gaaclarke Feb 27, 2024

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?

Copy link
Member Author

@zanderso zanderso Feb 27, 2024

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.

Copy link
Member

@gaaclarke gaaclarke Feb 27, 2024

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.

Copy link
Contributor

@keyonghan keyonghan Feb 27, 2024

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.

Copy link
Member Author

@zanderso zanderso Feb 27, 2024

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.

Copy link
Contributor

@keyonghan keyonghan Feb 27, 2024

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.

],
"name": "host_debug_clang_tidy",
"ninja": {
Expand All @@ -58,6 +68,7 @@
{
"name": "test: lint host_debug",
"parameters": [
"--verbose",
"--variant",
"host_debug_clang_tidy",
"--lint-all",
Expand Down Expand Up @@ -87,6 +98,7 @@
{
"name": "test: lint host_debug",
"parameters": [
"--verbose",
"--variant",
"host_debug_clang_tidy",
"--lint-all",
Expand Down Expand Up @@ -116,6 +128,7 @@
{
"name": "test: lint host_debug",
"parameters": [
"--verbose",
"--variant",
"host_debug_clang_tidy",
"--lint-all",
Expand Down Expand Up @@ -145,6 +158,7 @@
{
"name": "test: lint host_debug",
"parameters": [
"--verbose",
"--variant",
"host_debug_clang_tidy",
"--lint-all",
Expand Down Expand Up @@ -172,6 +186,7 @@
{
"name": "test: lint android_debug_arm64",
"parameters": [
"--verbose",
"--variant",
"android_debug_arm64_clang_tidy",
"--lint-all",
Expand Down
14 changes: 10 additions & 4 deletions ci/builders/mac_clang_tidy.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@
"cpu=arm64"
],
"gclient_variables": {
"download_android_deps": false
"download_android_deps": false,
"use_rbe": true
},
"gn": [
"--runtime-mode",
Expand All @@ -16,7 +17,9 @@
"--no-lto",
"--force-mac-arm64",
"--target-dir",
"host_debug_clang_tidy"
"host_debug_clang_tidy",
"--rbe",
"--no-goma"
],
"name": "host_debug_clang_tidy",
"ninja": {
Expand All @@ -30,7 +33,8 @@
"cpu=arm64"
],
"gclient_variables": {
"download_android_deps": false
"download_android_deps": false,
"use_rbe": true
},
"gn": [
"--ios",
Expand All @@ -40,7 +44,9 @@
"--no-lto",
"--force-mac-arm64",
"--target-dir",
"ios_debug_sim_clang_tidy"
"ios_debug_sim_clang_tidy",
"--rbe",
"--no-goma"
],
"name": "ios_debug_sim_clang_tidy",
"ninja": {
Expand Down
11 changes: 8 additions & 3 deletions tools/clang_tidy/lib/src/command.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,8 @@ class Command {
/// The file on which the command operates.
late final String filePath;

static final RegExp _pathRegex = RegExp(r'\S*clang/bin/clang');
static final RegExp _argRegex = RegExp(r'-MF \S*');
static final RegExp _pathRegex = RegExp(r'\S*clang/bin/clang(\+\+)?');
static final RegExp _argRegex = RegExp(r'-MF\s+\S+\s+');

// Filter out any extra commands that were appended to the compile command.
static final RegExp _extraCommandRegex = RegExp(r'&&.*$');
Expand All @@ -67,6 +67,11 @@ class Command {
String get tidyArgs {
return _tidyArgs ??= (() {
String result = command;
result = result.replaceAll(r'\s+', ' ');
// Remove everything that comes before the compiler command.
result = result.split(' ')
.skipWhile((String s) => !_pathRegex.hasMatch(s))
.join(' ');
result = result.replaceAll(_pathRegex, '');
result = result.replaceAll(_argRegex, '');
result = result.replaceAll(_extraCommandRegex, '');
Expand All @@ -81,7 +86,7 @@ class Command {
return _tidyPath ??= _pathRegex.stringMatch(command)?.replaceAll(
'clang/bin/clang',
'clang/bin/clang-tidy',
) ?? '';
).replaceAll('clang-tidy++', 'clang-tidy') ?? '';
}

/// Whether this command operates on any of the files in `queries`.
Expand Down
24 changes: 24 additions & 0 deletions tools/clang_tidy/test/clang_tidy_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -586,5 +586,29 @@ Future<int> main(List<String> args) async {
expect(command.tidyArgs.trim(), 'filename');
});

test('Command filters out the -MF flag', () {
final Command command = Command.fromMap(<String, String>{
'directory': '/unused',
'command':
'../../buildtools/mac-x64/clang/bin/clang -MF stuff filename ',
'file': 'unused',
});
expect(command.tidyArgs.trim(), 'filename');
});

test('Command filters out rewrapper command before a compile command', () {
final Command command = Command.fromMap(<String, String>{
'directory': '/unused',
'command':
'flutter/engine/src/buildtools/mac-arm64/reclient/rewrapper '
'--cfg=flutter/engine/src/flutter/build/rbe/rewrapper-mac-arm64.cfg '
'--exec_root=flutter/engine/src/ '
'--labels=type=compile,compiler=clang,lang=cpp '
'../../buildtools/mac-x64/clang/bin/clang++ filename ',
'file': 'unused',
});
expect(command.tidyArgs.trim(), 'filename');
});

return 0;
}