From faded065bf85bf5754f3aef5a9571c18e4d364cc Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Thu, 9 May 2024 10:41:33 -0700 Subject: [PATCH 1/3] Set --rbe default based on the presence of flutter/build/rbe. --- tools/engine_tool/lib/src/build_utils.dart | 20 ++++--------------- .../lib/src/commands/build_command.dart | 7 ++++--- .../lib/src/commands/query_command.dart | 7 +++++++ .../lib/src/commands/run_command.dart | 20 ++++++++++++++----- .../lib/src/commands/test_command.dart | 15 ++++++++++++-- tools/engine_tool/lib/src/environment.dart | 19 ++++++++++++++++++ tools/engine_tool/lib/src/gn_utils.dart | 3 ++- tools/engine_tool/test/gn_utils_test.dart | 11 ++++++++-- 8 files changed, 73 insertions(+), 29 deletions(-) diff --git a/tools/engine_tool/lib/src/build_utils.dart b/tools/engine_tool/lib/src/build_utils.dart index df3ad2b1c740a..7948eb87b1038 100644 --- a/tools/engine_tool/lib/src/build_utils.dart +++ b/tools/engine_tool/lib/src/build_utils.dart @@ -122,18 +122,12 @@ String demangleConfigName(Environment env, String name) { Future runBuild( Environment environment, Build build, { + required bool enableRbe, List extraGnArgs = const [], List targets = const [], }) 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 gnArgs = [ - if (!io.Directory(rbeConfigPath).existsSync()) '--no-rbe', + if (!enableRbe) '--no-rbe', ...extraGnArgs, ]; @@ -192,16 +186,10 @@ Future runGn( Environment environment, Build build, { List extraGnArgs = const [], + required bool enableRbe, }) 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 gnArgs = [ - if (!io.Directory(rbeConfigPath).existsSync()) '--no-rbe', + if (!enableRbe) '--no-rbe', ...extraGnArgs, ]; diff --git a/tools/engine_tool/lib/src/commands/build_command.dart b/tools/engine_tool/lib/src/commands/build_command.dart index bf716f15cae9f..9fdcf5dd70464 100644 --- a/tools/engine_tool/lib/src/commands/build_command.dart +++ b/tools/engine_tool/lib/src/commands/build_command.dart @@ -30,9 +30,8 @@ final class BuildCommand extends CommandBase { ); argParser.addFlag( rbeFlag, - defaultsTo: true, - help: 'RBE is enabled by default when available. Use --no-rbe to ' - 'disable it.', + defaultsTo: environment.hasRbeConfigInTree(), + help: 'RBE is enabled by default when available.', ); argParser.addFlag( ltoFlag, @@ -76,6 +75,7 @@ et build //flutter/fml:fml_benchmarks # Build a specific target in `//flutter/f environment, build, argResults!.rest, + enableRbe: useRbe, ); if (selectedTargets == null) { // The user typed something wrong and targetsFromCommandLine has already @@ -93,6 +93,7 @@ et build //flutter/fml:fml_benchmarks # Build a specific target in `//flutter/f build, extraGnArgs: extraGnArgs, targets: ninjaTargets, + enableRbe: useRbe, ); } } diff --git a/tools/engine_tool/lib/src/commands/query_command.dart b/tools/engine_tool/lib/src/commands/query_command.dart index e07d33cfbbbaf..0e699ad07e22c 100644 --- a/tools/engine_tool/lib/src/commands/query_command.dart +++ b/tools/engine_tool/lib/src/commands/query_command.dart @@ -154,6 +154,11 @@ final class QueryTargetsCommand extends CommandBase { help: 'Filter build targets to only include tests', negatable: false, ); + argParser.addFlag( + rbeFlag, + defaultsTo: environment.hasRbeConfigInTree(), + help: 'RBE is enabled by default when available.', + ); } /// Build configurations loaded from the engine from under ci/builders. @@ -176,6 +181,7 @@ et query targets //flutter/fml/... # List all targets under `//flutter/fml` Future run() async { final String configName = argResults![configFlag] as String; final bool testOnly = argResults![testOnlyFlag] as bool; + final bool useRbe = argResults![rbeFlag] as bool; final String demangledName = demangleConfigName(environment, configName); final Build? build = builds.where((Build build) => build.name == demangledName).firstOrNull; @@ -189,6 +195,7 @@ et query targets //flutter/fml/... # List all targets under `//flutter/fml` build, argResults!.rest, defaultToAll: true, + enableRbe: useRbe, ); if (selectedTargets == null) { // The user typed something wrong and targetsFromCommandLine has already diff --git a/tools/engine_tool/lib/src/commands/run_command.dart b/tools/engine_tool/lib/src/commands/run_command.dart index 8798a4e5d3c6c..d1e09e9dfe0b4 100644 --- a/tools/engine_tool/lib/src/commands/run_command.dart +++ b/tools/engine_tool/lib/src/commands/run_command.dart @@ -3,6 +3,7 @@ // found in the LICENSE file. import 'dart:io' show ProcessStartMode; +import 'dart:math'; import 'package:engine_build_configs/engine_build_configs.dart'; import 'package:process_runner/process_runner.dart'; @@ -36,9 +37,8 @@ final class RunCommand extends CommandBase { ); argParser.addFlag( rbeFlag, - defaultsTo: true, - help: 'RBE is enabled by default when available. Use --no-rbe to ' - 'disable it.', + defaultsTo: environment.hasRbeConfigInTree(), + help: 'RBE is enabled by default when available.', ); } @@ -157,14 +157,24 @@ See `flutter run --help` for a listing ]; // First build the host. - int r = await runBuild(environment, hostBuild, extraGnArgs: extraGnArgs); + int r = await runBuild( + environment, + hostBuild, + extraGnArgs: extraGnArgs, + enableRbe: useRbe, + ); 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, extraGnArgs: extraGnArgs); + r = await runBuild( + environment, + build, + extraGnArgs: extraGnArgs, + enableRbe: useRbe, + ); if (r != 0) { return r; } diff --git a/tools/engine_tool/lib/src/commands/test_command.dart b/tools/engine_tool/lib/src/commands/test_command.dart index 47de807fa6a59..7e9e868da073d 100644 --- a/tools/engine_tool/lib/src/commands/test_command.dart +++ b/tools/engine_tool/lib/src/commands/test_command.dart @@ -30,6 +30,11 @@ final class TestCommand extends CommandBase { argParser, builds, ); + argParser.addFlag( + rbeFlag, + defaultsTo: environment.hasRbeConfigInTree(), + help: 'RBE is enabled by default when available.', + ); } /// List of compatible builds. @@ -48,6 +53,7 @@ et test //flutter/fml:fml_benchmarks # Run a single test target in `//flutter/f @override Future run() async { final String configName = argResults![configFlag] as String; + final bool useRbe = argResults![rbeFlag] as bool; final String demangledName = demangleConfigName(environment, configName); final Build? build = builds.where((Build build) => build.name == demangledName).firstOrNull; @@ -61,6 +67,7 @@ et test //flutter/fml:fml_benchmarks # Run a single test target in `//flutter/f build, argResults!.rest, defaultToAll: true, + enableRbe: useRbe, ); if (selectedTargets == null) { // The user typed something wrong and targetsFromCommandLine has already @@ -90,8 +97,12 @@ et test //flutter/fml:fml_benchmarks # Run a single test target in `//flutter/f .toList(); // TODO(johnmccutchan): runBuild manipulates buildTargets and adds some // targets listed in Build. Fix this. - final int buildExitCode = - await runBuild(environment, build, targets: buildTargets); + final int buildExitCode = await runBuild( + environment, + build, + targets: buildTargets, + enableRbe: useRbe, + ); if (buildExitCode != 0) { return buildExitCode; } diff --git a/tools/engine_tool/lib/src/environment.dart b/tools/engine_tool/lib/src/environment.dart index 6ae232d83e126..ea1a5acc62322 100644 --- a/tools/engine_tool/lib/src/environment.dart +++ b/tools/engine_tool/lib/src/environment.dart @@ -3,8 +3,10 @@ // found in the LICENSE file. import 'dart:ffi' as ffi show Abi; +import 'dart:io' as io show Directory; import 'package:engine_repo_tools/engine_repo_tools.dart'; +import 'package:path/path.dart' as p; import 'package:platform/platform.dart'; import 'package:process_runner/process_runner.dart'; @@ -44,4 +46,21 @@ final class Environment { /// Facility for commands to run subprocesses. final ProcessRunner processRunner; + + /// Whether it appears that the current environment supports remote builds. + /// + /// This is a heuristic based on the presence of certain directories in the + /// engine repo; it is not a guarantee that remote builds will work (due to + /// authentication, network, or other issues). + /// + /// **Note**: This calls does synchronous I/O. + bool hasRbeConfigInTree() { + final String rbeConfigPath = p.join( + engine.srcDir.path, + 'flutter', + 'build', + 'rbe', + ); + return io.Directory(rbeConfigPath).existsSync(); + } } diff --git a/tools/engine_tool/lib/src/gn_utils.dart b/tools/engine_tool/lib/src/gn_utils.dart index 8d67843ff5380..65da125ca4069 100644 --- a/tools/engine_tool/lib/src/gn_utils.dart +++ b/tools/engine_tool/lib/src/gn_utils.dart @@ -222,6 +222,7 @@ Future?> targetsFromCommandLine( Build build, List commandLineTargets, { bool defaultToAll = false, + required bool enableRbe, }) async { // If there are no targets specified on the command line, then delegate to // the default targets specified in the Build object unless directed @@ -240,7 +241,7 @@ Future?> targetsFromCommandLine( environment.logger.status( 'Build output directory at ${buildDir.path} not found. Running GN.', ); - final int gnResult = await runGn(environment, build); + final int gnResult = await runGn(environment, build, enableRbe: enableRbe); if (gnResult != 0 || !buildDir.existsSync()) { environment.logger.error( 'The specified build did not produce the expected build ' diff --git a/tools/engine_tool/test/gn_utils_test.dart b/tools/engine_tool/test/gn_utils_test.dart index a24239abb225e..f398e16abdd0f 100644 --- a/tools/engine_tool/test/gn_utils_test.dart +++ b/tools/engine_tool/test/gn_utils_test.dart @@ -91,7 +91,10 @@ void main() { (Build build) => build.name == 'linux/host_debug', ).firstOrNull; final List? selectedTargets = await targetsFromCommandLine( - env, build!, [], + env, + build!, + [], + enableRbe: false, ); expect(selectedTargets, isNotNull); expect(selectedTargets, isEmpty); @@ -111,7 +114,11 @@ void main() { (Build build) => build.name == 'linux/host_debug', ).firstOrNull; final List? selectedTargets = await targetsFromCommandLine( - env, build!, [], defaultToAll: true, + env, + build!, + [], + defaultToAll: true, + enableRbe: false, ); expect(selectedTargets, isNotNull); expect(selectedTargets, isNotEmpty); From a6e1676b2deac357b639b62ae09ed3b3ba05aab1 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Thu, 9 May 2024 10:44:21 -0700 Subject: [PATCH 2/3] ++ --- tools/engine_tool/lib/src/commands/run_command.dart | 12 ++++++------ tools/engine_tool/lib/src/commands/test_command.dart | 4 ++-- tools/engine_tool/lib/src/environment.dart | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/tools/engine_tool/lib/src/commands/run_command.dart b/tools/engine_tool/lib/src/commands/run_command.dart index d1e09e9dfe0b4..db2aafcbb068a 100644 --- a/tools/engine_tool/lib/src/commands/run_command.dart +++ b/tools/engine_tool/lib/src/commands/run_command.dart @@ -158,9 +158,9 @@ See `flutter run --help` for a listing // First build the host. int r = await runBuild( - environment, - hostBuild, - extraGnArgs: extraGnArgs, + environment, + hostBuild, + extraGnArgs: extraGnArgs, enableRbe: useRbe, ); if (r != 0) { @@ -170,9 +170,9 @@ See `flutter run --help` for a listing // Now build the target if it isn't the same. if (hostBuild.name != build.name) { r = await runBuild( - environment, - build, - extraGnArgs: extraGnArgs, + environment, + build, + extraGnArgs: extraGnArgs, enableRbe: useRbe, ); if (r != 0) { diff --git a/tools/engine_tool/lib/src/commands/test_command.dart b/tools/engine_tool/lib/src/commands/test_command.dart index 7e9e868da073d..00deac4d737ed 100644 --- a/tools/engine_tool/lib/src/commands/test_command.dart +++ b/tools/engine_tool/lib/src/commands/test_command.dart @@ -98,8 +98,8 @@ et test //flutter/fml:fml_benchmarks # Run a single test target in `//flutter/f // TODO(johnmccutchan): runBuild manipulates buildTargets and adds some // targets listed in Build. Fix this. final int buildExitCode = await runBuild( - environment, - build, + environment, + build, targets: buildTargets, enableRbe: useRbe, ); diff --git a/tools/engine_tool/lib/src/environment.dart b/tools/engine_tool/lib/src/environment.dart index ea1a5acc62322..94bedd1e14f5f 100644 --- a/tools/engine_tool/lib/src/environment.dart +++ b/tools/engine_tool/lib/src/environment.dart @@ -52,7 +52,7 @@ final class Environment { /// This is a heuristic based on the presence of certain directories in the /// engine repo; it is not a guarantee that remote builds will work (due to /// authentication, network, or other issues). - /// + /// /// **Note**: This calls does synchronous I/O. bool hasRbeConfigInTree() { final String rbeConfigPath = p.join( From 782e0a64113e686e48c5a7a17f821a82cc6a53c8 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Fri, 10 May 2024 11:06:03 -0700 Subject: [PATCH 3/3] ++ --- tools/engine_tool/lib/src/build_utils.dart | 3 -- .../lib/src/commands/build_command.dart | 4 +++ .../lib/src/commands/query_command.dart | 4 +++ .../lib/src/commands/run_command.dart | 5 +++- .../lib/src/commands/test_command.dart | 4 +++ .../engine_tool/test/build_command_test.dart | 28 +++++++++++++++++++ 6 files changed, 44 insertions(+), 4 deletions(-) diff --git a/tools/engine_tool/lib/src/build_utils.dart b/tools/engine_tool/lib/src/build_utils.dart index 7948eb87b1038..5018b01a16349 100644 --- a/tools/engine_tool/lib/src/build_utils.dart +++ b/tools/engine_tool/lib/src/build_utils.dart @@ -2,10 +2,7 @@ // 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'; diff --git a/tools/engine_tool/lib/src/commands/build_command.dart b/tools/engine_tool/lib/src/commands/build_command.dart index 9fdcf5dd70464..e584fb00b7e73 100644 --- a/tools/engine_tool/lib/src/commands/build_command.dart +++ b/tools/engine_tool/lib/src/commands/build_command.dart @@ -56,6 +56,10 @@ et build //flutter/fml:fml_benchmarks # Build a specific target in `//flutter/f Future run() async { final String configName = argResults![configFlag] as String; final bool useRbe = argResults![rbeFlag] as bool; + if (useRbe && !environment.hasRbeConfigInTree()) { + environment.logger.error('RBE was requested but no RBE config was found'); + return 1; + } final bool useLto = argResults![ltoFlag] as bool; final String demangledName = demangleConfigName(environment, configName); final Build? build = diff --git a/tools/engine_tool/lib/src/commands/query_command.dart b/tools/engine_tool/lib/src/commands/query_command.dart index 0e699ad07e22c..f8951de333300 100644 --- a/tools/engine_tool/lib/src/commands/query_command.dart +++ b/tools/engine_tool/lib/src/commands/query_command.dart @@ -182,6 +182,10 @@ et query targets //flutter/fml/... # List all targets under `//flutter/fml` final String configName = argResults![configFlag] as String; final bool testOnly = argResults![testOnlyFlag] as bool; final bool useRbe = argResults![rbeFlag] as bool; + if (useRbe && !environment.hasRbeConfigInTree()) { + environment.logger.error('RBE was requested but no RBE config was found'); + return 1; + } final String demangledName = demangleConfigName(environment, configName); final Build? build = builds.where((Build build) => build.name == demangledName).firstOrNull; diff --git a/tools/engine_tool/lib/src/commands/run_command.dart b/tools/engine_tool/lib/src/commands/run_command.dart index db2aafcbb068a..0b59ff99c4f6a 100644 --- a/tools/engine_tool/lib/src/commands/run_command.dart +++ b/tools/engine_tool/lib/src/commands/run_command.dart @@ -3,7 +3,6 @@ // found in the LICENSE file. import 'dart:io' show ProcessStartMode; -import 'dart:math'; import 'package:engine_build_configs/engine_build_configs.dart'; import 'package:process_runner/process_runner.dart'; @@ -152,6 +151,10 @@ See `flutter run --help` for a listing } final bool useRbe = argResults![rbeFlag] as bool; + if (useRbe && !environment.hasRbeConfigInTree()) { + environment.logger.error('RBE was requested but no RBE config was found'); + return 1; + } final List extraGnArgs = [ if (!useRbe) '--no-rbe', ]; diff --git a/tools/engine_tool/lib/src/commands/test_command.dart b/tools/engine_tool/lib/src/commands/test_command.dart index 00deac4d737ed..2257bde934222 100644 --- a/tools/engine_tool/lib/src/commands/test_command.dart +++ b/tools/engine_tool/lib/src/commands/test_command.dart @@ -54,6 +54,10 @@ et test //flutter/fml:fml_benchmarks # Run a single test target in `//flutter/f Future run() async { final String configName = argResults![configFlag] as String; final bool useRbe = argResults![rbeFlag] as bool; + if (useRbe && !environment.hasRbeConfigInTree()) { + environment.logger.error('RBE was requested but no RBE config was found'); + return 1; + } final String demangledName = demangleConfigName(environment, configName); final Build? build = builds.where((Build build) => build.name == demangledName).firstOrNull; diff --git a/tools/engine_tool/test/build_command_test.dart b/tools/engine_tool/test/build_command_test.dart index 12c5aa1f82319..31da78db60698 100644 --- a/tools/engine_tool/test/build_command_test.dart +++ b/tools/engine_tool/test/build_command_test.dart @@ -9,6 +9,7 @@ import 'package:engine_build_configs/engine_build_configs.dart'; import 'package:engine_tool/src/build_utils.dart'; 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'; @@ -176,6 +177,33 @@ void main() { } }); + test('build command fails when rbe is enabled but not supported', () async { + final TestEnvironment testEnv = TestEnvironment.withTestEngine( + cannedProcesses: cannedProcesses, + // Intentionally omit withRbe: true. + // That means the //flutter/build/rbe directory will not be created. + ); + try { + final ToolCommandRunner runner = ToolCommandRunner( + environment: testEnv.environment, + configs: configs, + ); + final int result = await runner.run([ + 'build', + '--config', + 'ci/android_debug_rbe_arm64', + '--rbe', + ]); + expect(result, equals(1)); + expect( + testEnv.testLogs.map((LogRecord r) => r.message).join(), + contains('RBE was requested but no RBE config was found'), + ); + } finally { + testEnv.cleanup(); + } + }); + test('build command does not run rbe when disabled', () async { final TestEnvironment testEnv = TestEnvironment.withTestEngine( withRbe: true,