Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an initial 'build' command to engine_tool #50681

Merged
merged 1 commit into from
Feb 23, 2024
Merged
Changes from all commits
Commits
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
52 changes: 52 additions & 0 deletions tools/engine_tool/lib/src/build_utils.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
// 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_build_configs/engine_build_configs.dart';
johnmccutchan marked this conversation as resolved.
Show resolved Hide resolved

import 'environment.dart';

/// A function that returns true or false when given a [BuildConfig] and its
/// name.
typedef ConfigFilter = bool Function(String name, BuildConfig config);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally (here and below) I'd just inline the function definition.

That avoids having to explain these generically "returns true or false" and you can just document how it's used in the (single?) function below.


/// A function that returns true or false when given a [BuildConfig] name
/// and a [GlobalBuild].
typedef BuildFilter = bool Function(String configName, GlobalBuild build);

/// Returns a filtered copy of [input] filtering out configs where test
/// returns false.
Map<String, BuildConfig> filterBuildConfigs(
Map<String, BuildConfig> input, ConfigFilter test) {
return <String, BuildConfig>{
for (final MapEntry<String, BuildConfig> entry in input.entries)
if (test(entry.key, entry.value)) entry.key: entry.value,
};
}

/// Returns a copy of [input] filtering out configs that are not runnable
/// on the current platform.
Map<String, BuildConfig> runnableBuildConfigs(
Environment env, Map<String, BuildConfig> input) {
return filterBuildConfigs(input, (String name, BuildConfig config) {
return config.canRunOn(env.platform);
});
}

/// Returns a List of [GlobalBuild] that match test.
List<GlobalBuild> filterBuilds(
Map<String, BuildConfig> input, BuildFilter test) {
return <GlobalBuild>[
for (final MapEntry<String, BuildConfig> entry in input.entries)
for (final GlobalBuild build in entry.value.builds)
if (test(entry.key, build)) build,
];
}

/// Returns a list of runnable builds.
List<GlobalBuild> runnableBuilds(
Environment env, Map<String, BuildConfig> input) {
return filterBuilds(input, (String configName, GlobalBuild build) {
return build.canRunOn(env.platform);
});
}
92 changes: 92 additions & 0 deletions tools/engine_tool/lib/src/commands/build_command.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
// 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_build_configs/engine_build_configs.dart';
johnmccutchan marked this conversation as resolved.
Show resolved Hide resolved

import '../build_utils.dart';

import 'command.dart';

const String _configFlag = 'config';

// TODO(johnmccutchan): Should BuildConfig be BuilderConfig and GlobalBuild be BuildConfig?
// TODO(johnmccutchan): List all available build targets and allow the user
// to specify which one(s) we should build on the cli.
// TODO(johnmccutchan): Can we show a progress indicator like 'running gn...'?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think we can copy some code from flutter_tool for this.


/// The root 'build' command.
final class BuildCommand extends CommandBase {
/// Constructs the 'build' command.
BuildCommand({
required super.environment,
required Map<String, BuildConfig> configs,
}) {
builds = runnableBuilds(environment, configs);
// Add options here that are common to all queries.
argParser.addOption(
_configFlag,
abbr: 'c',
defaultsTo: 'host_debug',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely a TODO (unless you feel inspired) but I have the perfect default for you here:

https://github.com/flutter/engine/tree/main/tools/pkg/engine_repo_tools

Specifically:

/// Returns the most recently modified output target in [outDir].
///
/// If there are no output targets, returns `null`.
Output? latestOutput() {
final List<Output> outputs = this.outputs();
if (outputs.isEmpty) {
return null;
}
outputs.sort((Output a, Output b) {
return b.path.statSync().modified.compareTo(a.path.statSync().modified);
});
return outputs.first;
}
}

You can see a recent example of this being used here (not exactly the same, but close):

defaultsTo:
engine?.
outputs().
where((Output o) => basename(o.path.path).startsWith('android_')).
firstOrNull?.
path.path,

Copy link
Member

@loic-sharma loic-sharma Feb 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I'd rather have a deterministic default.

For Crank, you can set your default builder in a ~/.config/crank/config.json config file.

This config file also lets you add your own custom builders, which is useful if the CI builders don't support your desired scenario like Android from a macOS host. See: https://github.com/loic-sharma/crank#custom-builders

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hardcoding a default for now sgtm. I suspect we'll want a config file with defaults at some point, but that's big enough for a separate PR.

help: 'Specify the build config to use',
allowed: <String>[
for (final GlobalBuild config in runnableBuilds(environment, configs))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (final GlobalBuild config in runnableBuilds(environment, configs))
for (final GlobalBuild config in builds)

config.name,
],
allowedHelp: <String, String>{
for (final GlobalBuild config in runnableBuilds(environment, configs))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (final GlobalBuild config in runnableBuilds(environment, configs))
for (final GlobalBuild config in builds)

config.name: config.gn.join(' '),
},
);
}

/// List of compatible builds.
late final List<GlobalBuild> builds;

@override
String get name => 'build';

@override
String get description => 'Builds the engine';

@override
Future<int> run() async {
final String configName = argResults![_configFlag] as String;
final GlobalBuild? build = builds
.where((GlobalBuild build) => build.name == configName)
.firstOrNull;
if (build == null) {
environment.logger.error('Could not find config $configName');
return 1;
}
final GlobalBuildRunner buildRunner = GlobalBuildRunner(
platform: environment.platform,
processRunner: environment.processRunner,
abi: environment.abi,
engineSrcDir: environment.engine.srcDir,
build: build);
void handler(RunnerEvent event) {
switch (event) {
case RunnerStart():
environment.logger.info('$event: ${event.command.join(' ')}');
case RunnerProgress(done: true):
environment.logger.clearLine();
environment.logger.status(event);
case RunnerProgress(done: false):
{
final String percent = '${event.percent.toStringAsFixed(1)}%';
final String fraction = '(${event.completed}/${event.total})';
final String prefix = '[${event.name}] $percent $fraction ';
final String what = event.what;
environment.logger.clearLine();
environment.logger.status('$prefix$what');
}
default:
environment.logger.status(event);
}
}

final bool buildResult = await buildRunner.run(handler);
return buildResult ? 0 : 1;
}
}
15 changes: 6 additions & 9 deletions tools/engine_tool/lib/src/commands/command_runner.dart
Original file line number Diff line number Diff line change
@@ -3,11 +3,10 @@
// found in the LICENSE file.

import 'package:args/command_runner.dart';

import 'package:engine_build_configs/engine_build_configs.dart';

import '../environment.dart';
import 'command.dart';
import 'build_command.dart';
import 'format_command.dart';
import 'query_command.dart';

@@ -19,14 +18,12 @@ final class ToolCommandRunner extends CommandRunner<int> {
required this.environment,
required this.configs,
}) : super(toolName, toolDescription) {
final List<CommandBase> commands = <CommandBase>[
final List<Command<int>> commands = <Command<int>>[
FormatCommand(
environment: environment,
),
QueryCommand(
environment: environment,
configs: configs,
),
QueryCommand(environment: environment, configs: configs),
BuildCommand(environment: environment, configs: configs),
];
commands.forEach(addCommand);
}
@@ -38,7 +35,7 @@ final class ToolCommandRunner extends CommandRunner<int> {
/// The description of the tool reported in the tool's usage and help
/// messages.
static const String toolDescription = 'A command line tool for working on '
'the Flutter Engine.';
'the Flutter Engine.';

/// The host system environment.
final Environment environment;
@@ -48,7 +45,7 @@ final class ToolCommandRunner extends CommandRunner<int> {

@override
Future<int> run(Iterable<String> args) async {
try{
try {
return await runCommand(parse(args)) ?? 0;
} on FormatException catch (e) {
environment.logger.error(e);
21 changes: 10 additions & 11 deletions tools/engine_tool/lib/src/commands/query_command.dart
Original file line number Diff line number Diff line change
@@ -28,8 +28,7 @@ final class QueryCommand extends CommandBase {
help: 'Restrict the query to a single builder.',
allowed: <String>[
for (final MapEntry<String, BuildConfig> entry in configs.entries)
if (entry.value.canRunOn(environment.platform))
entry.key,
if (entry.value.canRunOn(environment.platform)) entry.key,
],
allowedHelp: <String, String>{
// TODO(zanderso): Add human readable descriptions to the json files.
@@ -45,7 +44,7 @@ final class QueryCommand extends CommandBase {
negatable: false,
);

addSubcommand(QueryBuildsCommand(
addSubcommand(QueryBuildersCommand(
environment: environment,
configs: configs,
));
@@ -59,13 +58,13 @@ final class QueryCommand extends CommandBase {

@override
String get description => 'Provides information about build configurations '
'and tests.';
'and tests.';
}

// ignore: public_member_api_docs
final class QueryBuildsCommand extends CommandBase {
// ignore: public_member_api_docs
QueryBuildsCommand({
/// The 'query builds' command.
final class QueryBuildersCommand extends CommandBase {
/// Constructs the 'query build' command.
QueryBuildersCommand({
required super.environment,
required this.configs,
});
@@ -74,11 +73,11 @@ final class QueryBuildsCommand extends CommandBase {
final Map<String, BuildConfig> configs;

@override
String get name => 'builds';
String get name => 'builders';

@override
String get description => 'Provides information about CI build '
'configurations';
String get description => 'Provides information about CI builder '
'configurations';

@override
Future<int> run() async {
122 changes: 122 additions & 0 deletions tools/engine_tool/test/build_command_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
// 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 'dart:convert' as convert;
import 'dart:ffi' as ffi show Abi;
import 'dart:io' as io;

import 'package:engine_build_configs/engine_build_configs.dart';
import 'package:engine_repo_tools/engine_repo_tools.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:platform/platform.dart';
import 'package:process_fakes/process_fakes.dart';
import 'package:process_runner/process_runner.dart';

import 'fixtures.dart' as fixtures;

void main() {
final Engine engine;
try {
engine = Engine.findWithin();
} catch (e) {
io.stderr.writeln(e);
io.exitCode = 1;
return;
}

final BuildConfig linuxTestConfig = BuildConfig.fromJson(
path: 'ci/builders/linux_test_config.json',
map: convert.jsonDecode(fixtures.testConfig('Linux'))
as Map<String, Object?>,
);

final BuildConfig macTestConfig = BuildConfig.fromJson(
path: 'ci/builders/mac_test_config.json',
map: convert.jsonDecode(fixtures.testConfig('Mac-12'))
as Map<String, Object?>,
);

final BuildConfig winTestConfig = BuildConfig.fromJson(
path: 'ci/builders/win_test_config.json',
map: convert.jsonDecode(fixtures.testConfig('Windows-11'))
as Map<String, Object?>,
);

final Map<String, BuildConfig> configs = <String, BuildConfig>{
'linux_test_config': linuxTestConfig,
'linux_test_config2': linuxTestConfig,
'mac_test_config': macTestConfig,
'win_test_config': winTestConfig,
};

(Environment, List<List<String>>) linuxEnv(Logger logger) {
final List<List<String>> runHistory = <List<String>>[];
return (
Environment(
abi: ffi.Abi.linuxX64,
engine: engine,
platform: FakePlatform(operatingSystem: Platform.linux),
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, '', '');
}),
),
logger: logger,
),
runHistory
);
}

test('can find host runnable build', () async {
final Logger logger = Logger.test();
final (Environment env, _) = linuxEnv(logger);
final List<GlobalBuild> result = runnableBuilds(env, configs);
expect(result.length, equals(2));
expect(result[0].name, equals('build_name'));
});

test('build command invokes gn', () 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',
'build_name',
]);
expect(result, equals(0));
expect(runHistory.length, greaterThanOrEqualTo(1));
expect(runHistory[0].length, greaterThanOrEqualTo(1));
expect(runHistory[0][0], contains('gn'));
});

test('build command invokes ninja', () 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',
'build_name',
]);
expect(result, equals(0));
expect(runHistory.length, greaterThanOrEqualTo(2));
expect(runHistory[1].length, greaterThanOrEqualTo(1));
expect(runHistory[1][0], contains('ninja'));
});
}
40 changes: 24 additions & 16 deletions tools/engine_tool/test/query_command_test.dart
Original file line number Diff line number Diff line change
@@ -9,7 +9,6 @@ import 'dart:io' as io;
import 'package:engine_build_configs/engine_build_configs.dart';
import 'package:engine_repo_tools/engine_repo_tools.dart';
import 'package:engine_tool/src/commands/command_runner.dart';
import 'package:engine_tool/src/commands/flags.dart';
import 'package:engine_tool/src/environment.dart';
import 'package:engine_tool/src/logger.dart';
import 'package:litetest/litetest.dart';
@@ -32,17 +31,20 @@ void main() {

final BuildConfig linuxTestConfig = BuildConfig.fromJson(
path: 'ci/builders/linux_test_config.json',
map: convert.jsonDecode(fixtures.testConfig('Linux')) as Map<String, Object?>,
map: convert.jsonDecode(fixtures.testConfig('Linux'))
as Map<String, Object?>,
);

final BuildConfig macTestConfig = BuildConfig.fromJson(
path: 'ci/builders/mac_test_config.json',
map: convert.jsonDecode(fixtures.testConfig('Mac-12')) as Map<String, Object?>,
map: convert.jsonDecode(fixtures.testConfig('Mac-12'))
as Map<String, Object?>,
);

final BuildConfig winTestConfig = BuildConfig.fromJson(
path: 'ci/builders/win_test_config.json',
map: convert.jsonDecode(fixtures.testConfig('Windows-11')) as Map<String, Object?>,
map: convert.jsonDecode(fixtures.testConfig('Windows-11'))
as Map<String, Object?>,
);

final Map<String, BuildConfig> configs = <String, BuildConfig>{
@@ -76,7 +78,8 @@ void main() {
configs: configs,
);
final int result = await runner.run(<String>[
'query', 'builds',
'query',
'builders',
]);
expect(result, equals(0));
expect(
@@ -92,26 +95,29 @@ void main() {
);
});

test('query command with --builder returns only from the named builder.', () async {
test('query command with --builder returns only from the named builder.',
() async {
final Logger logger = Logger.test();
final Environment env = linuxEnv(logger);
final ToolCommandRunner runner = ToolCommandRunner(
environment: env,
configs: configs,
);
final int result = await runner.run(<String>[
'query', 'builds', '--$builderFlag', 'linux_test_config',
'query',
'builders',
'--builder',
'linux_test_config',
]);
expect(result, equals(0));
expect(
stringsFromLogs(logger.testLogs),
equals(<String>[
'Add --verbose to see detailed information about each builder\n',
'\n',
'"linux_test_config" builder:\n',
' "build_name" config\n',
]),
);
stringsFromLogs(logger.testLogs),
equals(<String>[
'Add --verbose to see detailed information about each builder\n',
'\n',
'"linux_test_config" builder:\n',
' "build_name" config\n',
]));
});

test('query command with --all returns all builds.', () async {
@@ -122,7 +128,9 @@ void main() {
configs: configs,
);
final int result = await runner.run(<String>[
'query', 'builds', '--$allFlag',
'query',
'builders',
'--all',
]);
expect(result, equals(0));
expect(