From a7997f606e32bc7e54778df50e003f975b85a536 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Thu, 10 Aug 2023 17:25:30 -0700 Subject: [PATCH] Update `dev/devicelab/**` to provide `--local-engine-host`. (#132342) Partial work towards https://github.com/flutter/flutter/issues/132245. I have to admit I don't totally understand what I've updated, or whether there are more integration points needed. --- dev/devicelab/README.md | 11 ++++++--- dev/devicelab/bin/run.dart | 27 +++++++++++++++++++++- dev/devicelab/lib/command/test.dart | 10 ++++++++ dev/devicelab/lib/framework/ab.dart | 6 ++++- dev/devicelab/lib/framework/framework.dart | 4 ++++ dev/devicelab/lib/framework/runner.dart | 6 +++++ dev/devicelab/lib/framework/utils.dart | 10 ++++++++ dev/devicelab/lib/tasks/perf_tests.dart | 5 ++++ dev/devicelab/test/ab_test.dart | 2 +- dev/devicelab/test/run_test.dart | 2 +- dev/devicelab/test/utils_test.dart | 1 + 11 files changed, 77 insertions(+), 7 deletions(-) diff --git a/dev/devicelab/README.md b/dev/devicelab/README.md index afa8300f15..69ba2d61ea 100644 --- a/dev/devicelab/README.md +++ b/dev/devicelab/README.md @@ -91,10 +91,12 @@ flags to `bin/run.dart`: ```sh ../../bin/cache/dart-sdk/bin/dart bin/run.dart --task=[some_task] \ --local-engine-src-path=[path_to_local]/engine/src \ - --local-engine=[local_engine_architecture] + --local-engine=[local_engine_architecture] \ + --local-engine-host=[local_engine_host_architecture] ``` -An example of a local engine architecture is `android_debug_unopt_x86`. +An example of a local engine architecture is `android_debug_unopt_x86` and +an example of a local engine host architecture is `host_debug_unopt`. ### Running an A/B test for engine changes @@ -111,13 +113,16 @@ Example: ```sh ../../bin/cache/dart-sdk/bin/dart bin/run.dart --ab=10 \ --local-engine=host_debug_unopt \ + --local-engine-host=host_debug_unopt \ -t bin/tasks/web_benchmarks_canvaskit.dart ``` The `--ab=10` tells the runner to run an A/B test 10 times. `--local-engine=host_debug_unopt` tells the A/B test to use the -`host_debug_unopt` engine build. `--local-engine` is required for A/B test. +`host_debug_unopt` engine build. `--local-engine-host=host_debug_unopt` uses +the same engine build to run the `frontend_server` (in this example). +`--local-engine` is required for A/B test. `--ab-result-file=filename` can be used to provide an alternate location to output the JSON results file (defaults to `ABresults#.json`). A single `#` diff --git a/dev/devicelab/bin/run.dart b/dev/devicelab/bin/run.dart index cc0cb1bb29..28da34ad50 100644 --- a/dev/devicelab/bin/run.dart +++ b/dev/devicelab/bin/run.dart @@ -38,6 +38,11 @@ Future main(List rawArgs) async { /// Required for A/B test mode. final String? localEngine = args['local-engine'] as String?; + /// The build of the local engine to use as the host platform. + /// + /// Required if [localEngine] is set. + final String? localEngineHost = args['local-engine-host'] as String?; + /// The build of the local Web SDK to use. /// /// Required for A/B test mode. @@ -95,10 +100,16 @@ Future main(List rawArgs) async { stderr.writeln(argParser.usage); exit(1); } + if (localEngineHost == null) { + stderr.writeln('When running in A/B test mode --local-engine-host is required.\n'); + stderr.writeln(argParser.usage); + exit(1); + } await _runABTest( runsPerTest: runsPerTest, silent: silent, localEngine: localEngine, + localEngineHost: localEngineHost, localWebSdk: localWebSdk, localEngineSrcPath: localEngineSrcPath, deviceId: deviceId, @@ -125,6 +136,7 @@ Future _runABTest({ required int runsPerTest, required bool silent, required String? localEngine, + required String localEngineHost, required String? localWebSdk, required String? localEngineSrcPath, required String? deviceId, @@ -135,7 +147,11 @@ Future _runABTest({ assert(localEngine != null || localWebSdk != null); - final ABTest abTest = ABTest((localEngine ?? localWebSdk)!, taskName); + final ABTest abTest = ABTest( + localEngine: (localEngine ?? localWebSdk)!, + localEngineHost: localEngineHost, + taskName: taskName, + ); for (int i = 1; i <= runsPerTest; i++) { section('Run #$i'); @@ -292,6 +308,15 @@ ArgParser createArgParser(List taskNames) { 'This path is relative to --local-engine-src-path/out. This option\n' 'is required when running an A/B test (see the --ab option).', ) + ..addOption( + 'local-engine-host', + help: 'Name of a build output within the engine out directory, if you\n' + 'are building Flutter locally. Use this to select a specific\n' + 'version of the engine to use as the host platform if you have built ' + 'multiple engine targets.\n' + 'This path is relative to --local-engine-src-path/out. This option\n' + 'is required when running an A/B test (see the --ab option).', + ) ..addOption( 'local-web-sdk', help: 'Name of a build output within the engine out directory, if you\n' diff --git a/dev/devicelab/lib/command/test.dart b/dev/devicelab/lib/command/test.dart index a98d2b3ece..186999403f 100644 --- a/dev/devicelab/lib/command/test.dart +++ b/dev/devicelab/lib/command/test.dart @@ -37,6 +37,15 @@ class TestCommand extends Command { 'This path is relative to --local-engine-src-path/out. This option\n' 'is required when running an A/B test (see the --ab option).', ); + argParser.addOption( + 'local-engine-host', + help: 'Name of a build output within the engine out directory, if you\n' + 'are building Flutter locally. Use this to select a specific\n' + 'version of the engine to use as the host platform if you have built ' + 'multiple engine targets.\n' + 'This path is relative to --local-engine-src-path/out. This option\n' + 'is required when running an A/B test (see the --ab option).', + ); argParser.addOption( 'local-engine-src-path', help: 'Path to your engine src directory, if you are building Flutter\n' @@ -74,6 +83,7 @@ class TestCommand extends Command { deviceId: argResults!['device-id'] as String?, gitBranch: argResults!['git-branch'] as String?, localEngine: argResults!['local-engine'] as String?, + localEngineHost: argResults!['local-engine-host'] as String?, localEngineSrcPath: argResults!['local-engine-src-path'] as String?, luciBuilder: argResults!['luci-builder'] as String?, resultsPath: argResults!['results-file'] as String?, diff --git a/dev/devicelab/lib/framework/ab.dart b/dev/devicelab/lib/framework/ab.dart index e7da558550..0e05a130bf 100644 --- a/dev/devicelab/lib/framework/ab.dart +++ b/dev/devicelab/lib/framework/ab.dart @@ -9,6 +9,7 @@ import 'task_result.dart'; const String kBenchmarkTypeKeyName = 'benchmark_type'; const String kBenchmarkVersionKeyName = 'version'; const String kLocalEngineKeyName = 'local_engine'; +const String kLocalEngineHostKeyName = 'local_engine_host'; const String kTaskNameKeyName = 'task_name'; const String kRunStartKeyName = 'run_start'; const String kRunEndKeyName = 'run_end'; @@ -24,13 +25,14 @@ enum FieldJustification { LEFT, RIGHT, CENTER } /// /// See [printSummary] for more. class ABTest { - ABTest(this.localEngine, this.taskName) + ABTest({required this.localEngine, required this.localEngineHost, required this.taskName}) : runStart = DateTime.now(), _aResults = >{}, _bResults = >{}; ABTest.fromJsonMap(Map jsonResults) : localEngine = jsonResults[kLocalEngineKeyName] as String, + localEngineHost = jsonResults[kLocalEngineHostKeyName] as String, taskName = jsonResults[kTaskNameKeyName] as String, runStart = DateTime.parse(jsonResults[kRunStartKeyName] as String), _runEnd = DateTime.parse(jsonResults[kRunEndKeyName] as String), @@ -38,6 +40,7 @@ class ABTest { _bResults = _convertFrom(jsonResults[kBResultsKeyName] as Map); final String localEngine; + final String localEngineHost; final String taskName; final DateTime runStart; DateTime? _runEnd; @@ -86,6 +89,7 @@ class ABTest { kBenchmarkTypeKeyName: kBenchmarkResultsType, kBenchmarkVersionKeyName: kBenchmarkABVersion, kLocalEngineKeyName: localEngine, + kLocalEngineHostKeyName: localEngineHost, kTaskNameKeyName: taskName, kRunStartKeyName: runStart.toIso8601String(), kRunEndKeyName: runEnd!.toIso8601String(), diff --git a/dev/devicelab/lib/framework/framework.dart b/dev/devicelab/lib/framework/framework.dart index 2282907aaf..d64fe2e150 100644 --- a/dev/devicelab/lib/framework/framework.dart +++ b/dev/devicelab/lib/framework/framework.dart @@ -74,11 +74,13 @@ class _TaskRunner { final bool runFlutterConfig = parameters['runFlutterConfig'] != 'false'; // used by tests to avoid changing the configuration final bool runProcessCleanup = parameters['runProcessCleanup'] != 'false'; final String? localEngine = parameters['localEngine']; + final String? localEngineHost = parameters['localEngineHost']; final TaskResult result = await run( taskTimeout, runProcessCleanup: runProcessCleanup, runFlutterConfig: runFlutterConfig, localEngine: localEngine, + localEngineHost: localEngineHost, ); return ServiceExtensionResponse.result(json.encode(result.toJson())); }); @@ -115,6 +117,7 @@ class _TaskRunner { bool runFlutterConfig = true, bool runProcessCleanup = true, required String? localEngine, + required String? localEngineHost, }) async { try { _taskStarted = true; @@ -144,6 +147,7 @@ class _TaskRunner { '--enable-macos-desktop', '--enable-linux-desktop', if (localEngine != null) ...['--local-engine', localEngine], + if (localEngineHost != null) ...['--local-engine-host', localEngineHost], ], canFail: true); if (configResult != 0) { print('Failed to enable configuration, tasks may not run.'); diff --git a/dev/devicelab/lib/framework/runner.dart b/dev/devicelab/lib/framework/runner.dart index 88a81d81ba..5f11b4ffc7 100644 --- a/dev/devicelab/lib/framework/runner.dart +++ b/dev/devicelab/lib/framework/runner.dart @@ -35,6 +35,7 @@ Future runTasks( String? deviceId, String? gitBranch, String? localEngine, + String? localEngineHost, String? localEngineSrcPath, String? luciBuilder, String? resultsPath, @@ -52,6 +53,7 @@ Future runTasks( taskName, deviceId: deviceId, localEngine: localEngine, + localEngineHost: localEngineHost, localEngineSrcPath: localEngineSrcPath, terminateStrayDartProcesses: terminateStrayDartProcesses, silent: silent, @@ -99,6 +101,7 @@ Future rerunTask( String taskName, { String? deviceId, String? localEngine, + String? localEngineHost, String? localEngineSrcPath, bool terminateStrayDartProcesses = false, bool silent = false, @@ -114,6 +117,7 @@ Future rerunTask( taskName, deviceId: deviceId, localEngine: localEngine, + localEngineHost: localEngineHost, localEngineSrcPath: localEngineSrcPath, terminateStrayDartProcesses: terminateStrayDartProcesses, silent: silent, @@ -153,6 +157,7 @@ Future runTask( bool terminateStrayDartProcesses = false, bool silent = false, String? localEngine, + String? localEngineHost, String? localWebSdk, String? localEngineSrcPath, String? deviceId, @@ -182,6 +187,7 @@ Future runTask( '--enable-vm-service=0', // zero causes the system to choose a free port '--no-pause-isolates-on-exit', if (localEngine != null) '-DlocalEngine=$localEngine', + if (localEngineHost != null) '-DlocalEngineHost=$localEngineHost', if (localWebSdk != null) '-DlocalWebSdk=$localWebSdk', if (localEngineSrcPath != null) '-DlocalEngineSrcPath=$localEngineSrcPath', taskExecutable, diff --git a/dev/devicelab/lib/framework/utils.dart b/dev/devicelab/lib/framework/utils.dart index 20301e51ee..8b97890ac2 100644 --- a/dev/devicelab/lib/framework/utils.dart +++ b/dev/devicelab/lib/framework/utils.dart @@ -26,6 +26,14 @@ String? get localEngineFromEnv { return isDefined ? const String.fromEnvironment('localEngine') : null; } +/// The local engine host to use for [flutter] and [evalFlutter], if any. +/// +/// This is set as an environment variable when running the task, see runTask in runner.dart. +String? get localEngineHostFromEnv { + const bool isDefined = bool.hasEnvironment('localEngineHost'); + return isDefined ? const String.fromEnvironment('localEngineHost') : null; +} + /// The local engine source path to use if a local engine is used for [flutter] /// and [evalFlutter]. /// @@ -453,6 +461,7 @@ List _flutterCommandArgs(String command, List options) { 'screenshot', }; final String? localEngine = localEngineFromEnv; + final String? localEngineHost = localEngineHostFromEnv; final String? localEngineSrcPath = localEngineSrcPathFromEnv; final String? localWebSdk = localWebSdkFromEnv; return [ @@ -468,6 +477,7 @@ List _flutterCommandArgs(String command, List options) { hostAgent.dumpDirectory!.path, ], if (localEngine != null) ...['--local-engine', localEngine], + if (localEngineHost != null) ...['--local-engine-host', localEngineHost], if (localEngineSrcPath != null) ...['--local-engine-src-path', localEngineSrcPath], if (localWebSdk != null) ...['--local-web-sdk', localWebSdk], ...options, diff --git a/dev/devicelab/lib/tasks/perf_tests.dart b/dev/devicelab/lib/tasks/perf_tests.dart index 9416e505b2..989b5dcc00 100644 --- a/dev/devicelab/lib/tasks/perf_tests.dart +++ b/dev/devicelab/lib/tasks/perf_tests.dart @@ -1169,6 +1169,7 @@ class PerfTest { await selectedDevice.unlock(); final String deviceId = selectedDevice.deviceId; final String? localEngine = localEngineFromEnv; + final String? localEngineHost = localEngineHostFromEnv; final String? localEngineSrcPath = localEngineSrcPathFromEnv; Future Function()? manifestReset; @@ -1181,6 +1182,10 @@ class PerfTest { try { final List options = [ if (localEngine != null) ...['--local-engine', localEngine], + if (localEngineHost != null) ...[ + '--local-engine-host', + localEngineHost + ], if (localEngineSrcPath != null) ...[ '--local-engine-src-path', localEngineSrcPath diff --git a/dev/devicelab/test/ab_test.dart b/dev/devicelab/test/ab_test.dart index 27e92fd687..b6ff15b1c2 100644 --- a/dev/devicelab/test/ab_test.dart +++ b/dev/devicelab/test/ab_test.dart @@ -9,7 +9,7 @@ import 'common.dart'; void main() { test('ABTest', () { - final ABTest ab = ABTest('engine', 'test'); + final ABTest ab = ABTest(localEngine: 'engine', localEngineHost: 'engine', taskName: 'test'); for (int i = 0; i < 5; i++) { final TaskResult aResult = TaskResult.fromJson({ diff --git a/dev/devicelab/test/run_test.dart b/dev/devicelab/test/run_test.dart index a5332dd53c..933c7391a3 100644 --- a/dev/devicelab/test/run_test.dart +++ b/dev/devicelab/test/run_test.dart @@ -96,7 +96,7 @@ void main() { final ProcessResult result = await runScript( ['smoke_test_success'], - ['--ab=2', '--local-engine=host_debug_unopt', '--ab-result-file', abResultsFile.path], + ['--ab=2', '--local-engine=host_debug_unopt', '--local-engine-host=host_debug_unopt', '--ab-result-file', abResultsFile.path], ); expect(result.exitCode, 0); diff --git a/dev/devicelab/test/utils_test.dart b/dev/devicelab/test/utils_test.dart index c51ac5afa9..f137140abe 100644 --- a/dev/devicelab/test/utils_test.dart +++ b/dev/devicelab/test/utils_test.dart @@ -37,6 +37,7 @@ void main() { group('engine environment declarations', () { test('localEngine', () { expect(localEngineFromEnv, null); + expect(localEngineHostFromEnv, null); expect(localEngineSrcPathFromEnv, null); }); });