Skip to content
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

Xcode error message #94747

Merged
merged 1 commit into from Dec 21, 2021
Merged

Conversation

cyanglaz
Copy link
Contributor

@cyanglaz cyanglaz commented Dec 6, 2021

Parse issues in xcresult and display them in console.

Fixes #22536

Sample output:

Semantic Issue (Xcode): Use of undeclared identifier 'ss'
/path/ios/Runner/AppDelegate.m:11:56

Also in this PR:

  1. Updated XCResultGenerator uses a simpler "issuesMap" when parsing issues.
  2. Make simulator build to diagnose failure when build fails.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Dec 6, 2021
@google-cla google-cla bot added the cla: yes label Dec 6, 2021
@cyanglaz cyanglaz requested a review from jmagman December 7, 2021 23:42
@cyanglaz cyanglaz marked this pull request as ready for review December 7, 2021 23:44
@jmagman
Copy link
Member

jmagman commented Dec 8, 2021

There are some test failures.

@cyanglaz
Copy link
Contributor Author

cyanglaz commented Dec 8, 2021

@jmagman Tests passed, PTAL

Comment on lines 404 to 409
case XCResultIssueType.error:
globals.printError('Xcode build issue (${issue.subType}): ${issue.message}');
break;
case XCResultIssueType.warning:
globals.printWarning('Xcode build issue (${issue.subType}): ${issue.message}');
break;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add examples for what this output looks like in the case of errors and warnings in this PR description?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Member

@jmagman jmagman Dec 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add examples for the full flutter build ios and flutter build ios --verbose output of what this kind of error will look like? I can't picture how it will look in the context of all the Xcode logging and build failure text.

I would know more when I see the full output, but this seems too wordy:

Xcode build issue (Swift Compiler Error): Expected dictionary value type

How about:

Swift Compiler Error (Xcode): Expected dictionary value type
  1. Is this message going to be helpful without the class name and line number? Unfortunately this means parsing the url result fragment (which looks like a query, oddly, thanks Xcode...) file:///path/to/ios/Runner/AppDelegate.swift#CharacterRangeLen=0&EndingColumnNumber=8&EndingLineNumber=11&StartingColumnNumber=8&StartingLineNumber=11
  2. There are likely quite a few error messages we should not display, like Command PhaseScriptExecution failed with a nonzero exit code (would happen every time there's a dart compilation issue). Same for the warning that happens when a plugin supports a lower iOS version than the app. Let's get the obvious ones, and then we can add more in future PRs as we discover them.

Copy link
Contributor Author

@cyanglaz cyanglaz Dec 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the compiler error usually display at the end of the build, so it looks like:

Running Xcode build...                                                  
 └─Compiling, linking and signing...                        759ms
Xcode build done.                                            5.5s
Xcode build issue (Swift Compiler Error): Expected dictionary value type
Xcode build issue (Swift Compiler Error): Method does not override any method from its superclass
Failed to build iOS app
Error output from Xcode build:
↳
    ** BUILD FAILED **


Xcode's output:
↳
    Writing result bundle at path:
    	/var/folders/38/ypkvsh7n1d94tv_njrw0nwrh00l9tg/T/flutter_tools.CtyCRE/flutter_ios_build_temp_dire1EhUm/temporary_xcresult_bundle

    /Users/ychris/tmp/xcresult/xcresult_parsing/ios/Runner/AppDelegate.swift:8:83: error: expected dictionary value type
        didFinishLaunchingWithOptions launchOptions: [UIApplication.LaunchOptionsKey: nil]?
                                                                                      ^
    /Users/ychris/tmp/xcresult/xcresult_parsing/ios/Runner/AppDelegate.swift:6:17: error: method does not override any method from its superclass
      override func application(
      ~~~~~~~~      ^
    remark: Incremental compilation has been disabled: it is not compatible with whole module optimization
    note: Using new build system
    note: Planning
    note: Build preparation complete
    note: Building targets in parallel

    Result bundle written to path:
    	/var/folders/38/ypkvsh7n1d94tv_njrw0nwrh00l9tg/T/flutter_tools.CtyCRE/flutter_ios_build_temp_dire1EhUm/temporary_xcresult_bundle


════════════════════════════════════════════════════════════════════════════════
Building a deployable iOS app requires a selected Development Team with a 
Provisioning Profile. Please ensure that a Development Team is selected by:
  1- Open the Flutter project's Xcode target with
       open ios/Runner.xcworkspace
  2- Select the 'Runner' project in the navigator then the 'Runner' target
     in the project settings
  3- Make sure a 'Development Team' is selected under Signing & Capabilities > Team. 
     You may need to:
         - Log in with your Apple ID in Xcode first
         - Ensure you have a valid unique Bundle ID
         - Register your device with your Apple Developer Account
         - Let Xcode automatically provision a profile for your app
  4- Build or run your project again

For more information, please visit:
  https://flutter.dev/docs/get-started/install/macos#deploy-to-ios-devices

Or run on an iOS simulator without code signing
════════════════════════════════════════════════════════════════════════════════
Encountered error while building for device.

In this instance, the xcresult message is not particularly useful as the more useful messages are displayed at the end.

What do you think about this when parsing the url result:

File: file:///path/to/ios/Runner/AppDelegate.swift
StartLineNumber: 11
EndLineNumber: 11
StartColumnNumber: 8
EndColumnNumber: 8

I think this detailed compiler error code position is messy enough to deserve a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2
Good idea, I can create a separate PR to filter errors we don't want to display to user in non-verbose mode.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this instance, the xcresult message is not particularly useful as the more useful messages are displayed at the end.

Instead of printing as you have, what if instead add the XCResult stuff to the XcodeBuildResult? And then the caller can choose how and in what order to display this.

final XcodeBuildResult result = await buildXcodeProject(

File: file:///path/to/ios/Runner/AppDelegate.swift
StartLineNumber: 11
EndLineNumber: 11
StartColumnNumber: 8
EndColumnNumber: 8

Let's copy what Xcode does when it displays, with just the line and column start.
/path/to/ios/Runner/AppDelegate.swift:11:8

#95035

I don't think we should land this PR without making sure confusing PhaseScriptExecution errors shows up with dart compilation errors. We can polish in subsequent PRs, but a dart issue is a very core and much more common case than a Swift compilation error, for example.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See diagnoseXcodeBuildFailure for where it would probably make sense to handle these from a XcodeBuildResult.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also there are a few things in diagnoseXcodeBuildFailure that may actually be more accurate once we can parse these xcresults. For example, it just guesses there's a codesigning error sometimes, even when something else really failed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parsing "url" in this PR: #95054

packages/flutter_tools/lib/src/ios/mac.dart Show resolved Hide resolved
@cyanglaz
Copy link
Contributor Author

Created issues to track the 2 sub problems: #95035, #95036

Comment on lines 339 to 341
if (xcResult.issues.isEmpty) {
globals.printStatus('Xcode build has no issues!');
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove this trace, we should already be able to see similar output from Xcode in verbose mode.

issueSummary += issue.subType ?? 'Unknown';
issueSummary += ' (Xcode): ';
issueSummary += issue.message ?? '';
issueSummary += issue.location != null? '\n$issue.location' : '';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
issueSummary += issue.location != null? '\n$issue.location' : '';
issueSummary += issue.location != null ? '\n$issue.location' : '';

return;
}

xcResult.issues.forEach( (final XCResultIssue issue) => _handleXCResultIssue(issue: issue, logger: logger));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Analyzer didn't like this:

info • Avoid using `forEach` with a function literal • packages/flutter_tools/lib/src/ios/mac.dart:622:19 • avoid_function_literals_in_foreach_calls
Suggested change
xcResult.issues.forEach( (final XCResultIssue issue) => _handleXCResultIssue(issue: issue, logger: logger));
for (final XCResultIssue issue in xcResult.issues) {
_handleXCResultIssue(issue: issue, logger: logger);
}

}

// Add more custom output for flutter users.
if (issue.message != null && issue.message!.contains('Provisioning profile')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this may also be No matching provisioning profiles found, though I didn't remove the profiles on my machine to test. Can you make this case insensitive to make it a little more robust to string changes?
(I lamented to you on chat already that I wish we have error codes to go off of...)

'json'
],
stdout: stdout,
onRun: onRun);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
onRun: onRun);
onRun: onRun,
);


void _handleXCResultIssue({required XCResultIssue issue, required Logger logger}) {
// Issue summary from xcresult.
String issueSummary = '';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of += string concatenation, use a StringBuffer.

'--path',
_xcBundleFilePath,
'--format',
'json'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'json'
'json',

Comment on lines 320 to 321
_setUpXCResultCommand(onRun: () {
}),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onRun can be null.

Suggested change
_setUpXCResultCommand(onRun: () {
}),
_setUpXCResultCommand(),

@cyanglaz
Copy link
Contributor Author

@jmagman Ready to be reviewed again.

Copy link
Member

@jmagman jmagman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with minor whitespace nit.



testUsingContext('Extra error message for provision profile issue in xcresulb bundle.', () async {
final BuildCommand command = BuildCommand();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra indentation.

@fluttergithubbot
Copy link
Contributor

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite customer_testing-linux has failed. Please fix the issues identified (or deflake) before re-applying this label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

flutter run doesn't show some critical log items from Xcode
3 participants