Skip to content

Commit 9cd2fc9

Browse files
authored
Handle ProcessExceptions due to git missing on the host (flutter#154445)
Fixes flutter#133585. This PR elects to add a new catch within `_handleToolError` that checks any uncaught error. This new catch will exit the tool without crashing provided the following conditions are met: 1. the error is a `ProcessException`, 2. the error message contains `git` somewhere in the message (we don't match on the entire string in case it changes or is locale-dependent), and 3. `git` does not appear to be runnable on the host system (`ProcessManager.canRun` returns `false` for git). This is preferable to checking for runnability of `git` before we run it for 1) its simplicity and 2) lack of performance penalty for users that already have git installed (almost every single one). This PR also does some light refactoring to runner_test.dart to make room for tests that aren't related to crash reporting.
1 parent 8435e12 commit 9cd2fc9

File tree

3 files changed

+142
-12
lines changed

3 files changed

+142
-12
lines changed

packages/flutter_tools/lib/runner.dart

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import 'dart:async';
77
import 'package:args/command_runner.dart';
88
import 'package:intl/intl.dart' as intl;
99
import 'package:intl/intl_standalone.dart' as intl_standalone;
10+
import 'package:process/process.dart';
1011
import 'package:unified_analytics/unified_analytics.dart';
1112

1213
import 'src/base/async_guard.dart';
@@ -185,6 +186,16 @@ Future<int> _handleToolError(
185186
} else {
186187
return exitWithHooks(error.exitCode, shutdownHooks: shutdownHooks);
187188
}
189+
} else if (error is ProcessException &&
190+
_isErrorDueToGitMissing(error, globals.processManager, globals.logger)) {
191+
globals.printError('${error.message}\n');
192+
globals.printError(
193+
'An error was encountered when trying to run git.\n'
194+
"Please ensure git is installed and available in your system's search path. "
195+
'See https://docs.flutter.dev/get-started/install for instructions on '
196+
'installing git for your platform.',
197+
);
198+
return exitWithHooks(1, shutdownHooks: shutdownHooks);
188199
} else {
189200
// We've crashed; emit a log report.
190201
globals.stdio.stderrWrite('\n');
@@ -301,3 +312,22 @@ Future<File> _createLocalCrashReport(CrashDetails details) async {
301312

302313
return crashFile;
303314
}
315+
316+
bool _isErrorDueToGitMissing(
317+
ProcessException exception,
318+
ProcessManager processManager,
319+
Logger logger,
320+
) {
321+
if (!exception.message.contains('git')) {
322+
return false;
323+
}
324+
325+
try {
326+
return !processManager.canRun('git');
327+
} on Object catch (error) {
328+
logger.printTrace(
329+
'Unable to check whether git is runnable: $error\n'
330+
);
331+
return true;
332+
}
333+
}

packages/flutter_tools/lib/src/web/chrome.dart

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -578,7 +578,7 @@ Future<ChromeTab?> getChromeTabGuarded(
578578
void Function(Object error, StackTrace stackTrace)? onIoError,
579579
}) async {
580580
try {
581-
return asyncGuard(
581+
return await asyncGuard(
582582
() => chromeConnection.getTab(
583583
accept,
584584
retryFor: retryFor,

packages/flutter_tools/test/general.shard/runner/runner_test.dart

Lines changed: 111 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import 'package:flutter_tools/src/artifacts.dart';
1010
import 'package:flutter_tools/src/base/bot_detector.dart';
1111
import 'package:flutter_tools/src/base/file_system.dart';
1212
import 'package:flutter_tools/src/base/io.dart' as io;
13+
import 'package:flutter_tools/src/base/logger.dart';
1314
import 'package:flutter_tools/src/base/net.dart';
1415
import 'package:flutter_tools/src/base/platform.dart';
1516
import 'package:flutter_tools/src/base/process.dart';
@@ -19,6 +20,7 @@ import 'package:flutter_tools/src/globals.dart' as globals;
1920
import 'package:flutter_tools/src/reporting/crash_reporting.dart';
2021
import 'package:flutter_tools/src/reporting/reporting.dart';
2122
import 'package:flutter_tools/src/runner/flutter_command.dart';
23+
import 'package:test/fake.dart';
2224
import 'package:unified_analytics/unified_analytics.dart';
2325

2426
import '../../src/common.dart';
@@ -29,12 +31,10 @@ import '../../src/fakes.dart';
2931
const String kCustomBugInstructions = 'These are instructions to report with a custom bug tracker.';
3032

3133
void main() {
32-
int? firstExitCode;
33-
late MemoryFileSystem fileSystem;
34-
35-
group('runner', () {
34+
group('runner (crash reporting)', () {
35+
int? firstExitCode;
36+
late MemoryFileSystem fileSystem;
3637
late FakeAnalytics fakeAnalytics;
37-
late TestUsage testUsage;
3838

3939
setUp(() {
4040
// Instead of exiting with dart:io exit(), this causes an exception to
@@ -59,8 +59,6 @@ void main() {
5959
fs: fileSystem,
6060
fakeFlutterVersion: FakeFlutterVersion(),
6161
);
62-
63-
testUsage = TestUsage();
6462
});
6563

6664
tearDown(() {
@@ -327,10 +325,84 @@ void main() {
327325
HttpClientFactory: () => () => FakeHttpClient.any(),
328326
});
329327
});
328+
});
330329

331-
testUsingContext('do not print welcome on bots', () async {
332-
io.setExitFunctionForTests((int exitCode) {});
330+
group('runner', () {
331+
late MemoryFileSystem fs;
332+
333+
setUp(() {
334+
io.setExitFunctionForTests((int exitCode) {});
333335

336+
fs = MemoryFileSystem.test();
337+
338+
Cache.disableLocking();
339+
});
340+
341+
tearDown(() {
342+
io.restoreExitFunction();
343+
Cache.enableLocking();
344+
});
345+
346+
testUsingContext("catches ProcessException calling git because it's not available", () async {
347+
final _GitNotFoundFlutterCommand command = _GitNotFoundFlutterCommand();
348+
349+
await runner.run(
350+
<String>[command.name],
351+
() => <FlutterCommand>[
352+
command,
353+
],
354+
// This flutterVersion disables crash reporting.
355+
flutterVersion: '[user-branch]/',
356+
reportCrashes: false,
357+
shutdownHooks: ShutdownHooks(),
358+
);
359+
360+
expect(
361+
(globals.logger as BufferLogger).errorText,
362+
'Failed to find "git" in the search path.\n'
363+
'\n'
364+
'An error was encountered when trying to run git.\n'
365+
"Please ensure git is installed and available in your system's search path. "
366+
'See https://docs.flutter.dev/get-started/install for instructions on installing git for your platform.\n');
367+
},
368+
overrides: <Type, Generator>{
369+
FileSystem: () => fs,
370+
Artifacts: () => Artifacts.test(),
371+
ProcessManager: () =>
372+
FakeProcessManager.any()..excludedExecutables.add('git'),
373+
},
374+
);
375+
376+
testUsingContext('handles ProcessException calling git when ProcessManager.canRun fails', () async {
377+
final _GitNotFoundFlutterCommand command = _GitNotFoundFlutterCommand();
378+
379+
await runner.run(
380+
<String>[command.name],
381+
() => <FlutterCommand>[
382+
command,
383+
],
384+
// This flutterVersion disables crash reporting.
385+
flutterVersion: '[user-branch]/',
386+
reportCrashes: false,
387+
shutdownHooks: ShutdownHooks(),
388+
);
389+
390+
expect(
391+
(globals.logger as BufferLogger).errorText,
392+
'Failed to find "git" in the search path.\n'
393+
'\n'
394+
'An error was encountered when trying to run git.\n'
395+
"Please ensure git is installed and available in your system's search path. "
396+
'See https://docs.flutter.dev/get-started/install for instructions on installing git for your platform.\n');
397+
},
398+
overrides: <Type, Generator>{
399+
FileSystem: () => fs,
400+
Artifacts: () => Artifacts.test(),
401+
ProcessManager: () => _ErrorOnCanRunFakeProcessManager(),
402+
},
403+
);
404+
405+
testUsingContext('do not print welcome on bots', () async {
334406
await runner.run(
335407
<String>['--version', '--machine'],
336408
() => <FlutterCommand>[],
@@ -339,13 +411,13 @@ void main() {
339411
shutdownHooks: ShutdownHooks(),
340412
);
341413

342-
expect(testUsage.printedWelcome, false);
414+
expect((globals.flutterUsage as TestUsage).printedWelcome, false);
343415
},
344416
overrides: <Type, Generator>{
345417
FileSystem: () => MemoryFileSystem.test(),
346418
ProcessManager: () => FakeProcessManager.any(),
347419
BotDetector: () => const FakeBotDetector(true),
348-
Usage: () => testUsage,
420+
Usage: () => TestUsage(),
349421
},
350422
);
351423
});
@@ -570,6 +642,23 @@ class CrashingFlutterCommand extends FlutterCommand {
570642
}
571643
}
572644

645+
class _GitNotFoundFlutterCommand extends FlutterCommand {
646+
@override
647+
String get description => '';
648+
649+
@override
650+
String get name => 'git-not-found';
651+
652+
@override
653+
Future<FlutterCommandResult> runCommand() {
654+
throw const io.ProcessException(
655+
'git',
656+
<String>['log'],
657+
'Failed to find "git" in the search path.',
658+
);
659+
}
660+
}
661+
573662
class CrashingUsage implements Usage {
574663
CrashingUsage() : _impl = Usage(
575664
versionOverride: '[user-branch]',
@@ -670,3 +759,14 @@ class WaitingCrashReporter implements CrashReporter {
670759
return _future;
671760
}
672761
}
762+
763+
class _ErrorOnCanRunFakeProcessManager extends Fake implements FakeProcessManager {
764+
final FakeProcessManager delegate = FakeProcessManager.any();
765+
@override
766+
bool canRun(dynamic executable, {String? workingDirectory}) {
767+
if (executable == 'git') {
768+
throw Exception("oh no, we couldn't check for git!");
769+
}
770+
return delegate.canRun(executable, workingDirectory: workingDirectory);
771+
}
772+
}

0 commit comments

Comments
 (0)