diff --git a/packages/flutter_tools/lib/src/commands/update_packages.dart b/packages/flutter_tools/lib/src/commands/update_packages.dart index b2698c023c2..0dc01d58cae 100644 --- 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 index c619a774701..656ab79109d 100644 --- 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()!; @@ -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 pubCommand = [..._pubCommand, ...arguments]; final Map 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 index 104177f7aaa..78bb249779c 100644 --- 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 index cbef04a9892..77ea1b13d0d 100644 --- 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 index b19633d4b9b..8a8b51bb0fe 100644 --- 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 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 index 4e08a755008..1a9e079620c 100644 --- 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() - .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([ + const FakeCommand( + command: [ + '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: { + '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.