Removes retries from "dart pub get" and un-buffers its stdout/stderr …

…output (flutter#115801) * Removes retries from "pub get" and proxies its stdout output * Fix issue where ErrorHandlingProcessManager does not forward "mode" parameter to backing ProcessManager's "start" method * Make "pub get" use ProcessStartMode.inheritStdio instead of forwarding bytes to stdout and stderr * Fix tests * Remove unused env var * Add back 'Running "flutter pub get"...' status log * Fix indent * Add Pub.test() constructor which lets tests mock stdio
mit-mit · Dec 6, 2022 · 439aef6 · 439aef6
1 parent 81c8465
commit 439aef6
Show file tree

Hide file tree

Showing 10 changed files with 235 additions and 447 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
@@ -699,6 +699,7 @@ class ErrorHandlingProcessManager extends ProcessManager {
         environment: environment,
         includeParentEnvironment: includeParentEnvironment,
         runInShell: runInShell,
+        mode: mode,
       );
     }, platform: _platform);
   }

diff --git a/packages/flutter_tools/lib/src/base/io.dart b/packages/flutter_tools/lib/src/base/io.dart
@@ -267,7 +267,6 @@ class Stdio {
   }
   io.Stdout? _stdout;
 
-  @visibleForTesting
   io.IOSink get stderr {
     if (_stderr != null) {
       return _stderr!;

diff --git a/packages/flutter_tools/lib/src/base/process.dart b/packages/flutter_tools/lib/src/base/process.dart
@@ -197,6 +197,7 @@ abstract class ProcessUtils {
     String? workingDirectory,
     bool allowReentrantFlutter = false,
     Map<String, String>? environment,
+    ProcessStartMode mode = ProcessStartMode.normal,
   });
 
   /// This runs the command and streams stdout/stderr from the child process to
@@ -422,12 +423,14 @@ class _DefaultProcessUtils implements ProcessUtils {
     String? workingDirectory,
     bool allowReentrantFlutter = false,
     Map<String, String>? environment,
+    ProcessStartMode mode = ProcessStartMode.normal,
   }) {
     _traceCommand(cmd, workingDirectory: workingDirectory);
     return _processManager.start(
       cmd,
       workingDirectory: workingDirectory,
       environment: _environment(allowReentrantFlutter, environment),
+      mode: mode,
     );
   }
 

diff --git a/packages/flutter_tools/lib/src/context_runner.dart b/packages/flutter_tools/lib/src/context_runner.dart
@@ -311,7 +311,6 @@ Future<T> runInContext<T>(
         botDetector: globals.botDetector,
         platform: globals.platform,
         usage: globals.flutterUsage,
-        stdio: globals.stdio,
       ),
       Stdio: () => Stdio(),
       SystemClock: () => const SystemClock(),

diff --git a/packages/flutter_tools/lib/src/dart/pub.dart b/packages/flutter_tools/lib/src/dart/pub.dart
@@ -4,6 +4,7 @@
 
 import 'dart:async';
 
+import 'package:meta/meta.dart';
 import 'package:package_config/package_config.dart';
 import 'package:process/process.dart';
 
@@ -31,10 +32,6 @@ const String _kPubEnvironmentKey = 'PUB_ENVIRONMENT';
 /// The console environment key used by the pub tool to find the cache directory.
 const String _kPubCacheEnvironmentKey = 'PUB_CACHE';
 
-/// The UNAVAILABLE exit code returned by the pub tool.
-/// (see https://github.com/dart-lang/pub/blob/master/lib/src/exit_codes.dart)
-const int _kPubExitCodeUnavailable = 69;
-
 typedef MessageFilter = String? Function(String message);
 
 /// globalCachePath is the directory in which the content of the localCachePath will be moved in
@@ -150,9 +147,20 @@ abstract class Pub {
     required Platform platform,
     required BotDetector botDetector,
     required Usage usage,
-    required Stdio stdio,
   }) = _DefaultPub;
 
+  /// Create a [Pub] instance with a mocked [stdio].
+  @visibleForTesting
+  factory Pub.test({
+    required FileSystem fileSystem,
+    required Logger logger,
+    required ProcessManager processManager,
+    required Platform platform,
+    required BotDetector botDetector,
+    required Usage usage,
+    required Stdio stdio,
+  }) = _DefaultPub.test;
+
   /// Runs `pub get` or `pub upgrade` for [project].
   ///
   /// [context] provides extra information to package server requests to
@@ -214,6 +222,26 @@ class _DefaultPub implements Pub {
     required Platform platform,
     required BotDetector botDetector,
     required Usage usage,
+  }) : _fileSystem = fileSystem,
+       _logger = logger,
+       _platform = platform,
+       _botDetector = botDetector,
+       _usage = usage,
+       _processUtils = ProcessUtils(
+         logger: logger,
+         processManager: processManager,
+       ),
+       _processManager = processManager,
+       _stdio = null;
+
+  @visibleForTesting
+  _DefaultPub.test({
+    required FileSystem fileSystem,
+    required Logger logger,
+    required ProcessManager processManager,
+    required Platform platform,
+    required BotDetector botDetector,
+    required Usage usage,
     required Stdio stdio,
   }) : _fileSystem = fileSystem,
        _logger = logger,
@@ -234,7 +262,7 @@ class _DefaultPub implements Pub {
   final BotDetector _botDetector;
   final Usage _usage;
   final ProcessManager _processManager;
-  final Stdio _stdio;
+  final Stdio? _stdio;
 
   @override
   Future<void> get({
@@ -315,7 +343,7 @@ class _DefaultPub implements Pub {
         '--offline',
       '--example',
     ];
-    await _runWithRetries(
+    await _runWithStdioInherited(
       args,
       command: command,
       context: context,
@@ -346,15 +374,15 @@ class _DefaultPub implements Pub {
     }
   }
 
-  /// Runs pub with [arguments].
+  /// Runs pub with [arguments] and [ProcessStartMode.inheritStdio] mode.
   ///
-  /// Retries the command as long as the exit code is
-  /// `_kPubExitCodeUnavailable`.
+  /// Uses [ProcessStartMode.normal] and [Pub._stdio] if [Pub.test] constructor
+  /// was used.
   ///
-  /// Prints the stderr and stdout of the last run.
+  /// Prints the stdout and stderr of the whole run.
   ///
-  /// Sends an analytics event
-  Future<void> _runWithRetries(
+  /// Sends an analytics event.
+  Future<void> _runWithStdioInherited(
     List<String> arguments, {
     required String command,
     required bool printProgress,
@@ -365,57 +393,47 @@ class _DefaultPub implements Pub {
     String? flutterRootOverride,
   }) async {
     int exitCode;
-    int attempts = 0;
-    int duration = 1;
-
-    List<_OutputLine>? output;
-    StreamSubscription<String> recordLines(Stream<List<int>> stream, _OutputStream streamName) {
-      return stream
-        .transform<String>(utf8.decoder)
-        .transform<String>(const LineSplitter())
-        .listen((String line) => output!.add(_OutputLine(line, streamName)));
-    }
 
-    final Status? status = printProgress
-      ? _logger.startProgress('Running "flutter pub $command" in ${_fileSystem.path.basename(directory)}...',)
-      : null;
+    _logger.printStatus('Running "flutter pub $command" in ${_fileSystem.path.basename(directory)}...');
     final List<String> pubCommand = _pubCommand(arguments);
     final Map<String, String> pubEnvironment = await _createPubEnvironment(context, flutterRootOverride);
     try {
-      do {
-        output = <_OutputLine>[];
-        attempts += 1;
-        final io.Process process = await _processUtils.start(
+      final io.Process process;
+      final io.Stdio? stdio = _stdio;
+
+      if (stdio != null) {
+        // Omit mode parameter and direct pub output to [Pub._stdio] for tests.
+        process = await _processUtils.start(
           pubCommand,
           workingDirectory: _fileSystem.path.current,
           environment: pubEnvironment,
         );
-        final StreamSubscription<String> stdoutSubscription =
-          recordLines(process.stdout, _OutputStream.stdout);
-        final StreamSubscription<String> stderrSubscription =
-          recordLines(process.stderr, _OutputStream.stderr);
 
-        exitCode = await process.exitCode;
+        final StreamSubscription<List<int>> stdoutSubscription =
+          process.stdout.listen(stdio.stdout.add);
+        final StreamSubscription<List<int>> stderrSubscription =
+          process.stderr.listen(stdio.stderr.add);
+
+        await Future.wait<void>(<Future<void>>[
+          stdoutSubscription.asFuture<void>(),
+          stderrSubscription.asFuture<void>(),
+        ]);
+
         unawaited(stdoutSubscription.cancel());
         unawaited(stderrSubscription.cancel());
+      } else {
+        // Let pub inherit stdio for normal operation.
+        process = await _processUtils.start(
+          pubCommand,
+          workingDirectory: _fileSystem.path.current,
+          environment: pubEnvironment,
+          mode: ProcessStartMode.inheritStdio,
+        );
+      }
 
-        if (retry && exitCode == _kPubExitCodeUnavailable) {
-          _logger.printStatus(
-            '$failureMessage (server unavailable) -- attempting retry $attempts in $duration '
-            'second${ duration == 1 ? "" : "s"}...',
-          );
-          await Future<void>.delayed(Duration(seconds: duration));
-          if (duration < 64) {
-            duration *= 2;
-          }
-          // This will cause a retry.
-          output = null;
-        }
-      } while (output == null);
-      status?.stop();
+      exitCode = await process.exitCode;
     // The exception is rethrown, so don't catch only Exceptions.
     } catch (exception) { // ignore: avoid_catches_without_on_clauses
-      status?.cancel();
       if (exception is io.ProcessException) {
         final StringBuffer buffer = StringBuffer('${exception.message}\n');
         final String directoryExistsMessage = _fileSystem.directory(directory).existsSync()
@@ -434,40 +452,19 @@ class _DefaultPub implements Pub {
       rethrow;
     }
 
-    if (printProgress) {
-      // Show the output of the last run.
-      for (final _OutputLine line in output) {
-        switch (line.stream) {
-          case _OutputStream.stdout:
-            _stdio.stdoutWrite('${line.line}\n');
-            break;
-          case _OutputStream.stderr:
-            _stdio.stderrWrite('${line.line}\n');
-            break;
-        }
-      }
-    }
-
     final int code = exitCode;
-    String result = 'success';
-    if (output.any((_OutputLine line) => line.line.contains('version solving failed'))) {
-      result = 'version-solving-failed';
-    } else if (code != 0) {
-      result = 'failure';
-    }
+    final String result = code == 0 ? 'success' : 'failure';
     PubResultEvent(
       context: context.toAnalyticsString(),
       result: result,
       usage: _usage,
     ).send();
-    final String lastPubMessage = output.isEmpty ? 'no message' : output.last.line;
 
     if (code != 0) {
       final StringBuffer buffer = StringBuffer('$failureMessage\n');
       buffer.writeln('command: "${pubCommand.join(' ')}"');
       buffer.write(_stringifyPubEnv(pubEnvironment));
       buffer.writeln('exit code: $code');
-      buffer.writeln('last line of pub output: "${lastPubMessage.trim()}"');
       throwToolExit(
         buffer.toString(),
         exitCode: code,
@@ -813,14 +810,3 @@ class _DefaultPub implements Pub {
     return buffer.toString();
   }
 }
-
-class _OutputLine {
-  _OutputLine(this.line, this.stream);
-  final String line;
-  final _OutputStream stream;
-}
-
-enum _OutputStream {
-  stdout,
-  stderr,
-}
diff --git a/packages/flutter_tools/test/commands.shard/hermetic/analyze_continuously_test.dart b/packages/flutter_tools/test/commands.shard/hermetic/analyze_continuously_test.dart
@@ -77,7 +77,7 @@ void main() {
     testUsingContext('AnalysisServer success', () async {
       createSampleProject(tempDir);
 
-      final Pub pub = Pub(
+      final Pub pub = Pub.test(
         fileSystem: fileSystem,
         logger: logger,
         processManager: processManager,
@@ -117,7 +117,7 @@ void main() {
   testUsingContext('AnalysisServer errors', () async {
     createSampleProject(tempDir, brokenCode: true);
 
-    final Pub pub = Pub(
+    final Pub pub = Pub.test(
       fileSystem: fileSystem,
       logger: logger,
       processManager: processManager,