Skip to content

Commit

Permalink
Suppress stderr during Xcode command line installation check (#163785)
Browse files Browse the repository at this point in the history
Xcode is in some kind of half-installed state (missing a cert? unknown)
on some devicelab Macs flutter/flutter#161655.
As of flutter/flutter#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 flutter/flutter#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].

<!-- Links -->
[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
  • Loading branch information
jmagman authored Feb 21, 2025
1 parent 911aa75 commit bb306c5
Show file tree
Hide file tree
Showing 2 changed files with 68 additions and 28 deletions.
44 changes: 16 additions & 28 deletions packages/flutter_tools/lib/src/macos/xcode.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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(<String>[
...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(<String>[
...xcrunCommand(),
'simctl',
'list',
'devices',
'booted',
]);
return _isSimctlInstalled ?? false;
}

Expand All @@ -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(<String>[
...xcrunCommand(),
'devicectl',
'--version',
]);
_isDevicectlInstalled = result.exitCode == 0;
} on ProcessException {
if (currentVersion == null || currentVersion!.major < 15) {
_isDevicectlInstalled = false;
return _isDevicectlInstalled!;
}
_isDevicectlInstalled = _processUtils.exitsHappySync(<String>[
...xcrunCommand(),
'devicectl',
'--version',
]);
}
return _isDevicectlInstalled ?? false;
}
Expand Down
52 changes: 52 additions & 0 deletions packages/flutter_tools/test/general.shard/macos/xcode_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -78,18 +78,45 @@ void main() {
fakeProcessManager.addCommand(
const FakeCommand(
command: <String>['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: <String>['xcrun', 'simctl', 'list', 'devices', 'booted'],
exception: ProcessException('xcrun', <String>['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(
Expand All @@ -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: <String>['xcrun', 'devicectl', '--version'],
exitCode: 1,
exception: ProcessException('xcrun', <String>['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', () {
Expand Down

0 comments on commit bb306c5

Please sign in to comment.