Xcode backend refactor #23387

dnfield · 2018-10-22T23:58:01Z

Currently, the tooling and build system for Xcode set some variables to control the Flutter build mode (debug/profile/release) based on the command line options in a generated file (Generated.xcconfig).

This file should not be edited, but it can easily fall out of sync with Add2App scenarios (or if the user is just trying to build and run Runner.xcodeproj directly in Xcode). It can cause several problems (such as inconsistency between Xcode build configuration and Flutter build mode, which local engine is used, or which engine architecture is used). The engine architecture issue in particular (incorrect $FLUTTER_FRAMEWORKS_DIR set from previous run that isn't updated by Xcode) will cause runtime errors that are confusing ("Engine could not start with configuration...").

This patch does the following:

Remove logic to set $FLUTTER_BUILD_MODE and $FLUTTER_FRAMEWORKS_DIR in Generated.xcconfig.
Add a "Profile" Build Configuration to our default Xcode project templates.
Associate the "Profile" build configuration with "Profiling" build (e.g. ⌘I). (Fixes Build in profile mode when built by running Xcode's profiler #15249)
Refactor xcode_backend.sh to drive the $FLUTTER_BUILD_MODE from $CONFIGURATION, unless $FLUTTER_BUILD_MODE is explicitly set (for users with exotic/custom configurations).
Ensure that if $LOCAL_ENGINE is set, it's compatible with the build mode (otherwise error out).
Ensure that if you are trying to run in Profile or Release mode from Xcode on a Simulator, an error will be printed and the build will fail (this already fails when doing flutter run --profile, which is not invoked in Add2App scenarios).

dnfield · 2018-10-23T00:03:15Z

/cc @matthew-carroll - I don't know that the code will be of much interest, but the basic problems may exist on the Android side as well (inconsistency between flutter run ... and how the module chooses to embed the engine library etc.).

dnfield · 2018-10-23T00:05:05Z

This is a more comprehensive solution for what #23309 was trying to solve for (in part).

dnfield · 2018-10-23T02:26:38Z

Breaking change announcement: https://groups.google.com/forum/#!topic/flutter-dev/XEezoVrTaqk

…actor

Hixie · 2018-10-23T03:04:37Z

packages/flutter_tools/bin/xcode_backend.sh

    EchoError "ERROR: Flutter archive builds must be run in Release mode."
    EchoError ""
-    EchoError "To correct, run:"
+    EchoError "To correct, ensure FLUTTER_BUILD_MODE to release or run:"


oh maybe you meant ensure .. is set?

Should be ensure FLUTTER_BUILD_MODE is set to release... are you saying it should use a verb other than ensure?

Hixie · 2018-10-23T03:06:13Z

packages/flutter_tools/lib/src/ios/mac.dart

+    printError('     https://stackoverflow.com/questions/19842746/adding-a-build-configuration-in-xcode');
+    printError('   If you have created a completely custom set of build configurations,');
+    printError('     you can set the FLUTTER_BUILD_MODE=${buildInfo.modeName.toLowerCase()}');
+    printError('     in the .xcconfig file for that configuration and run from Xcode.');


This indentation is a bit weird.

I was trying to avoid a wall of text in the warning - perhaps switching from hanging indent to regular indent would do that?

Changed to add a couple extra new-lines and removed the extra indendtation.

Hixie · 2018-10-23T03:07:03Z

packages/flutter_tools/lib/src/ios/xcodeproj.dart

  }

-  static String _baseConfigurationFor(BuildInfo buildInfo) => buildInfo.isDebug ? 'Debug' : 'Release';
+  static String _baseConfigurationFor(BuildInfo buildInfo) =>  buildInfo.isDebug


nit: extraneous space after =>

but also, generally we prefer to not line-break a => expression; this might be clearer as a block with if statements and returns.

Hixie · 2018-10-23T03:08:28Z

tests?

xster · 2018-10-23T03:15:03Z

packages/flutter_tools/bin/xcode_backend.sh

+  # Use FLUTTER_BUILD_MODE if it's set, otherwise use the Xcode build configuration name
+  # This means that if someone wants to use an Xcode build config other than Debug/Profile/Release,
+  # they _must_ set FLUTTER_BUILD_MODE so we know what type of artifact to build.
+  local build_mode="$(echo "${FLUTTER_BUILD_MODE:-${CONFIGURATION}}" | tr "[:upper:]" "[:lower:]")"


This seems really implicit and would cause issues with flavors https://medium.com/@salvatoregiordanoo/flavoring-flutter-392aaa875f36.

While you're here, I think it would be great if the flavors mechanism could be documented somewhere in https://flutter.io/docs/ if priorities could work. (So you can append to that doc the fact that you if create custom schemes / build configs, a $FLUTTER_BUILD_MODE variable needs to be defined or passed in)

Flavors would already have issues with the current system, but they'd be hidden by the fact that you'd be left with whatever $FLUTTER_BUILD_MODE was set from the last time you ran from the command line. I agree this needs to be documented, but I'm not sure if we can really do much more to help flavors.

dnfield · 2018-10-23T03:15:15Z

I've updated some existing tests to cover these scenarios.

I was considering writing some tests that might exercise xcode_backend.sh with different environment variable configurations to ensure it would fail when it should. I started writing a shell script to do it but that felt messy and not well integrated with our testing framework. I guess I could write something that would shell out to it from Dart with various environment configurations, unless there's a better idea.

dnfield · 2018-10-23T17:56:09Z

Added some tests to test for early exits from the bash script.

Add2App in general needs some integration tests.

xster

LGTM

packages/flutter_tools/bin/xcode_backend.sh

xster · 2018-10-23T21:57:24Z

packages/flutter_tools/bin/xcode_backend.sh

+      EchoError "ERROR: Requested build with Flutter local engine at '${LOCAL_ENGINE}'"
+      EchoError "This engine is not compatible with FLUTTER_BUILD_MODE: '${build_mode}'."
+      EchoError "You can fix this by updating the LOCAL_ENGINE environment variable, or"
+      EchoError "by running flutter build ios --local-engine=..."


Be more specific with the solution. e.g. you need to run --local-engine=ios_profile_unopt or some such if you use build_mode profile.

packages/flutter_tools/bin/xcode_backend.sh

xster · 2018-10-23T22:03:30Z

packages/flutter_tools/lib/src/ios/mac.dart

+    printError('   you can set the FLUTTER_BUILD_MODE=${buildInfo.modeName.toLowerCase()}');
+    printError('   in the .xcconfig file for that configuration and run from Xcode.');
+    printError('');
+    printError('4. Name the newly created configuration ${buildInfo.modeName}.');


Caveat that this step isn't necessary if you followed the 'If you have created a completely custom set of build configurations' part of the previous step.

This reverts commit def1d80.

* Use Xcode build configurations to drive Flutter build mode

This reverts commit def1d80.

dnfield added 5 commits October 22, 2018 15:25

Use Xcode build configurations to drive Flutter build mode

85221f4

Proper check wrt local_engine, print error if profile mode misisng

c84318e

Remove unused code, update tests, fix template problem, update warning

8970afd

fix up warning

d7ca71a

add explanatory dev comment

efe2baa

dnfield requested review from cbracken, chinmaygarde and xster October 22, 2018 23:58

googlebot added the cla: yes label Oct 22, 2018

dnfield added 2 commits October 22, 2018 19:31

fix whitespace

b4b1088

Merge remote-tracking branch 'upstream/master' into xcode_backend_ref…

d8bdc54

…actor

Hixie reviewed Oct 23, 2018

View reviewed changes

missing words, change lambda arrow to function body

11f6e6b

xster reviewed Oct 23, 2018

View reviewed changes

dnfield added 3 commits October 22, 2018 20:36

error indentation

cf6038e

Test early exits for xcode_backend.sh

e24bb9d

only on macOS, use right test

420b2b2

dnfield mentioned this pull request Oct 23, 2018

Update contributing.md for xcode_backend refactor notes flutter/engine#6642

Merged

xster reviewed Oct 23, 2018

View reviewed changes

dnfield added 2 commits October 23, 2018 15:11

Update error messages

90de35a

case insensitive compare for build config

2c8a3db

dnfield removed the request for review from chinmaygarde October 24, 2018 19:47

dnfield merged commit def1d80 into flutter:master Oct 25, 2018

dnfield added a commit that referenced this pull request Oct 25, 2018

Revert "Xcode backend refactor (#23387)"

eed6f95

This reverts commit def1d80.

dnfield mentioned this pull request Oct 25, 2018

Revert "Xcode backend refactor" #23528

Merged

dnfield added a commit that referenced this pull request Oct 25, 2018

Revert "Xcode backend refactor (#23387)" (#23528)

628e8ec

This reverts commit def1d80.

Xavjer pushed a commit to Xavjer/flutter that referenced this pull request Nov 1, 2018

Xcode backend refactor (flutter#23387)

f4891cf

* Use Xcode build configurations to drive Flutter build mode

Xavjer pushed a commit to Xavjer/flutter that referenced this pull request Nov 1, 2018

Revert "Xcode backend refactor (flutter#23387)" (flutter#23528)

f3e1700

This reverts commit def1d80.

neiljaywarner mentioned this pull request Mar 6, 2019

add2app: doc: references merged PR 23387 as not landed #28950

Closed

jmagman mentioned this pull request Jan 18, 2020

Xcode Archive builds should automatically build in release mode #12774

Closed

dnfield mentioned this pull request Feb 29, 2020

Document how to run profile/release from Xcode #51673

Open

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

Xcode backend refactor #23387

Xcode backend refactor #23387

Uh oh!

Conversation

dnfield commented Oct 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfield commented Oct 23, 2018

Uh oh!

dnfield commented Oct 23, 2018

Uh oh!

dnfield commented Oct 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Oct 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield commented Oct 23, 2018

Uh oh!

dnfield commented Oct 23, 2018

Uh oh!

xster left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dnfield commented Oct 22, 2018 •

edited

Loading