[et] Properly disable RBE with a flag or when not available

Author: zanderso

tools/engine_tool/lib/src/build_utils.dart

@@ -2,7 +2,10 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+import 'dart:io' as io show Directory;
+
 import 'package:engine_build_configs/engine_build_configs.dart';
+import 'package:path/path.dart' as p;
 
 import 'environment.dart';
 import 'logger.dart';
@@ -63,13 +66,30 @@ void debugCheckBuilds(List<Build> builds) {
 }
 
 /// Build the build target in the environment.
-Future<int> runBuild(Environment environment, Build build) async {
+Future<int> runBuild(
+  Environment environment,
+  Build build, {
+  List<String> extraGnArgs = const <String>[],
+}) async {
+  // If RBE config files aren't in the tree, then disable RBE.
+  final String rbeConfigPath = p.join(
+    environment.engine.srcDir.path,
+    'flutter',
+    'build',
+    'rbe',
+  );
+  final List<String> gnArgs = <String>[
+    ...extraGnArgs,
+    if (!io.Directory(rbeConfigPath).existsSync()) '--no-rbe',
+  ];
+
   final BuildRunner buildRunner = BuildRunner(
     platform: environment.platform,
     processRunner: environment.processRunner,
     abi: environment.abi,
     engineSrcDir: environment.engine.srcDir,
     build: build,
+    extraGnArgs: gnArgs,
     runTests: false,
   );
 

tools/engine_tool/lib/src/commands/build_command.dart

@@ -31,6 +31,12 @@ final class BuildCommand extends CommandBase {
           config.name: config.gn.join(' '),
       },
     );
+    argParser.addFlag(
+      rbeFlag,
+      defaultsTo: true,
+      help: 'RBE is enabled by default when available. Use --no-rbe to '
+            'disable it.',
+    );
   }
 
   /// List of compatible builds.
@@ -45,14 +51,19 @@ final class BuildCommand extends CommandBase {
   @override
   Future<int> run() async {
     final String configName = argResults![configFlag] as String;
+    final bool useRbe = argResults![rbeFlag] as bool;
     final Build? build =
         builds.where((Build build) => build.name == configName).firstOrNull;
     if (build == null) {
       environment.logger.error('Could not find config $configName');
       return 1;
     }
 
+    final List<String> extraGnArgs = <String>[
+      if (!useRbe) '--no-rbe',
+    ];
+
     // TODO(loic-sharma): Fetch dependencies if needed.
-    return runBuild(environment, build);
+    return runBuild(environment, build, extraGnArgs: extraGnArgs);
   }
 }

tools/engine_tool/lib/src/commands/flags.dart

@@ -16,5 +16,6 @@ const String builderFlag = 'builder';
 const String configFlag = 'config';
 const String dryRunFlag = 'dry-run';
 const String quietFlag = 'quiet';
+const String rbeFlag = 'rbe';
 const String runTestsFlag = 'run-tests';
 const String verboseFlag = 'verbose';

tools/engine_tool/lib/src/commands/run_command.dart

@@ -37,6 +37,12 @@ final class RunCommand extends CommandBase {
           build.name: build.gn.join(' '),
       },
     );
+    argParser.addFlag(
+      rbeFlag,
+      defaultsTo: true,
+      help: 'RBE is enabled by default when available. Use --no-rbe to '
+            'disable it.',
+    );
   }
 
   /// List of compatible builds.
@@ -142,15 +148,20 @@ final class RunCommand extends CommandBase {
       return 1;
     }
 
+    final bool useRbe = argResults![rbeFlag] as bool;
+    final List<String> extraGnArgs = <String>[
+      if (!useRbe) '--no-rbe',
+    ];
+
     // First build the host.
-    int r = await runBuild(environment, hostBuild);
+    int r = await runBuild(environment, hostBuild, extraGnArgs: extraGnArgs);
     if (r != 0) {
       return r;
     }
 
     // Now build the target if it isn't the same.
     if (hostBuild.name != build.name) {
-      r = await runBuild(environment, build);
+      r = await runBuild(environment, build, extraGnArgs: extraGnArgs);
       if (r != 0) {
         return r;
       }

tools/engine_tool/test/build_command_test.dart

@@ -13,6 +13,7 @@ import 'package:engine_tool/src/commands/command_runner.dart';
 import 'package:engine_tool/src/environment.dart';
 import 'package:engine_tool/src/logger.dart';
 import 'package:litetest/litetest.dart';
+import 'package:path/path.dart' as path;
 import 'package:platform/platform.dart';
 import 'package:process_fakes/process_fakes.dart';
 import 'package:process_runner/process_runner.dart';
@@ -54,23 +55,28 @@ void main() {
     'win_test_config': winTestConfig,
   };
 
-  (Environment, List<List<String>>) linuxEnv(Logger logger) {
+  (Environment, List<List<String>>) linuxEnv(Logger logger, {Engine? eng}) {
+    eng ??= engine;
     final List<List<String>> runHistory = <List<String>>[];
     return (
       Environment(
         abi: ffi.Abi.linuxX64,
-        engine: engine,
+        engine: eng,
         platform: FakePlatform(
             operatingSystem: Platform.linux,
             resolvedExecutable: io.Platform.resolvedExecutable),
         processRunner: ProcessRunner(
-          processManager: FakeProcessManager(onStart: (List<String> command) {
-            runHistory.add(command);
-            return FakeProcess();
-          }, onRun: (List<String> command) {
-            runHistory.add(command);
-            return io.ProcessResult(81, 0, '', '');
-          }),
+          processManager: FakeProcessManager(
+            canRun: (Object? exe, {String? workingDirectory}) => true,
+            onStart: (List<String> command) {
+              runHistory.add(command);
+              return FakeProcess();
+            },
+            onRun: (List<String> command) {
+              runHistory.add(command);
+              return io.ProcessResult(81, 0, '', '');
+            },
+          ),
         ),
         logger: logger,
       ),
@@ -82,7 +88,7 @@ void main() {
     final Logger logger = Logger.test();
     final (Environment env, _) = linuxEnv(logger);
     final List<Build> result = runnableBuilds(env, configs);
-    expect(result.length, equals(6));
+    expect(result.length, equals(8));
     expect(result[0].name, equals('build_name'));
   });
 
@@ -157,4 +163,69 @@ void main() {
     expect(result, equals(0));
     expect(runHistory.length, lessThanOrEqualTo(3));
   });
+
+  test('build command runs rbe on an rbe build', () async {
+    final Logger logger = Logger.test();
+    final (Environment env, List<List<String>> runHistory) = linuxEnv(logger);
+    final ToolCommandRunner runner = ToolCommandRunner(
+      environment: env,
+      configs: configs,
+    );
+    final int result = await runner.run(<String>[
+      'build',
+      '--config',
+      'android_debug_rbe_arm64',
+    ]);
+    expect(result, equals(0));
+    expect(runHistory[0][0], contains(path.join('tools', 'gn')));
+    expect(runHistory[0][4], equals('--rbe'));
+    expect(runHistory[1][0], contains(path.join('reclient', 'bootstrap')));
+  });
+
+  test('build command does not run rbe when disabled', () async {
+    final Logger logger = Logger.test();
+    final (Environment env, List<List<String>> runHistory) = linuxEnv(logger);
+    final ToolCommandRunner runner = ToolCommandRunner(
+      environment: env,
+      configs: configs,
+    );
+    final int result = await runner.run(<String>[
+      'build',
+      '--config',
+      'android_debug_rbe_arm64',
+      '--no-rbe',
+    ]);
+    expect(result, equals(0));
+    expect(runHistory[0][0], contains(path.join('tools', 'gn')));
+    expect(runHistory[0], doesNotContainAny(<String>['--rbe']));
+    expect(runHistory[1][0], contains(path.join('ninja', 'ninja')));
+  });
+
+  test('build command does not run rbe when rbe configs do not exist', () async {
+    final Logger logger = Logger.test();
+    final io.Directory rootDir = io.Directory.systemTemp.createTempSync('et');
+    try {
+      final Engine eng = TestEngine.createTemp(rootDir: rootDir);
+      final (Environment env, List<List<String>> runHistory) = linuxEnv(logger, eng: eng);
+      final ToolCommandRunner runner = ToolCommandRunner(
+        environment: env,
+        configs: configs,
+      );
+      final int result = await runner.run(<String>[
+        'build',
+        '--config',
+        'android_debug_rbe_arm64',
+      ]);
+      expect(result, equals(0));
+      expect(runHistory[0][0], contains(path.join('tools', 'gn')));
+      expect(runHistory[0], doesNotContainAny(<String>['--rbe']));
+      expect(runHistory[1][0], contains(path.join('ninja', 'ninja')));
+    } finally {
+      try {
+        rootDir.deleteSync(recursive: true);
+      } catch (_) {
+        // Ignore temp directory deletion failure.
+      }
+    }
+  });
 }

tools/engine_tool/test/fixtures.dart

@@ -70,6 +70,17 @@ String testConfig(String os) => '''
         "config": "android_debug_arm64",
         "targets": ["ninja_target"]
       }
+    },
+    {
+      "drone_dimensions": [
+        "os=$os"
+      ],
+      "gn": ["--gn-arg", "--lto", "--no-goma", "--rbe"],
+      "name": "android_debug_rbe_arm64",
+      "ninja": {
+        "config": "android_debug_rbe_arm64",
+        "targets": ["ninja_target"]
+      }
     }
   ],
   "generators": {