Skip to content

Commit ecb7c78

Browse files
authored
Recognizegroup, and warn when we silently discard targets. (flutter/engine#55791)
Closes flutter#156260. Fixes the degenerate case where you specify an unrecognized target, and it falls back to rebuilding the entire engine as if you specified nothing. In addition, added recognition of `group`.
1 parent ce933da commit ecb7c78

File tree

5 files changed

+167
-1
lines changed

5 files changed

+167
-1
lines changed

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,20 @@ et build //flutter/fml:fml_benchmarks # Build a specific target in `//flutter/f
7272
allTargets.addAll(targets.map((target) => target.label));
7373
}
7474

75+
// Warn that we've discarded some targets.
76+
// Other warnings should have been emitted above, so if this ends up being
77+
// unneccesarily noisy, we can remove it or limit it to verbose mode.
78+
if (allTargets.length < commandLineTargets.length) {
79+
// Report which targets were not found.
80+
final notFound = commandLineTargets.where(
81+
(target) => !allTargets.contains(Label.parse(target)),
82+
);
83+
environment.logger.warning(
84+
'One or more targets specified did not match any build targets:\n\n'
85+
'${notFound.join('\n')}',
86+
);
87+
}
88+
7589
return runBuild(
7690
environment,
7791
plan.build,

engine/src/flutter/tools/engine_tool/lib/src/gn.dart

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,8 @@
1+
// Copyright 2013 The Flutter Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
import 'package:collection/collection.dart';
16
import 'package:meta/meta.dart';
27
import 'package:path/path.dart' as p;
38

@@ -106,6 +111,9 @@ interface class Gn {
106111
JsonObject(properties),
107112
);
108113
if (target == null) {
114+
_environment.logger.warning(
115+
'Unknown target type for $label: type=${properties['type']}',
116+
);
109117
return null;
110118
}
111119
return target;
@@ -180,6 +188,11 @@ sealed class BuildTarget {
180188
testOnly: testOnly,
181189
json: json,
182190
),
191+
'group' => GroupBuildTarget(
192+
label: Label.parseGn(label),
193+
testOnly: testOnly,
194+
deps: json.stringList('deps').map(Label.parseGn).toList(),
195+
),
183196
_ => null,
184197
};
185198
}
@@ -281,3 +294,36 @@ final class ExecutableBuildTarget extends BuildTarget {
281294
String toString() =>
282295
'ExecutableBuildTarget($label, testOnly=$testOnly, executable=$executable)';
283296
}
297+
298+
/// A build target that [group]s a meta-target of other build targets.
299+
///
300+
/// [group]: https://gn.googlesource.com/gn/+/main/docs/reference.md#func_group
301+
final class GroupBuildTarget extends BuildTarget {
302+
/// Construct a group build target.
303+
const GroupBuildTarget({
304+
required super.label,
305+
required super.testOnly,
306+
required this.deps,
307+
});
308+
309+
/// The list of dependencies for this group target.
310+
final List<Label> deps;
311+
312+
@override
313+
bool operator ==(Object other) {
314+
if (other is! GroupBuildTarget) {
315+
return false;
316+
}
317+
if (label != other.label || testOnly != other.testOnly) {
318+
return false;
319+
}
320+
return const ListEquality<Object>().equals(deps, other.deps);
321+
}
322+
323+
@override
324+
int get hashCode => Object.hash(label, testOnly, Object.hashAll(deps));
325+
326+
@override
327+
String toString() =>
328+
'GroupBuildTarget($label, testOnly=$testOnly, deps=$deps)';
329+
}

engine/src/flutter/tools/engine_tool/test/commands/build_command_test.dart

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import 'package:engine_tool/src/logger.dart';
1111
import 'package:path/path.dart' as path;
1212
import 'package:test/test.dart';
1313

14+
import '../src/matchers.dart';
1415
import '../src/test_build_configs.dart';
1516
import '../src/utils.dart';
1617

@@ -659,6 +660,77 @@ The input testing/scenario_app:sceario_app matches no targets, configs or files.
659660
);
660661
});
661662

663+
test('build command warns on an unrecognized action', () async {
664+
final testEnv = TestEnvironment.withTestEngine(
665+
abi: Abi.macosArm64,
666+
cannedProcesses: [
667+
CannedProcess(
668+
(command) => command.contains('desc'),
669+
stdout: convert.jsonEncode({
670+
'//flutter/tools/unrecognized:action': {
671+
'outputs': [
672+
'//out/host_debug/unrecognized_action',
673+
],
674+
'testonly': true,
675+
'type': 'unrecognized',
676+
},
677+
}),
678+
),
679+
],
680+
);
681+
addTearDown(testEnv.cleanup);
682+
683+
final builder = TestBuilderConfig();
684+
builder.addBuild(
685+
name: 'ci/host_debug',
686+
targetDir: 'host_debug',
687+
dimension: TestDroneDimension.mac,
688+
);
689+
final runner = ToolCommandRunner(
690+
environment: testEnv.environment,
691+
configs: {
692+
'mac_test_config': builder.buildConfig(
693+
path: 'ci/builders/mac_test_config.json',
694+
),
695+
},
696+
);
697+
698+
final result = await runner.run([
699+
'build',
700+
'--config',
701+
'ci/host_debug',
702+
'//flutter/tools/unrecognized:action',
703+
]);
704+
705+
printOnFailure(testEnv.testLogs.map((r) => r.message).join('\n'));
706+
expect(result, equals(0));
707+
708+
expect(
709+
testEnv.testLogs,
710+
contains(
711+
logRecord(
712+
stringContainsInOrder([
713+
'Unknown target type',
714+
'//flutter/tools/unrecognized:action',
715+
'type=unrecognized',
716+
]),
717+
level: Logger.warningLevel,
718+
),
719+
),
720+
);
721+
722+
expect(
723+
testEnv.testLogs,
724+
contains(logRecord(
725+
stringContainsInOrder([
726+
'One or more targets specified did not match',
727+
'//flutter/tools/unrecognized:action',
728+
]),
729+
level: Logger.warningLevel,
730+
)),
731+
);
732+
});
733+
662734
test('et help build line length is not too big', () async {
663735
final testEnv = TestEnvironment.withTestEngine(
664736
verbose: true,

engine/src/flutter/tools/engine_tool/test/run_command_test.dart renamed to engine/src/flutter/tools/engine_tool/test/commands/run_command_test.dart

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ import 'package:process_fakes/process_fakes.dart';
1919
import 'package:process_runner/process_runner.dart';
2020
import 'package:test/test.dart';
2121

22-
import 'src/test_build_configs.dart';
22+
import '../src/test_build_configs.dart';
2323

2424
void main() {
2525
late io.Directory tempRoot;

engine/src/flutter/tools/engine_tool/test/external_tools/gn_test.dart

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,40 @@ void main() {
127127
);
128128
});
129129

130+
test('parses a group', () async {
131+
final testEnv = TestEnvironment.withTestEngine(
132+
cannedProcesses: [
133+
CannedProcess(
134+
(List<String> command) => command.contains('desc'),
135+
stdout: '''
136+
{
137+
"//foo/bar:baz_group": {
138+
"deps": ["//foo/bar:baz_shared_library"],
139+
"testonly": true,
140+
"type": "group"
141+
}
142+
}
143+
''',
144+
),
145+
],
146+
);
147+
addTearDown(testEnv.cleanup);
148+
149+
final gn = Gn.fromEnvironment(testEnv.environment);
150+
final targets = await gn.desc('out/Release', TargetPattern('//foo', 'bar'));
151+
expect(targets, hasLength(1));
152+
153+
final groupTarget = targets.single;
154+
expect(
155+
groupTarget,
156+
GroupBuildTarget(
157+
label: Label('//foo/bar', 'baz_group'),
158+
testOnly: true,
159+
deps: [Label('//foo/bar', 'baz_shared_library')],
160+
),
161+
);
162+
});
163+
130164
test('parses a dart_test action as an executable', () async {
131165
final testEnv = TestEnvironment.withTestEngine(
132166
cannedProcesses: [

0 commit comments

Comments
 (0)