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 backend refactor #23387

Merged
merged 13 commits into from Oct 25, 2018

Conversation

Projects
None yet
4 participants
@dnfield
Copy link
Member

commented Oct 22, 2018

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:

  1. Remove logic to set $FLUTTER_BUILD_MODE and $FLUTTER_FRAMEWORKS_DIR in Generated.xcconfig.
  2. Add a "Profile" Build Configuration to our default Xcode project templates.
  3. Associate the "Profile" build configuration with "Profiling" build (e.g. ⌘I). (Fixes #15249)
  4. 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).
  5. Ensure that if $LOCAL_ENGINE is set, it's compatible with the build mode (otherwise error out).
  6. 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 dnfield requested review from chinmaygarde, xster and cbracken Oct 22, 2018

@googlebot googlebot added the cla: yes label Oct 22, 2018

@dnfield

This comment has been minimized.

Copy link
Member Author

commented Oct 23, 2018

/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

This comment has been minimized.

Copy link
Member Author

commented Oct 23, 2018

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

@dnfield

This comment has been minimized.

Copy link
Member Author

commented Oct 23, 2018

dnfield added some commits Oct 23, 2018

EchoError "========================================================================"
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:"

This comment has been minimized.

Copy link
@Hixie

Hixie Oct 23, 2018

Contributor

"ensure"?

This comment has been minimized.

Copy link
@Hixie

Hixie Oct 23, 2018

Contributor

oh maybe you meant ensure .. is set?

This comment has been minimized.

Copy link
@dnfield

dnfield Oct 23, 2018

Author Member

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

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 comment has been minimized.

Copy link
@Hixie

Hixie Oct 23, 2018

Contributor

This indentation is a bit weird.

This comment has been minimized.

Copy link
@dnfield

dnfield Oct 23, 2018

Author Member

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

This comment has been minimized.

Copy link
@dnfield

dnfield Oct 23, 2018

Author Member

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

@@ -274,7 +264,11 @@ class XcodeProjectInfo {
});
}

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

This comment has been minimized.

Copy link
@Hixie

Hixie Oct 23, 2018

Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2018

tests?

# 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 comment has been minimized.

Copy link
@xster

xster Oct 23, 2018

Contributor

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)

This comment has been minimized.

Copy link
@dnfield

dnfield Oct 23, 2018

Author Member

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

This comment has been minimized.

Copy link
Member Author

commented Oct 23, 2018

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 added some commits Oct 23, 2018

@dnfield

This comment has been minimized.

Copy link
Member Author

commented Oct 23, 2018

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

Add2App in general needs some integration tests.

@xster
Copy link
Contributor

left a comment

LGTM

Show resolved Hide resolved 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=..."

This comment has been minimized.

Copy link
@xster

xster Oct 23, 2018

Contributor

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.

Show resolved Hide resolved packages/flutter_tools/bin/xcode_backend.sh
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}.');

This comment has been minimized.

Copy link
@xster

xster Oct 23, 2018

Contributor

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.

dnfield added some commits Oct 23, 2018

@dnfield dnfield removed the request for review from chinmaygarde Oct 24, 2018

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

23 checks passed

WIP Ready for review
Details
analyze Task Summary
Details
analyze
Details
aot_build_tests-linux Task Summary
Details
aot_build_tests-linux
Details
cla/google All necessary CLAs are signed
codelabs-build-test Task Summary
Details
codelabs-build-test
Details
docs Task Summary
Details
docs
Details
flutter-build
tests-linux Task Summary
Details
tests-linux
Details
tests-macos Task Summary
Details
tests-macos
Details
tests-windows Task Summary
Details
tests-windows
Details
tool_tests-linux Task Summary
Details
tool_tests-linux
Details
tool_tests-macos Task Summary
Details
tool_tests-macos
Details
tool_tests-windows Task Summary
Details
tool_tests-windows
Details

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

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

johnsonmh added a commit to johnsonmh/flutter that referenced this pull request Oct 26, 2018

Xcode backend refactor (flutter#23387)
* Use Xcode build configurations to drive Flutter build mode

johnsonmh added a commit to johnsonmh/flutter that referenced this pull request Oct 26, 2018

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

Xcode backend refactor (flutter#23387)
* Use Xcode build configurations to drive Flutter build mode

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

GaryQian added a commit to GaryQian/flutter that referenced this pull request Nov 1, 2018

Xcode backend refactor (flutter#23387)
* Use Xcode build configurations to drive Flutter build mode

GaryQian added a commit to GaryQian/flutter that referenced this pull request Nov 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.