From bb306c53a353988d3e03b60c6909c1af08a97e3a Mon Sep 17 00:00:00 2001 From: Jenn Magder Date: Fri, 21 Feb 2025 10:36:35 -0800 Subject: [PATCH] Suppress stderr during Xcode command line installation check (#163785) Xcode is in some kind of half-installed state (missing a cert? unknown) on some devicelab Macs https://github.com/flutter/flutter/issues/161655. As of https://github.com/flutter/flutter/pull/163685 Xcode cipd installation isn't requested as part of the builder configuration, so it seems like Xcode is just hanging out on that devicelab bot quasi-installed, unrelated to the recipe. In any case, the tool is actually doing the right thing and detecting that Xcode isn't in a good state and continuing as if it isn't installed, but is logging a wall of error text about it to stderr, which that Android test doesn't like. Instead of updating the test to allow stderr, instead swap the Xcode installation path to `exitsHappySync` to only check the exit code (or exception), which is the original intention behind the Xcode command line checks. https://github.com/flutter/flutter/blob/e6730613c97cc3628ecda39c20c60ecb5938366f/packages/flutter_tools/lib/src/base/process.dart#L596-L601 Fixes https://github.com/flutter/flutter/issues/161655 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --- .../flutter_tools/lib/src/macos/xcode.dart | 44 ++++++---------- .../test/general.shard/macos/xcode_test.dart | 52 +++++++++++++++++++ 2 files changed, 68 insertions(+), 28 deletions(-) diff --git a/packages/flutter_tools/lib/src/macos/xcode.dart b/packages/flutter_tools/lib/src/macos/xcode.dart index 491d5ecd89..e31d4735fd 100644 --- a/packages/flutter_tools/lib/src/macos/xcode.dart +++ b/packages/flutter_tools/lib/src/macos/xcode.dart @@ -172,22 +172,15 @@ class Xcode { /// Verifies that simctl is installed by trying to run it. bool get isSimctlInstalled { - if (_isSimctlInstalled == null) { - try { - // This command will error if additional components need to be installed in - // xcode 9.2 and above. - final RunResult result = _processUtils.runSync([ - ...xcrunCommand(), - 'simctl', - 'list', - 'devices', - 'booted', - ]); - _isSimctlInstalled = result.exitCode == 0; - } on ProcessException { - _isSimctlInstalled = false; - } - } + // This command will error if additional components need to be installed in + // xcode 9.2 and above. + _isSimctlInstalled ??= _processUtils.exitsHappySync([ + ...xcrunCommand(), + 'simctl', + 'list', + 'devices', + 'booted', + ]); return _isSimctlInstalled ?? false; } @@ -197,20 +190,15 @@ class Xcode { /// to run it. `devicectl` is made available in Xcode 15. bool get isDevicectlInstalled { if (_isDevicectlInstalled == null) { - try { - if (currentVersion == null || currentVersion!.major < 15) { - _isDevicectlInstalled = false; - return _isDevicectlInstalled!; - } - final RunResult result = _processUtils.runSync([ - ...xcrunCommand(), - 'devicectl', - '--version', - ]); - _isDevicectlInstalled = result.exitCode == 0; - } on ProcessException { + if (currentVersion == null || currentVersion!.major < 15) { _isDevicectlInstalled = false; + return _isDevicectlInstalled!; } + _isDevicectlInstalled = _processUtils.exitsHappySync([ + ...xcrunCommand(), + 'devicectl', + '--version', + ]); } return _isDevicectlInstalled ?? false; } diff --git a/packages/flutter_tools/test/general.shard/macos/xcode_test.dart b/packages/flutter_tools/test/general.shard/macos/xcode_test.dart index 3d7f8e3b7d..70d90a3b50 100644 --- a/packages/flutter_tools/test/general.shard/macos/xcode_test.dart +++ b/packages/flutter_tools/test/general.shard/macos/xcode_test.dart @@ -78,18 +78,45 @@ void main() { fakeProcessManager.addCommand( const FakeCommand( command: ['xcrun', 'simctl', 'list', 'devices', 'booted'], + stderr: 'failed to run', exitCode: 1, ), ); final Xcode xcode = Xcode.test( processManager: fakeProcessManager, xcodeProjectInterpreter: xcodeProjectInterpreter, + logger: logger, ); expect(xcode.isSimctlInstalled, isFalse); expect(fakeProcessManager, hasNoRemainingExpectations); + expect(logger.statusText, isEmpty); + expect(logger.errorText, isEmpty); }); + testWithoutContext( + 'isSimctlInstalled is false when simctl list throws process exception', + () { + fakeProcessManager.addCommand( + const FakeCommand( + command: ['xcrun', 'simctl', 'list', 'devices', 'booted'], + exception: ProcessException('xcrun', ['simctl']), + ), + ); + final Xcode xcode = Xcode.test( + processManager: fakeProcessManager, + xcodeProjectInterpreter: xcodeProjectInterpreter, + logger: logger, + ); + + expect(xcode.isSimctlInstalled, isFalse); + expect(fakeProcessManager, hasNoRemainingExpectations); + expect(logger.statusText, isEmpty); + expect(logger.errorText, isEmpty); + expect(logger.traceText, contains('ProcessException')); + }, + ); + group('isDevicectlInstalled', () { testWithoutContext('is true when Xcode is 15+ and devicectl succeeds', () { fakeProcessManager.addCommand( @@ -113,10 +140,35 @@ void main() { final Xcode xcode = Xcode.test( processManager: fakeProcessManager, xcodeProjectInterpreter: xcodeProjectInterpreter, + logger: logger, ); expect(xcode.isDevicectlInstalled, isFalse); expect(fakeProcessManager, hasNoRemainingExpectations); + expect(logger.statusText, isEmpty); + expect(logger.errorText, isEmpty); + }); + + testWithoutContext('is false when devicectl throws process exception', () { + fakeProcessManager.addCommand( + const FakeCommand( + command: ['xcrun', 'devicectl', '--version'], + exitCode: 1, + exception: ProcessException('xcrun', ['devicectl']), + ), + ); + xcodeProjectInterpreter.version = Version(15, 0, 0); + final Xcode xcode = Xcode.test( + processManager: fakeProcessManager, + xcodeProjectInterpreter: xcodeProjectInterpreter, + logger: logger, + ); + + expect(xcode.isDevicectlInstalled, isFalse); + expect(fakeProcessManager, hasNoRemainingExpectations); + expect(logger.statusText, isEmpty); + expect(logger.errorText, isEmpty); + expect(logger.traceText, contains('ProcessException')); }); testWithoutContext('is false when Xcode is less than 15', () {