From dcd061c70bbe0fa9320ec833872afc5dcf841b3c Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Wed, 3 Feb 2021 10:56:22 -0800 Subject: [PATCH] [flutter_tools] experiment with skipping process resolution (#74447) --- .../lib/src/base/error_handling_io.dart | 166 +++++++++++++++--- packages/flutter_tools/lib/src/dart/pub.dart | 15 +- .../base/error_handling_io_test.dart | 20 +++ 3 files changed, 170 insertions(+), 31 deletions(-) diff --git a/packages/flutter_tools/lib/src/base/error_handling_io.dart b/packages/flutter_tools/lib/src/base/error_handling_io.dart index ced497c0bed..2298cf66260 100644 --- a/packages/flutter_tools/lib/src/base/error_handling_io.dart +++ b/packages/flutter_tools/lib/src/base/error_handling_io.dart @@ -575,6 +575,70 @@ T _runSync(T Function() op, { } } +class _ProcessDelegate { + const _ProcessDelegate(); + + Future start( + List command, { + String workingDirectory, + Map environment, + bool includeParentEnvironment = true, + bool runInShell = false, + io.ProcessStartMode mode = io.ProcessStartMode.normal, + }) { + return io.Process.start( + command[0], + command.skip(1).toList(), + workingDirectory: workingDirectory, + environment: environment, + includeParentEnvironment: includeParentEnvironment, + runInShell: runInShell, + ); + } + + Future run( + List command, { + String workingDirectory, + Map environment, + bool includeParentEnvironment = true, + bool runInShell = false, + Encoding stdoutEncoding = io.systemEncoding, + Encoding stderrEncoding = io.systemEncoding, + }) { + return io.Process.run( + command[0], + command.skip(1).toList(), + workingDirectory: workingDirectory, + environment: environment, + includeParentEnvironment: includeParentEnvironment, + runInShell: runInShell, + stdoutEncoding: stdoutEncoding, + stderrEncoding: stderrEncoding, + ); + } + + io.ProcessResult runSync( + List command, { + String workingDirectory, + Map environment, + bool includeParentEnvironment = true, + bool runInShell = false, + Encoding stdoutEncoding = io.systemEncoding, + Encoding stderrEncoding = io.systemEncoding, + }) { + return io.Process.runSync( + command[0], + command.skip(1).toList(), + workingDirectory: workingDirectory, + environment: environment, + includeParentEnvironment: includeParentEnvironment, + runInShell: runInShell, + stdoutEncoding: stdoutEncoding, + stderrEncoding: stderrEncoding, + ); + } +} + /// A [ProcessManager] that throws a [ToolExit] on certain errors. /// /// If a [ProcessException] is not caused by the Flutter tool, and can only be @@ -592,6 +656,21 @@ class ErrorHandlingProcessManager extends ProcessManager { final ProcessManager _delegate; final Platform _platform; + static const _ProcessDelegate _processDelegate = _ProcessDelegate(); + static bool _skipCommandLookup = false; + + /// Bypass package:process command lookup for all functions in this block. + /// + /// This required that the fully resolved executable path is provided. + static Future skipCommandLookup(Future Function() operation) async { + final bool previousValue = ErrorHandlingProcessManager._skipCommandLookup; + try { + ErrorHandlingProcessManager._skipCommandLookup = true; + return await operation(); + } finally { + ErrorHandlingProcessManager._skipCommandLookup = previousValue; + } + } @override bool canRun(dynamic executable, {String workingDirectory}) { @@ -619,15 +698,28 @@ class ErrorHandlingProcessManager extends ProcessManager { Encoding stdoutEncoding = io.systemEncoding, Encoding stderrEncoding = io.systemEncoding, }) { - return _run(() => _delegate.run( - command, - workingDirectory: workingDirectory, - environment: environment, - includeParentEnvironment: includeParentEnvironment, - runInShell: runInShell, - stdoutEncoding: stdoutEncoding, - stderrEncoding: stderrEncoding, - ), platform: _platform); + return _run(() { + if (_skipCommandLookup && _delegate is LocalProcessManager) { + return _processDelegate.run( + command.cast(), + workingDirectory: workingDirectory, + environment: environment, + includeParentEnvironment: includeParentEnvironment, + runInShell: runInShell, + stdoutEncoding: stdoutEncoding, + stderrEncoding: stderrEncoding, + ); + } + return _delegate.run( + command, + workingDirectory: workingDirectory, + environment: environment, + includeParentEnvironment: includeParentEnvironment, + runInShell: runInShell, + stdoutEncoding: stdoutEncoding, + stderrEncoding: stderrEncoding, + ); + }, platform: _platform); } @override @@ -639,13 +731,24 @@ class ErrorHandlingProcessManager extends ProcessManager { bool runInShell = false, io.ProcessStartMode mode = io.ProcessStartMode.normal, }) { - return _run(() => _delegate.start( - command, - workingDirectory: workingDirectory, - environment: environment, - includeParentEnvironment: includeParentEnvironment, - runInShell: runInShell, - ), platform: _platform); + return _run(() { + if (_skipCommandLookup && _delegate is LocalProcessManager) { + return _processDelegate.start( + command.cast(), + workingDirectory: workingDirectory, + environment: environment, + includeParentEnvironment: includeParentEnvironment, + runInShell: runInShell, + ); + } + return _delegate.start( + command, + workingDirectory: workingDirectory, + environment: environment, + includeParentEnvironment: includeParentEnvironment, + runInShell: runInShell, + ); + }, platform: _platform); } @override @@ -658,15 +761,28 @@ class ErrorHandlingProcessManager extends ProcessManager { Encoding stdoutEncoding = io.systemEncoding, Encoding stderrEncoding = io.systemEncoding, }) { - return _runSync(() => _delegate.runSync( - command, - workingDirectory: workingDirectory, - environment: environment, - includeParentEnvironment: includeParentEnvironment, - runInShell: runInShell, - stdoutEncoding: stdoutEncoding, - stderrEncoding: stderrEncoding, - ), platform: _platform); + return _runSync(() { + if (_skipCommandLookup && _delegate is LocalProcessManager) { + return _processDelegate.runSync( + command.cast(), + workingDirectory: workingDirectory, + environment: environment, + includeParentEnvironment: includeParentEnvironment, + runInShell: runInShell, + stdoutEncoding: stdoutEncoding, + stderrEncoding: stderrEncoding, + ); + } + return _delegate.runSync( + command, + workingDirectory: workingDirectory, + environment: environment, + includeParentEnvironment: includeParentEnvironment, + runInShell: runInShell, + stdoutEncoding: stdoutEncoding, + stderrEncoding: stderrEncoding, + ); + }, platform: _platform); } } diff --git a/packages/flutter_tools/lib/src/dart/pub.dart b/packages/flutter_tools/lib/src/dart/pub.dart index 8828c278ac5..535a08b41fe 100644 --- a/packages/flutter_tools/lib/src/dart/pub.dart +++ b/packages/flutter_tools/lib/src/dart/pub.dart @@ -4,7 +4,6 @@ // @dart = 2.8 - import 'package:meta/meta.dart'; import 'package:package_config/package_config.dart'; import 'package:process/process.dart'; @@ -12,6 +11,7 @@ import 'package:process/process.dart'; import '../base/bot_detector.dart'; import '../base/common.dart'; import '../base/context.dart'; +import '../base/error_handling_io.dart'; import '../base/file_system.dart'; import '../base/io.dart' as io; import '../base/logger.dart'; @@ -333,11 +333,14 @@ class _DefaultPub implements Pub { bool touchesPackageConfig = false, bool generateSyntheticPackage = false, }) async { - final io.Process process = await _processUtils.start( - _pubCommand(arguments), - workingDirectory: directory, - environment: await _createPubEnvironment(PubContext.interactive), - ); + // Fully resolved pub or pub.bat is calculated based on current platform. + final io.Process process = await ErrorHandlingProcessManager.skipCommandLookup(() async { + return _processUtils.start( + _pubCommand(arguments), + workingDirectory: directory, + environment: await _createPubEnvironment(PubContext.interactive), + ); + }); // Pipe the Flutter tool stdin to the pub stdin. unawaited(process.stdin.addStream(stdio.stdin) diff --git a/packages/flutter_tools/test/general.shard/base/error_handling_io_test.dart b/packages/flutter_tools/test/general.shard/base/error_handling_io_test.dart index 258f32ee448..1d705cf73e9 100644 --- a/packages/flutter_tools/test/general.shard/base/error_handling_io_test.dart +++ b/packages/flutter_tools/test/general.shard/base/error_handling_io_test.dart @@ -15,6 +15,7 @@ import 'package:flutter_tools/src/globals.dart' as globals show flutterUsage; import 'package:flutter_tools/src/reporting/reporting.dart'; import 'package:mockito/mockito.dart'; import 'package:path/path.dart' as path; // ignore: package_path_import +import 'package:process/process.dart'; import '../../src/common.dart'; import '../../src/context.dart'; @@ -669,6 +670,25 @@ void main() { }); }); + test('skipCommandLookup invokes Process calls directly', () async { + final ErrorHandlingProcessManager processManager = ErrorHandlingProcessManager( + delegate: const LocalProcessManager(), + platform: windowsPlatform, + ); + + // Throws an argument error because package:process fails to locate the executable. + expect(() => processManager.runSync(['foo']), throwsArgumentError); + expect(() => processManager.run(['foo']), throwsArgumentError); + expect(() => processManager.start(['foo']), throwsArgumentError); + + // Throws process exception because the executable does not exist. + await ErrorHandlingProcessManager.skipCommandLookup(() async { + expect(() => processManager.runSync(['foo']), throwsA(isA())); + expect(() => processManager.run(['foo']), throwsA(isA())); + expect(() => processManager.start(['foo']), throwsA(isA())); + }); + }); + group('ProcessManager on windows throws tool exit', () { const int kDeviceFull = 112; const int kUserMappedSectionOpened = 1224;