Do not silently fail pub get even if output-mode is "none" (#153596)

I am making an assumption `OutputMode.none` should _really_ mean `OutputMode.failuresOnly`, that is, if we ever get a non-zero exit code, we still want to know why. If I've somehow misunderstood that, LMK and I'm happy to revert this PR or make adjustments. This fixes the bug where if you were to do: ```sh git clone https://github.com/myuser/fork-of-flutter cd fork-of-flutter ./bin/flutter update-packages ``` You now get: 1. An actual error message, versus no output at all. 2. A warning that a common reason is not tracking a remote, with instructions to fix it. Closes #148569.
flutter · Aug 22, 2024 · a9e94d9 · a9e94d9
1 parent 544ce7c
commit a9e94d9
Show file tree

Hide file tree

Showing 6 changed files with 100 additions and 19 deletions.
diff --git a/packages/flutter_tools/lib/src/commands/update_packages.dart b/packages/flutter_tools/lib/src/commands/update_packages.dart
@@ -524,15 +524,13 @@ class UpdatePackagesCommand extends FlutterCommand {
 
     // Run "pub get" on it in order to force the download of any
     // needed packages to the pub cache, upgrading if requested.
-    // TODO(ianh): If this fails, the tool exits silently.
-    // It can fail, e.g., if --cherry-pick-version is invalid.
     await pub.get(
       context: PubContext.updatePackages,
       project: FlutterProject.fromDirectory(syntheticPackageDir),
       upgrade: doUpgrade,
       offline: boolArg('offline'),
       flutterRootOverride: temporaryFlutterSdk?.path,
-      outputMode: PubOutputMode.none,
+      outputMode: PubOutputMode.failuresOnly,
     );
 
     if (reportDependenciesToTree) {
@@ -615,7 +613,7 @@ class UpdatePackagesCommand extends FlutterCommand {
             // All dependencies should already have been downloaded by the fake
             // package, so the concurrent checks can all happen offline.
             offline: true,
-            outputMode: PubOutputMode.none,
+            outputMode: PubOutputMode.failuresOnly,
           );
           stopwatch.stop();
           final double seconds = stopwatch.elapsedMilliseconds / 1000.0;

diff --git a/packages/flutter_tools/lib/src/dart/pub.dart b/packages/flutter_tools/lib/src/dart/pub.dart
@@ -22,6 +22,7 @@ import '../convert.dart';
 import '../dart/package_map.dart';
 import '../project.dart';
 import '../reporting/reporting.dart';
+import '../version.dart';
 
 /// The [Pub] instance.
 Pub get pub => context.get<Pub>()!;
@@ -91,13 +92,16 @@ class PubContext {
 }
 
 /// Describes the amount of output that should get printed from a `pub` command.
-/// [PubOutputMode.all] indicates that the complete output is printed. This is
-/// typically the default.
-/// [PubOutputMode.none] indicates that no output should be printed.
-/// [PubOutputMode.summaryOnly] indicates that only summary information should be printed.
 enum PubOutputMode {
-  none,
+  /// No normal output should be printed.
+  ///
+  /// If the command were to fail, failures are still printed.
+  failuresOnly,
+
+  /// The complete output should be printed; this is typically the default.
   all,
+
+  /// Only summary information should be printed.
   summaryOnly,
 }
 
@@ -392,8 +396,9 @@ class _DefaultPub implements Pub {
     final List<String> pubCommand = <String>[..._pubCommand, ...arguments];
     final Map<String, String> pubEnvironment = await _createPubEnvironment(context: context, flutterRootOverride: flutterRootOverride, summaryOnly: outputMode == PubOutputMode.summaryOnly);
 
+    String? pubStderr;
     try {
-      if (outputMode != PubOutputMode.none) {
+      if (outputMode != PubOutputMode.failuresOnly) {
         final io.Stdio? stdio = _stdio;
         if (stdio == null) {
           // Let pub inherit stdio and output directly to the tool's stdout and
@@ -447,6 +452,7 @@ class _DefaultPub implements Pub {
         );
 
         exitCode = result.exitCode;
+        pubStderr = result.stderr;
       }
     } on io.ProcessException catch (exception) {
       final StringBuffer buffer = StringBuffer('${exception.message}\n');
@@ -477,7 +483,27 @@ class _DefaultPub implements Pub {
       buffer.write(_stringifyPubEnv(pubEnvironment));
       buffer.writeln('exit code: $code');
       _logger.printTrace(buffer.toString());
-      throwToolExit(null, exitCode: code);
+
+      // When this is null, but a failure happened, it is assumed that stderr
+      // was already redirected to the process stderr. This handles the corner
+      // case where we otherwise would log nothing. See
+      // https://github.com/flutter/flutter/issues/148569 for details.
+      if (pubStderr != null) {
+        _logger.printError(pubStderr);
+      }
+      if (context == PubContext.updatePackages) {
+        _logger.printWarning(
+          'If the current version was resolved as $kUnknownFrameworkVersion '
+          'and this is a fork of flutter/flutter, you forgot to set the remote '
+          'upstream branch to point to the canonical flutter/flutter: \n\n'
+          '  git remote set-url upstream https://github.com/flutter/flutter.git\n'
+          '\n'
+          'See https://github.com/flutter/flutter/blob/main/docs/contributing/Setting-up-the-Framework-development-environment.md#set-up-your-environment.');
+      }
+      throwToolExit(
+        'Failed to update packages.',
+        exitCode: code,
+      );
     }
   }
 

diff --git a/packages/flutter_tools/lib/src/flutter_cache.dart b/packages/flutter_tools/lib/src/flutter_cache.dart
@@ -129,7 +129,7 @@ class PubDependencies extends ArtifactSet {
         fileSystem.directory(fileSystem.path.join(_flutterRoot(), 'packages', 'flutter_tools'))
       ),
       offline: offline,
-      outputMode: PubOutputMode.none,
+      outputMode: PubOutputMode.failuresOnly,
     );
   }
 }

diff --git a/packages/flutter_tools/lib/src/version.dart b/packages/flutter_tools/lib/src/version.dart
@@ -15,7 +15,8 @@ import 'cache.dart';
 import 'convert.dart';
 import 'globals.dart' as globals;
 
-const String _unknownFrameworkVersion = '0.0.0-unknown';
+/// The default version when a version could not be determined.
+const String kUnknownFrameworkVersion = '0.0.0-unknown';
 
 /// A git shortcut for the branch that is being tracked by the current one.
 ///
@@ -228,7 +229,7 @@ abstract class FlutterVersion {
 
   @override
   String toString() {
-    final String versionText = frameworkVersion == _unknownFrameworkVersion ? '' : ' $frameworkVersion';
+    final String versionText = frameworkVersion == kUnknownFrameworkVersion ? '' : ' $frameworkVersion';
     final String flutterText = 'Flutter$versionText • channel $channel • ${repositoryUrl ?? 'unknown source'}';
     final String frameworkText = 'Framework • revision $frameworkRevisionShort ($frameworkAge) • $frameworkCommitDate';
     final String engineText = 'Engine • revision $engineRevisionShort';
@@ -359,7 +360,7 @@ abstract class FlutterVersion {
 
   /// Return a short string for the version (e.g. `master/0.0.59-pre.92`, `scroll_refactor/a76bc8e22b`).
   String getVersionString({ bool redactUnknownBranches = false }) {
-    if (frameworkVersion != _unknownFrameworkVersion) {
+    if (frameworkVersion != kUnknownFrameworkVersion) {
       return '${getBranchName(redactUnknownBranches: redactUnknownBranches)}/$frameworkVersion';
     }
     return '${getBranchName(redactUnknownBranches: redactUnknownBranches)}/$frameworkRevisionShort';
@@ -1053,7 +1054,7 @@ class GitTagVersion {
 
   String frameworkVersionFor(String revision) {
     if (x == null || y == null || z == null || (hash != null && !revision.startsWith(hash!))) {
-      return _unknownFrameworkVersion;
+      return kUnknownFrameworkVersion;
     }
     if (commits == 0 && gitTag != null) {
       return gitTag!;

diff --git a/packages/flutter_tools/test/general.shard/cache_test.dart b/packages/flutter_tools/test/general.shard/cache_test.dart
@@ -1039,7 +1039,7 @@ void main() {
     expect(
       pub.invocations.first,
       predicate<FakePubInvocation>(
-        (FakePubInvocation invocation) => invocation.outputMode == PubOutputMode.none,
+        (FakePubInvocation invocation) => invocation.outputMode == PubOutputMode.failuresOnly,
         'Pub invoked with PubOutputMode.none',
       ),
     );

diff --git a/packages/flutter_tools/test/general.shard/dart/pub_get_test.dart b/packages/flutter_tools/test/general.shard/dart/pub_get_test.dart
@@ -616,7 +616,7 @@ exit code: 66
         context: PubContext.flutterTests,
       ),
       throwsA(isA<ToolExit>()
-          .having((ToolExit error) => error.message, 'message', null)),
+          .having((ToolExit error) => error.message, 'message', contains('Failed to update packages'))),
     );
     expect(logger.statusText, isEmpty);
     expect(logger.traceText, contains(toolExitMessage));
@@ -629,6 +629,62 @@ exit code: 66
     expect(processManager, hasNoRemainingExpectations);
   });
 
+  testWithoutContext('pub get with failing exit code even with OutputMode == failuresOnly', () async {
+    final BufferLogger logger = BufferLogger.test();
+    final FileSystem fileSystem = MemoryFileSystem.test();
+
+    final FakeProcessManager processManager = FakeProcessManager.list(<FakeCommand>[
+      const FakeCommand(
+        command: <String>[
+          'bin/cache/dart-sdk/bin/dart',
+          'pub',
+          '--suppress-analytics',
+          '--directory',
+          '.',
+          'get',
+          '--example',
+        ],
+        exitCode: 1,
+        stderr: '===pub get failed stderr here===',
+        stdout: 'out1\nout2\nout3\n',
+        environment: <String, String>{
+          'FLUTTER_ROOT': '',
+          'PUB_ENVIRONMENT': 'flutter_cli:update_packages'
+        },
+      ),
+    ]);
+
+    // Intentionally not using pub.test to simulate a real environment, but
+    // we are using non-inherited I/O to avoid printing to the console.
+    final Pub pub = Pub(
+      platform: FakePlatform(),
+      fileSystem: fileSystem,
+      logger: logger,
+      usage: TestUsage(),
+      botDetector: const BotDetectorAlwaysNo(),
+      processManager: processManager,
+    );
+
+    await expectLater(
+      () => pub.get(
+        project: FlutterProject.fromDirectoryTest(fileSystem.currentDirectory),
+        context: PubContext.updatePackages,
+        outputMode: PubOutputMode.failuresOnly,
+      ),
+      throwsToolExit(
+        message: 'Failed to update packages',
+      ),
+    );
+    expect(logger.statusText, isEmpty);
+    expect(logger.errorText, contains('===pub get failed stderr here==='));
+    expect(
+      logger.warningText,
+      contains('git remote set-url upstream'),
+      reason: 'When update-packages fails, it is often because of missing an upsteam remote.',
+    );
+    expect(processManager, hasNoRemainingExpectations);
+  });
+
   testWithoutContext('pub get shows working directory on process exception',
       () async {
     final BufferLogger logger = BufferLogger.test();
@@ -745,7 +801,7 @@ exit code: 66
       await pub.get(
         project: FlutterProject.fromDirectoryTest(fileSystem.currentDirectory),
         context: PubContext.flutterTests,
-        outputMode: PubOutputMode.none,
+        outputMode: PubOutputMode.failuresOnly,
       );
     } on ToolExit {
       // Ignore.