From 771a07bfe9e8df8214ddd0eae579e79dba8c298e Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Wed, 15 May 2024 16:38:29 -0700 Subject: [PATCH 1/6] Include stdout on a failed gn desc call. --- tools/engine_tool/lib/src/gn.dart | 12 ++- tools/engine_tool/test/gn_test.dart | 112 ++++++++++++++++++++++++++++ 2 files changed, 123 insertions(+), 1 deletion(-) create mode 100644 tools/engine_tool/test/gn_test.dart diff --git a/tools/engine_tool/lib/src/gn.dart b/tools/engine_tool/lib/src/gn.dart index 986c47c20a04a..b8812dffc7194 100644 --- a/tools/engine_tool/lib/src/gn.dart +++ b/tools/engine_tool/lib/src/gn.dart @@ -67,7 +67,7 @@ interface class Gn { result = JsonObject.parse(process.stdout); } on FormatException catch (e) { _environment.logger.fatal( - 'Failed to parse JSON output from `gn desc`:\n$e', + 'Failed to parse JSON output from `gn desc`:\n$e\n${process.stdout}', ); } @@ -136,6 +136,10 @@ sealed class BuildTarget { @mustBeOverridden @override int get hashCode; + + @mustBeOverridden + @override + String toString(); } /// A build target that produces a [shared library][] or [static library][]. @@ -158,6 +162,9 @@ final class LibraryBuildTarget extends BuildTarget { @override int get hashCode => Object.hash(label, testOnly); + + @override + String toString() => 'LibraryBuildTarget($label, testOnly=$testOnly)'; } /// A build target that produces an [executable][] program. @@ -184,4 +191,7 @@ final class ExecutableBuildTarget extends BuildTarget { @override int get hashCode => Object.hash(label, testOnly, executable); + + @override + String toString() => 'ExecutableBuildTarget($label, testOnly=$testOnly, executable=$executable)'; } diff --git a/tools/engine_tool/test/gn_test.dart b/tools/engine_tool/test/gn_test.dart new file mode 100644 index 0000000000000..8bebbd335c7ab --- /dev/null +++ b/tools/engine_tool/test/gn_test.dart @@ -0,0 +1,112 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'package:engine_tool/src/gn.dart'; +import 'package:engine_tool/src/label.dart'; +import 'package:litetest/litetest.dart'; + +import 'utils.dart'; + +void main() { + test('gn.desc handles a non-zero exit code', () async { + final TestEnvironment testEnv = TestEnvironment.withTestEngine( + cannedProcesses: [ + CannedProcess( + (List command) => command.contains('desc'), + exitCode: 1, + stdout: 'stdout', + stderr: 'stderr', + ), + ], + ); + try { + final Gn gn = Gn.fromEnvironment(testEnv.environment); + await gn.desc('out/Release', TargetPattern('//foo', 'bar')); + fail('Expected an exception'); + } catch (e) { + final String message = '$e'; + expect(message, contains('Failed to run')); + expect(message, contains('exit code 1')); + expect(message, contains('STDOUT:\nstdout')); + expect(message, contains('STDERR:\nstderr')); + } finally { + testEnv.cleanup(); + } + }); + + test('gn.desc handles unparseable stdout', () async { + final TestEnvironment testEnv = TestEnvironment.withTestEngine( + cannedProcesses: [ + CannedProcess( + (List command) => command.contains('desc'), + stdout: 'not json', + ), + ], + ); + try { + final Gn gn = Gn.fromEnvironment(testEnv.environment); + await gn.desc('out/Release', TargetPattern('//foo', 'bar')); + fail('Expected an exception'); + } catch (e) { + final String message = '$e'; + expect(message, contains('Failed to parse JSON')); + expect(message, contains('not json')); + } finally { + testEnv.cleanup(); + } + }); + + test('gn.desc parses build targets', () async { + final TestEnvironment testEnv = TestEnvironment.withTestEngine( + cannedProcesses: [ + CannedProcess( + (List command) => command.contains('desc'), + stdout: ''' + { + "//foo/bar:baz_test": { + "outputs": ["//out/host_debug/foo/bar/baz_test"], + "testonly": true, + "type": "executable" + }, + "//foo/bar:baz_shared_library": { + "testonly": false, + "type": "shared_library" + }, + "//foo/bar:baz_static_library": { + "testonly": false, + "type": "static_library" + } + } + ''', + ), + ], + ); + try { + final Gn gn = Gn.fromEnvironment(testEnv.environment); + final List targets = await gn.desc('out/Release', TargetPattern('//foo', 'bar')); + expect(targets, hasLength(3)); + + // There should be exactly one binary test target and two library targets. + final ExecutableBuildTarget testTarget = targets.whereType().single; + expect(testTarget, ExecutableBuildTarget( + label: Label('//foo/bar', 'baz_test'), + testOnly: true, + executable: 'out/host_debug/foo/bar/baz_test', + )); + + final List libraryTargets = targets.whereType().toList(); + expect(libraryTargets, hasLength(2)); + expect(libraryTargets.contains(LibraryBuildTarget( + label: Label('//foo/bar', 'baz_shared_library'), + testOnly: false, + )), isTrue); + expect(libraryTargets.contains(LibraryBuildTarget( + label: Label('//foo/bar', 'baz_static_library'), + testOnly: false, + )), isTrue); + } finally { + testEnv.cleanup(); + } + }); +} From 2e1a25e80c51a747a5c7089a61f06382e235e0a7 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Wed, 15 May 2024 17:56:48 -0700 Subject: [PATCH 2/6] Fix building a missing output directory. --- tools/engine_tool/lib/src/build_utils.dart | 30 ++++++++++++++++--- .../lib/src/commands/build_command.dart | 4 +++ .../lib/src/commands/query_command.dart | 4 +++ .../lib/src/commands/test_command.dart | 4 +++ 4 files changed, 38 insertions(+), 4 deletions(-) diff --git a/tools/engine_tool/lib/src/build_utils.dart b/tools/engine_tool/lib/src/build_utils.dart index 0674129dd368c..8436c8e9e22fa 100644 --- a/tools/engine_tool/lib/src/build_utils.dart +++ b/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; + import 'package:engine_build_configs/engine_build_configs.dart'; +import 'package:path/path.dart' as p; import 'environment.dart'; import 'label.dart'; @@ -179,13 +182,22 @@ Future runBuild( return buildResult ? 0 : 1; } -/// Given a [Build] object, run only its GN step. -Future runGn( +/// Run a [build]'s GN step if the output directory is missing. +/// +/// TODO(matanlurey): https://github.com/flutter/flutter/issues/148442. +Future ensureBuildDir( Environment environment, Build build, { List extraGnArgs = const [], required bool enableRbe, }) async { + final io.Directory buildDir = io.Directory( + p.join(environment.engine.outDir.path, build.ninja.config), + ); + if (buildDir.existsSync()) { + return true; + } + final List gnArgs = [ if (!enableRbe) '--no-rbe', ...extraGnArgs, @@ -203,12 +215,22 @@ Future runGn( runTests: false, ); - final bool buildResult = await buildRunner.run((RunnerEvent event) { + final bool built = await buildRunner.run((RunnerEvent event) { switch (event) { case RunnerResult(ok: false): environment.logger.error(event); default: } }); - return buildResult ? 0 : 1; + if (built) { + if (!buildDir.existsSync()) { + environment.logger.error( + 'The specified build did not produce the expected output directory: ' + '${buildDir.path}', + ); + return false; + } + return true; + } + return false; } diff --git a/tools/engine_tool/lib/src/commands/build_command.dart b/tools/engine_tool/lib/src/commands/build_command.dart index 91f024732dc1c..8aa940c7cc7b6 100644 --- a/tools/engine_tool/lib/src/commands/build_command.dart +++ b/tools/engine_tool/lib/src/commands/build_command.dart @@ -76,6 +76,10 @@ et build //flutter/fml:fml_benchmarks # Build a specific target in `//flutter/f if (useLto) '--lto' else '--no-lto', ]; + if (!await ensureBuildDir(environment, build, enableRbe: useRbe)) { + return 1; + } + // Builds only accept labels as arguments, so convert patterns to labels. // TODO(matanlurey): Can be optimized in cases where wildcards are not used. final Gn gn = Gn.fromEnvironment(environment); diff --git a/tools/engine_tool/lib/src/commands/query_command.dart b/tools/engine_tool/lib/src/commands/query_command.dart index 991623131aa75..37ed01031a290 100644 --- a/tools/engine_tool/lib/src/commands/query_command.dart +++ b/tools/engine_tool/lib/src/commands/query_command.dart @@ -195,6 +195,10 @@ et query targets //flutter/fml/... # List all targets under `//flutter/fml` return 1; } + if (!await ensureBuildDir(environment, build, enableRbe: useRbe)) { + return 1; + } + // Builds only accept labels as arguments, so convert patterns to labels. // TODO(matanlurey): Can be optimized in cases where wildcards are not used. final Gn gn = Gn.fromEnvironment(environment); diff --git a/tools/engine_tool/lib/src/commands/test_command.dart b/tools/engine_tool/lib/src/commands/test_command.dart index 381ceba75ceaa..a48e10efec093 100644 --- a/tools/engine_tool/lib/src/commands/test_command.dart +++ b/tools/engine_tool/lib/src/commands/test_command.dart @@ -68,6 +68,10 @@ et test //flutter/fml:fml_benchmarks # Run a single test target in `//flutter/f return 1; } + if (!await ensureBuildDir(environment, build, enableRbe: useRbe)) { + return 1; + } + // Builds only accept labels as arguments, so convert patterns to labels. final Gn gn = Gn.fromEnvironment(environment); From ad5de84e3506bc13b92821a1a6f91f69bfb7eed7 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Wed, 15 May 2024 18:44:28 -0700 Subject: [PATCH 3/6] Fix tests. --- tools/engine_tool/lib/src/build_utils.dart | 8 +++-- .../engine_tool/test/build_command_test.dart | 31 ++++++++++++------- tools/engine_tool/test/utils.dart | 5 +-- 3 files changed, 28 insertions(+), 16 deletions(-) diff --git a/tools/engine_tool/lib/src/build_utils.dart b/tools/engine_tool/lib/src/build_utils.dart index 8436c8e9e22fa..fa9bcab51fac8 100644 --- a/tools/engine_tool/lib/src/build_utils.dart +++ b/tools/engine_tool/lib/src/build_utils.dart @@ -183,16 +183,18 @@ Future runBuild( } /// Run a [build]'s GN step if the output directory is missing. -/// -/// TODO(matanlurey): https://github.com/flutter/flutter/issues/148442. Future ensureBuildDir( Environment environment, Build build, { List extraGnArgs = const [], required bool enableRbe, }) async { + // TODO(matanlurey): https://github.com/flutter/flutter/issues/148442. final io.Directory buildDir = io.Directory( - p.join(environment.engine.outDir.path, build.ninja.config), + p.join( + environment.engine.outDir.path, + build.ninja.config, + ), ); if (buildDir.existsSync()) { return true; diff --git a/tools/engine_tool/test/build_command_test.dart b/tools/engine_tool/test/build_command_test.dart index c8af8cc3fe68a..d5c10d076ea95 100644 --- a/tools/engine_tool/test/build_command_test.dart +++ b/tools/engine_tool/test/build_command_test.dart @@ -64,6 +64,7 @@ void main() { test('build command invokes gn', () async { final TestEnvironment testEnv = TestEnvironment.withTestEngine( cannedProcesses: cannedProcesses, + expectedOutDir: 'build_name', ); try { final ToolCommandRunner runner = ToolCommandRunner( @@ -86,6 +87,7 @@ void main() { test('build command invokes ninja', () async { final TestEnvironment testEnv = TestEnvironment.withTestEngine( cannedProcesses: cannedProcesses, + expectedOutDir: 'build_name', ); try { final ToolCommandRunner runner = ToolCommandRunner( @@ -99,7 +101,7 @@ void main() { ]); expect(result, equals(0)); expect(testEnv.processHistory.length, greaterThanOrEqualTo(2)); - expect(testEnv.processHistory[1].command[0], contains('ninja')); + expect(testEnv.processHistory[2].command[0], contains('ninja')); } finally { testEnv.cleanup(); } @@ -108,6 +110,7 @@ void main() { test('build command invokes generator', () async { final TestEnvironment testEnv = TestEnvironment.withTestEngine( cannedProcesses: cannedProcesses, + expectedOutDir: 'build_name', ); try { final ToolCommandRunner runner = ToolCommandRunner( @@ -122,7 +125,7 @@ void main() { expect(result, equals(0)); expect(testEnv.processHistory.length, greaterThanOrEqualTo(3)); expect( - testEnv.processHistory[2].command, + testEnv.processHistory[3].command, containsStringsInOrder(['python3', 'gen/script.py']), ); } finally { @@ -133,6 +136,7 @@ void main() { test('build command does not invoke tests', () async { final TestEnvironment testEnv = TestEnvironment.withTestEngine( cannedProcesses: cannedProcesses, + expectedOutDir: 'build_name', ); try { final ToolCommandRunner runner = ToolCommandRunner( @@ -155,6 +159,7 @@ void main() { final TestEnvironment testEnv = TestEnvironment.withTestEngine( withRbe: true, cannedProcesses: cannedProcesses, + expectedOutDir: 'android_debug_rbe_arm64', ); try { final ToolCommandRunner runner = ToolCommandRunner( @@ -167,10 +172,10 @@ void main() { 'ci/android_debug_rbe_arm64', ]); expect(result, equals(0)); - expect(testEnv.processHistory[0].command[0], - contains(path.join('tools', 'gn'))); - expect(testEnv.processHistory[0].command[2], equals('--rbe')); expect(testEnv.processHistory[1].command[0], + contains(path.join('tools', 'gn'))); + expect(testEnv.processHistory[1].command[2], equals('--rbe')); + expect(testEnv.processHistory[2].command[0], contains(path.join('reclient', 'bootstrap'))); } finally { testEnv.cleanup(); @@ -208,6 +213,7 @@ void main() { final TestEnvironment testEnv = TestEnvironment.withTestEngine( withRbe: true, cannedProcesses: cannedProcesses, + expectedOutDir: 'android_debug_rbe_arm64', ); try { final ToolCommandRunner runner = ToolCommandRunner( @@ -225,7 +231,7 @@ void main() { contains(path.join('tools', 'gn'))); expect(testEnv.processHistory[0].command, doesNotContainAny(['--rbe'])); - expect(testEnv.processHistory[1].command[0], + expect(testEnv.processHistory[2].command[0], contains(path.join('ninja', 'ninja'))); } finally { testEnv.cleanup(); @@ -236,6 +242,7 @@ void main() { () async { final TestEnvironment testEnv = TestEnvironment.withTestEngine( cannedProcesses: cannedProcesses, + expectedOutDir: 'android_debug_rbe_arm64', ); try { final ToolCommandRunner runner = ToolCommandRunner( @@ -252,7 +259,7 @@ void main() { contains(path.join('tools', 'gn'))); expect(testEnv.processHistory[0].command, doesNotContainAny(['--rbe'])); - expect(testEnv.processHistory[1].command[0], + expect(testEnv.processHistory[2].command[0], contains(path.join('ninja', 'ninja'))); } finally { testEnv.cleanup(); @@ -309,6 +316,7 @@ void main() { }; final TestEnvironment testEnv = TestEnvironment.withTestEngine( cannedProcesses: cannedProcesses, + expectedOutDir: 'local_host_debug', ); try { final ToolCommandRunner runner = ToolCommandRunner( @@ -321,10 +329,10 @@ void main() { 'host_debug', ]); expect(result, equals(0)); - expect(testEnv.processHistory[1].command[0], + expect(testEnv.processHistory[2].command[0], contains(path.join('ninja', 'ninja'))); expect( - testEnv.processHistory[1].command[2], contains('local_host_debug')); + testEnv.processHistory[2].command[2], contains('local_host_debug')); } finally { testEnv.cleanup(); } @@ -341,6 +349,7 @@ void main() { }; final TestEnvironment testEnv = TestEnvironment.withTestEngine( cannedProcesses: cannedProcesses, + expectedOutDir: 'ci/host_debug', ); final Environment env = testEnv.environment; try { @@ -354,9 +363,9 @@ void main() { 'ci/host_debug', ]); expect(result, equals(0)); - expect(testEnv.processHistory[1].command[0], + expect(testEnv.processHistory[2].command[0], contains(path.join('ninja', 'ninja'))); - expect(testEnv.processHistory[1].command[2], contains('ci/host_debug')); + expect(testEnv.processHistory[2].command[2], contains('ci/host_debug')); } finally { testEnv.cleanup(); } diff --git a/tools/engine_tool/test/utils.dart b/tools/engine_tool/test/utils.dart index b00e009d546a0..a21954c6f8c80 100644 --- a/tools/engine_tool/test/utils.dart +++ b/tools/engine_tool/test/utils.dart @@ -89,6 +89,7 @@ class TestEnvironment { ffi.Abi abi = ffi.Abi.linuxX64, List cannedProcesses = const [], bool verbose = false, + String expectedOutDir = 'host_debug', }) { final io.Directory rootDir = io.Directory.systemTemp.createTempSync('et'); final TestEngine engine = TestEngine.createTemp(rootDir: rootDir); @@ -100,12 +101,12 @@ class TestEnvironment { 'rbe', )).createSync(recursive: true); } - // When GN runs, always try to create out/host_debug. + // When GN runs, always try to create {expectedOutDir}. final CannedProcess cannedGn = CannedProcess((List command) { if (command[0].endsWith('/gn') && !command.contains('desc')) { io.Directory(path.join( engine.outDir.path, - 'host_debug', + expectedOutDir, )).createSync(recursive: true); return true; } From 0a733f3a3cc01faa73f4d09485a6e2075933cd27 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Wed, 15 May 2024 20:05:37 -0700 Subject: [PATCH 4/6] Address feedback. --- tools/engine_tool/lib/src/build_utils.dart | 37 ++++++++++++------- .../lib/src/commands/build_command.dart | 5 ++- .../engine_tool/test/build_command_test.dart | 22 +++++------ 3 files changed, 38 insertions(+), 26 deletions(-) diff --git a/tools/engine_tool/lib/src/build_utils.dart b/tools/engine_tool/lib/src/build_utils.dart index fa9bcab51fac8..6c29fe15b486b 100644 --- a/tools/engine_tool/lib/src/build_utils.dart +++ b/tools/engine_tool/lib/src/build_utils.dart @@ -187,7 +187,7 @@ Future ensureBuildDir( Environment environment, Build build, { List extraGnArgs = const [], - required bool enableRbe, + required bool enableRbe, }) async { // TODO(matanlurey): https://github.com/flutter/flutter/issues/148442. final io.Directory buildDir = io.Directory( @@ -200,6 +200,28 @@ Future ensureBuildDir( return true; } + final bool built = await _runGn( + environment, + build, + extraGnArgs: extraGnArgs, + enableRbe: enableRbe, + ); + if (built && !buildDir.existsSync()) { + environment.logger.error( + 'The specified build did not produce the expected output directory: ' + '${buildDir.path}', + ); + return false; + } + return built; +} + +Future _runGn( + Environment environment, + Build build, { + List extraGnArgs = const [], + required bool enableRbe, +}) async { final List gnArgs = [ if (!enableRbe) '--no-rbe', ...extraGnArgs, @@ -217,22 +239,11 @@ Future ensureBuildDir( runTests: false, ); - final bool built = await buildRunner.run((RunnerEvent event) { + return buildRunner.run((RunnerEvent event) { switch (event) { case RunnerResult(ok: false): environment.logger.error(event); default: } }); - if (built) { - if (!buildDir.existsSync()) { - environment.logger.error( - 'The specified build did not produce the expected output directory: ' - '${buildDir.path}', - ); - return false; - } - return true; - } - return false; } diff --git a/tools/engine_tool/lib/src/commands/build_command.dart b/tools/engine_tool/lib/src/commands/build_command.dart index 8aa940c7cc7b6..9dc0c42c4ed3a 100644 --- a/tools/engine_tool/lib/src/commands/build_command.dart +++ b/tools/engine_tool/lib/src/commands/build_command.dart @@ -76,7 +76,8 @@ et build //flutter/fml:fml_benchmarks # Build a specific target in `//flutter/f if (useLto) '--lto' else '--no-lto', ]; - if (!await ensureBuildDir(environment, build, enableRbe: useRbe)) { + final List commandLineTargets = argResults!.rest; + if (commandLineTargets.isNotEmpty && !await ensureBuildDir(environment, build, enableRbe: useRbe)) { return 1; } @@ -84,7 +85,7 @@ et build //flutter/fml:fml_benchmarks # Build a specific target in `//flutter/f // TODO(matanlurey): Can be optimized in cases where wildcards are not used. final Gn gn = Gn.fromEnvironment(environment); final Set