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.

Sign up for GitHub

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

Jump to bottom

[et] Lookup output filesystem path, not label #52248

Merged
merged 1 commit into from
Apr 22, 2024
Merged

[et] Lookup output filesystem path, not label #52248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
76 changes: 52 additions & 24 deletions tools/engine_tool/lib/src/gn_utils.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,30 +77,9 @@ final class BuildTarget {
Future<Map<String, BuildTarget>> findTargets(
Environment environment, Directory buildDir) async {
final Map<String, BuildTarget> r = <String, BuildTarget>{};
final List<String> getBuildInfoCommandLine = <String>[
gnBinPath(environment),
'desc',
buildDir.path,
'*',
'--format=json',
];

final ProcessRunnerResult result = await environment.processRunner.runProcess(
getBuildInfoCommandLine,
workingDirectory: environment.engine.srcDir,
failOk: true);

// Handle any process failures.
fatalIfFailed(environment, getBuildInfoCommandLine, result);

late final Map<String, Object?> jsonResult;
try {
jsonResult = jsonDecode(result.stdout) as Map<String, Object?>;
} catch (e) {
environment.logger.fatal(
'gn desc output could not be parsed:\nE=$e\nIN=${result.stdout}\n');
}

final Map<String, Object?> jsonResult =
await _runGnDesc(buildDir.path, '*', environment);
for (final MapEntry<String, Object?> targetEntry in jsonResult.entries) {
final String label = targetEntry.key;
if (targetEntry.value == null) {
Expand All @@ -120,7 +99,7 @@ Future<Map<String, BuildTarget>> findTargets(
}
final bool testOnly = getBool(properties, 'testonly');
final List<String> outputs =
getListOfString(properties, 'outputs') ?? <String>[];
await _runGnOutputs(buildDir.path, label, environment);
File? executable;
if (type == BuildTargetType.executable) {
if (outputs.isEmpty) {
Expand All @@ -135,6 +114,55 @@ Future<Map<String, BuildTarget>> findTargets(
return r;
}

/// Returns the JSON output of running `gn desc buildDir label`.
Future<Map<String, Object?>> _runGnDesc(
String buildDir, String label, Environment environment) async {
final List<String> commandline = <String>[
gnBinPath(environment),
'desc',
buildDir,
label,
'--format=json',
];

final ProcessRunnerResult result = await environment.processRunner.runProcess(
commandline,
workingDirectory: environment.engine.srcDir,
failOk: true);

// Handle any process failures.
fatalIfFailed(environment, commandline, result);

late final Map<String, Object?> jsonResult;
try {
jsonResult = jsonDecode(result.stdout) as Map<String, Object?>;
} catch (e) {
environment.logger.fatal(
'gn desc output could not be parsed:\nE=$e\nIN=${result.stdout}\n');
}
return jsonResult;
}

/// Returns the output paths returned by `gn outputs buildDir label`.
Future<List<String>> _runGnOutputs(
String buildDir, String label, Environment environment) async {
final List<String> commandline = <String>[
gnBinPath(environment),
'outputs',
buildDir,
label,
];
final ProcessRunnerResult result = await environment.processRunner.runProcess(
commandline,
workingDirectory: environment.engine.srcDir,
failOk: true);

// Handle any process failures.
fatalIfFailed(environment, commandline, result);

return result.stdout.split('\n');
}

/// Process selectors and filter allTargets for matches.
///
/// We support:
Expand Down
20 changes: 10 additions & 10 deletions tools/engine_tool/test/build_command_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,11 +360,11 @@ void main() {
'//flutter/fml:fml_arc_unittests',
]);
expect(result, equals(0));
expect(testEnv.processHistory.length, greaterThanOrEqualTo(2));
expect(testEnv.processHistory[3].command[0], contains('ninja'));
expect(testEnv.processHistory[3].command[2], endsWith('/host_debug'));
expect(testEnv.processHistory.length, greaterThan(6));
Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to a strictly greater-than check so that it matches the processHistory index we refer to, which will make it a little more obvious to people who modify this in the future.

expect(testEnv.processHistory[6].command[0], contains('ninja'));
expect(testEnv.processHistory[6].command[2], endsWith('/host_debug'));
expect(
testEnv.processHistory[3].command[5],
testEnv.processHistory[6].command[5],
equals('flutter/fml:fml_arc_unittests'),
);
} finally {
Expand All @@ -389,19 +389,19 @@ void main() {
'//flutter/...',
]);
expect(result, equals(0));
expect(testEnv.processHistory.length, greaterThanOrEqualTo(2));
expect(testEnv.processHistory[3].command[0], contains('ninja'));
expect(testEnv.processHistory[3].command[2], endsWith('/host_debug'));
expect(testEnv.processHistory.length, greaterThan(6));
expect(testEnv.processHistory[6].command[0], contains('ninja'));
expect(testEnv.processHistory[6].command[2], endsWith('/host_debug'));
expect(
testEnv.processHistory[3].command[5],
testEnv.processHistory[6].command[5],
equals('flutter/display_list:display_list_unittests'),
);
expect(
testEnv.processHistory[3].command[6],
testEnv.processHistory[6].command[6],
equals('flutter/flow:flow_unittests'),
);
expect(
testEnv.processHistory[3].command[7],
testEnv.processHistory[6].command[7],
equals('flutter/fml:fml_arc_unittests'),
);
} finally {
Expand Down
2 changes: 2 additions & 0 deletions tools/engine_tool/test/gn_utils_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ void main() {
final List<CannedProcess> cannedProcesses = <CannedProcess>[
CannedProcess((List<String> command) => command.contains('desc'),
stdout: fixtures.gnDescOutput()),
CannedProcess((List<String> command) => command.contains('outputs'),
stdout: 'display_list_unittests'),
];

test('find test targets', () async {
Expand Down
4 changes: 3 additions & 1 deletion tools/engine_tool/test/test_command_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ void main() {
final List<CannedProcess> cannedProcesses = <CannedProcess>[
CannedProcess((List<String> command) => command.contains('desc'),
stdout: fixtures.gnDescOutput()),
CannedProcess((List<String> command) => command.contains('outputs'),
stdout: 'display_list_unittests'),
];

test('test command executes test', () async {
Expand Down Expand Up @@ -83,7 +85,7 @@ void main() {
'//third_party/protobuf:protoc',
]);
expect(result, equals(1));
expect(testEnvironment.processHistory.length, lessThan(3));
expect(testEnvironment.processHistory.length, lessThan(6));
Copy link
Member Author

Choose a reason for hiding this comment

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

Here and in the test above it: This expectation is impacted by implementation details of the test command. I don't think the number of processes executed is the right test, and instead we should just test that test processes are executed and that non-test processes are executed.

I can send out a followup that fixes that. (I realise I also just added this test in a previous patch so I'm 100% to blame for this one :P)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think this count is too brittle. Better to have something like:

expect(testEnvironment.processHistory, executed('gn desc'));

expect(testEnvironment.processHistory.where((ExecutedProcess process) {
return process.command[0].contains('protoc');
}), isEmpty);
Expand Down