Made xcode_backend stop on error. #50664

gaaclarke · 2020-02-12T20:58:51Z

Description

Started trying to make xcode_backend.sh abort execution earlier if it runs into an error so that users will be reporting more apropos problems instead of linker errors.

Related Issues

#32454

Tests

Just manual testing.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.

cbracken

Make sure to test with various settings of ARCH but if that looks good, lgtm modulo nits.

cbracken · 2020-02-12T21:34:36Z

packages/flutter_tools/bin/xcode_backend.sh

 # found in the LICENSE file.

+# Exit on error
+set -e


Have you verified that no valid code paths currently return non-zero? I remember filing a bug at one point wanting to add set -e to this script but needing to hold off. If we’re lucky, I documented the known cases on that issue. (No promises!)

The only one I could get to hit was the plistbuddy error, that's why I pulled that out to check for the presence before extraction.

cbracken · 2020-02-12T21:39:34Z

packages/flutter_tools/bin/xcode_backend.sh

 }

+PlistHasField() {
+  if /usr/libexec/PlistBuddy -c "Print :$1" $2 1>/dev/null 2>/dev/null; then


You should be able to use CMD > /dev/null 2>&1 for this check.

Here and elsewhere: Since this is bash consider using a double-square bracket test.

Changed the redirect, I don't think double bracket is what I want here since I'm checking the return code of the process.

jmagman · 2020-02-12T22:11:03Z

\cc @jonahwilliams

jonahwilliams · 2020-02-12T22:12:53Z

packages/flutter_tools/bin/xcode_backend.sh

-    assets_path="${FLTAssetsPath}"
+  # The value of assets_path can set by add FLTAssetsPath to
+  # AppFrameworkInfo.plist.
+  if PlistHasField "FLTAssetsPath" "${derived_dir}/AppFrameworkInfo.plist"; then


Does this need to be implemented in bash? Could it be something in the tool build instead?

jonahwilliams

I'd rather not add more logic to xcode_backend.sh if possible

jonahwilliams · 2020-02-12T22:16:23Z

packages/flutter_tools/bin/xcode_backend.sh

+  if PlistHasField "FLTAssetsPath" "${derived_dir}/AppFrameworkInfo.plist"; then
+    FLTAssetsPath=$(/usr/libexec/PlistBuddy -c "Print :FLTAssetsPath" "${derived_dir}/AppFrameworkInfo.plist" 2>/dev/null)
+    if [[ -n "$FLTAssetsPath" ]]; then
+      assets_path="${FLTAssetsPath}"


This setting was never tested and regressed sometime in the past few months. Moving the asset override into Dart will let us cover it with a unit test at least.

Something here: https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/build_system/targets/ios.dart#L286

I support that. In the meantime this just added error checking to the whole script. Check out the delta now, hardly touched it.

gaaclarke · 2020-02-12T22:22:29Z

I'd rather not add more logic to xcode_backend.sh if possible

The function has been removed. Accomplished the same thing with checking the return status.

jmagman

Checked out this branch, built Debug, Profile, Release, and archiving. Didn't see any errors.

jmagman

Actually what about line 185?

  if [[ $? -ne 0 ]]; then
    EchoError "Failed to package ${project_path}."
    exit -1
  fi

gaaclarke · 2020-02-12T23:06:47Z

Actually what about line 185?

  if [[ $? -ne 0 ]]; then
    EchoError "Failed to package ${project_path}."
    exit -1
  fi

The "RunCommand" function messes it up. I had removed that one, but if it errors out it is going to halt at line line 13. I suspect the message "Failed to package $(project_path)" isn't all that useful considering the text that will be generated by the command failing. The 'exit -1' isn't an issue because we will halt when we get an error anyway.

gaaclarke · 2020-02-12T23:07:37Z

I thought of getting rid of "RunCommand" and using "-x" instead, but I tried to keep the change smaller.

jmagman · 2020-02-12T23:08:09Z

Actually what about line 185?
  if [[ $? -ne 0 ]]; then
    EchoError "Failed to package ${project_path}."
    exit -1
  fi
The "RunCommand" function messes it up. I had removed that one, but if it errors out it is going to halt at line line 13. I suspect the message "Failed to package $(project_path)" isn't all that useful considering the text that will be generated by the command failing. The 'exit -1' isn't an issue because we will halt when we get an error anyway.

Can you remove the Failed to package $(project_path)" part since it's dead code?

gaaclarke · 2020-02-12T23:13:19Z

I could introduce:

TestCommand() {
  if [[ -n "$VERBOSE_SCRIPT_LOGGING" ]]; then
    echo "♦ $*"
  fi
  if "$@"; then
    return 0
  else
    return 1
  fi
}

but there was some resistance to adding more logic to the script.

cbracken · 2020-02-12T23:56:33Z

Does this need to be implemented in bash? Could it be something in the tool build instead?

Honestly, I think we could implement almost this entire script in Dart. We need to keep the script itself forever, for backward compatibility with existing projects that invoke it in build phases, but we could probably make it a ~one-line script that just invokes the flutter tool with a new sub-command.

jonahwilliams · 2020-02-12T23:58:42Z

Right, long-term there are two different directions we can make improvements:

Move logic from xcode_backend.sh into the tool where appropriate, to reduce the usage of bash.
(Unfunded) develop a new project structure for iOS that is 100% dart, and takes advantage of newer Xcode features like depfile inputs for Run Script Phases.

Made xcode_backend stop on error.

640f5db

fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label Feb 12, 2020

googlebot added the cla: yes label Feb 12, 2020

gaaclarke requested review from cbracken and jmagman February 12, 2020 20:58

cbracken approved these changes Feb 12, 2020

View reviewed changes

jonahwilliams reviewed Feb 12, 2020

View reviewed changes

jonahwilliams suggested changes Feb 12, 2020

View reviewed changes

tweaked the redirections

f46c04b

jonahwilliams reviewed Feb 12, 2020

View reviewed changes

removed helper function, just checked the return status

8748563

gaaclarke requested a review from jonahwilliams February 12, 2020 22:24

jonahwilliams approved these changes Feb 12, 2020

View reviewed changes

jmagman approved these changes Feb 12, 2020

View reviewed changes

jmagman requested changes Feb 12, 2020

View reviewed changes

jmagman approved these changes Feb 12, 2020

View reviewed changes

gaaclarke added the waiting for tree to go green label Feb 12, 2020

fluttergithubbot merged commit d3c318e into flutter:master Feb 13, 2020

jmagman mentioned this pull request Aug 15, 2020

[flutter_tools] correctly forward error only stdout in non-verbose modes #63815

Merged

github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2021

Made xcode_backend stop on error. #50664

Made xcode_backend stop on error. #50664

Uh oh!

Conversation

gaaclarke commented Feb 12, 2020

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

cbracken Feb 12, 2020

Choose a reason for hiding this comment

Uh oh!

gaaclarke Feb 12, 2020

Choose a reason for hiding this comment

Uh oh!

cbracken Feb 12, 2020

Choose a reason for hiding this comment

Uh oh!

gaaclarke Feb 12, 2020

Choose a reason for hiding this comment

Uh oh!

jmagman commented Feb 12, 2020

Uh oh!

jonahwilliams Feb 12, 2020

Choose a reason for hiding this comment

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

jonahwilliams Feb 12, 2020

Choose a reason for hiding this comment

Uh oh!

gaaclarke Feb 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaaclarke commented Feb 12, 2020

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

gaaclarke commented Feb 12, 2020

Uh oh!

gaaclarke commented Feb 12, 2020

Uh oh!

jmagman commented Feb 12, 2020

Uh oh!

gaaclarke commented Feb 12, 2020

Uh oh!

cbracken commented Feb 12, 2020

Uh oh!

jonahwilliams commented Feb 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

gaaclarke Feb 12, 2020 •

edited

Loading