-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Log XCResult before other build issues #100787
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when there's provisioning profile in the error? Looks like _handleXCResultIssue and the noProvisioningProfileInstruction check below both output?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean? The _handleXCResultIssue has a special case to handles general provisioning profile issues and have some special logs for it. It doesn't print the noProvisioningProfileInstruction. The later check (result.stdout?.contains("Xcode couldn't find a provisioning profile matching") would print an instruction for user if they don't have a provisioning profile in the project.
Did you mean we can add the special "no provisioning profile" case check in _handleXCResultIssue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before your change, if there was a provisioning profile issue it would print this and return:
flutter/packages/flutter_tools/lib/src/ios/mac.dart
Lines 590 to 597 in c01cbd2
| if (xcodeBuildExecution != null | |
| && xcodeBuildExecution.environmentType == EnvironmentType.physical | |
| && (result.stdout?.contains('BCEROR') ?? false) | |
| // May need updating if Xcode changes its outputs. | |
| && (result.stdout?.contains("Xcode couldn't find a provisioning profile matching") ?? false)) { | |
| logger.printError(noProvisioningProfileInstruction, emphasis: true); | |
| return; | |
| } |
Now, it prints
flutter/packages/flutter_tools/lib/src/ios/mac.dart
Lines 748 to 756 in c01cbd2
| if (issue.message != null && issue.message!.toLowerCase().contains('provisioning profile')) { | |
| logger.printError(''); | |
| logger.printError('It appears that there was a problem signing your application prior to installation on the device.'); | |
| logger.printError(''); | |
| logger.printError('Verify that the Bundle Identifier in your project is your signing id in Xcode'); | |
| logger.printError(' open ios/Runner.xcworkspace'); | |
| logger.printError(''); | |
| logger.printError("Also try selecting 'Product > Build' to fix the problem:"); | |
| } |
and then the above. Unless I'm missing something.
What does that text together look like? Probably needs to be adjusted, or the one only printed if the xcresult wasn't created?
cyanglaz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmagman Updated per our offline discussion. PTAL.
I will update the PR description to reflect all the changes when the general idea of the PR is agreed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The noDevelopmentTeamInstruction is documented as a fallback message for signing issues. I do think it is a more proper user message + instructions. Let me know if you think otherwise. @jmagman
|
Looks like there are some test failures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a better situation to use noDevelopmentTeamInstruction instead of message.toLowerCase().contains('provisioning profile') since it actually checks if you have DEVELOPMENT_TEAM or PROVISIONING_PROFILE set. Can we pipe the xcodeBuildExecution into the xcresult handler to do similar checking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is technically not part of handing xcresult. XCResult does detect this error by showing a message: Showing Recent Messages Signing for "Runner" requires a development team. Select a development team in the Signing & Capabilities editor.
How about move this part where we directly check the flags all the way above xcresult handling?
So the flow becomes:
- Check
DEVELOPMENT_TEAMandPROVISIONING_PROFILEflags. - Handle xcresults
- Handle other issues with stdout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is technically not part of handing xcresult
I know, I meant, maybe it should be, the xcresult can print noDevelopmentTeamInstruction if, instead of just message.toLowerCase().contains('provisioning profile') it also checks the build settings?
- Check
DEVELOPMENT_TEAMandPROVISIONING_PROFILEflags.- Handle xcresults
- Handle other issues with stdout.
I'm not following this, why would this one example be a special case where it's handled before the message is parsed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not following this, why would this one example be a special case where it's handled before the message is parsed?
So the "Check DEVELOPMENT_TEAM and PROVISIONING_PROFILE flag" can be done regardless if xcresult's parsing is successful or not. If this check is part of xcresult handling, it will not be run when xcresult is failed to parse. Or we do the same thing in xcresult handling as well as the fallback error handling when xcresult is failed to parse?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how you want to organize it 🙂 We should only print out the same error once, and include useful recovery suggestions where possible. There's currently fallback to string parsing if the result bundle fails to build, but maybe that never happens (or we should add Usage data about it to see).
|
492cc2d to
a70903c
Compare
This is twice in the description. Is that a typo or did it actually print twice? |
It was a typo |
jmagman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM w/ minor nit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you make the local before the null check you won't need the !
| if (issue.message == null) { | |
| return _XCResultIssueHandlingResult(requiresProvisioningProfile: false, hasProvisioningProfileIssue: false); | |
| } | |
| // Add more error messages for flutter users for some special errors. | |
| final String message = issue.message!; | |
| // Add more error messages for flutter users for some special errors. | |
| final String? message = issue.message; | |
| if (issue.message == null) { | |
| return _XCResultIssueHandlingResult(requiresProvisioningProfile: false, hasProvisioningProfileIssue: false); | |
| } |
6fa630e to
cd8bd29
Compare
|
This pull request is not suitable for automatic merging in its current state.
|
cd8bd29 to
6fa630e
Compare
6fa630e to
cd4acce
Compare
|
This pull request is not suitable for automatic merging in its current state.
|
|
This pull request is not suitable for automatic merging in its current state.
|
|
This pull request is not suitable for automatic merging in its current state.
|
Always log XCResult first so it won't get lost if there are other issues.
Also updated the order issues are detected.
Some sample error messages after the change are:
Fixes #100723
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.