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

Build IPA command #67781

Merged
merged 2 commits into from
Oct 22, 2020
Merged

Build IPA command #67781

merged 2 commits into from
Oct 22, 2020

Conversation

jmagman
Copy link
Member

@jmagman jmagman commented Oct 9, 2020

Description

Add a parameter to flutter build ipa to pass to xcodebuild -exportArchive to generate an IPA from an xcarchive.

Related Issues

Fixes #13065
#67598
Follow up documentation at flutter/website#4852

Tests

build_ipa_test

Checklist

  • 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

  • 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.

@jmagman jmagman added platform-ios iOS applications specifically t: xcode "xcodebuild" on iOS and general Xcode project management labels Oct 9, 2020
@jmagman jmagman self-assigned this Oct 9, 2020
@flutter-dashboard flutter-dashboard bot added tool Affects the "flutter" command-line tool. See also t: labels. work in progress; do not review labels Oct 9, 2020
@google-cla google-cla bot added the cla: yes label Oct 9, 2020
@jmagman jmagman force-pushed the build-ipa branch 2 times, most recently from e1b3888 to 37a3482 Compare October 9, 2020 23:30
@jmagman jmagman force-pushed the build-ipa branch 3 times, most recently from 0bd9040 to 792152d Compare October 13, 2020 01:28
@jmagman jmagman marked this pull request as ready for review October 13, 2020 02:19
@jonahwilliams
Copy link
Member

Requiring users to run flutter build xcarchive and then flutter build ipa could enable some mistakes if users aren't careful. i.e. If the archive is not up to date, could users accidentally upload a stale build?

Similarly, if all CI build scripts would need to be something like flutter build xcarchive && flutter build ipa, then perhaps flutter build ipa would imply the archiving?

processManager: FakeProcessManager.any(),
);

await expectToolExitLater(
Copy link
Member

Choose a reason for hiding this comment

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

Til about expectToolExitLater

@jmagman
Copy link
Member Author

jmagman commented Oct 13, 2020

Requiring users to run flutter build xcarchive and then flutter build ipa could enable some mistakes if users aren't careful. i.e. If the archive is not up to date, could users accidentally upload a stale build?

I was taking cues from install here, it has similar problems of staleness, it bails if the app binary is missing, and prompts the user to run another flutter command:

_logger.printError('Could not find application bundle at ${bundle.path}; have you run "flutter build ios"?');

I could update this to run xcarchive if the archive isn't in the expected spot, though it doesn't seem that big a deal to prompt the user to run the right command (with all the flavor, dart, etc flags they need).

I was thinking of this command is a thin convenience wrapper on top of an xcodebuild command now that flutter build xcarchive exists.

@jonahwilliams
Copy link
Member

install probably isn't the best designed flutter command though

@jonahwilliams
Copy link
Member

jonahwilliams commented Oct 13, 2020

One way you could support this as either the primary development command or a wrapper is allow the user to provide a --path-to-xcarchive (is this customizable?) and then use it as is if provided.

@@ -385,6 +385,11 @@ class BuildableIOSApp extends IOSApp {
String get archiveBundlePath
=> globals.fs.path.join(getIosBuildDirectory(), 'archive', globals.fs.path.withoutExtension(_hostAppBundleName));
Copy link
Member

Choose a reason for hiding this comment

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

slightly tangential: at some point we might want to take stock of all the "temporary but user accessible" build artifacts mixed with build intermediaries in the build/ folder and organize them.

Right now I have

build
├── 3f44400bd8f8c7820069645ed94a731d
├── app
│   ├── generated
│   │   ├── ap_generated_sources
│   │   ├── not_namespaced_r_class_sources
│   │   ├── res
│   │   └── source
│   ├── intermediates
│   │   ├── annotation_processor_list
│   │   ├── apk_list
│   │   ├── blame
│   │   ├── bundle_manifest
│   │   ├── check_manifest_result
│   │   ├── compatible_screen_manifest
│   │   ├── dex
│   │   ├── duplicate_classes_check
│   │   ├── external_file_lib_dex_archives
│   │   ├── external_libs_dex
│   │   ├── flutter
│   │   ├── generated_proguard_file
│   │   ├── incremental
│   │   ├── instant_app_manifest
│   │   ├── javac
│   │   ├── lint_jar
│   │   ├── manifest_merge_blame_file
│   │   ├── merged-not-compiled-resources
│   │   ├── merged_assets
│   │   ├── merged_java_res
│   │   ├── merged_jni_libs
│   │   ├── merged_manifests
│   │   ├── merged_native_libs
│   │   ├── merged_shaders
│   │   ├── metadata_feature_manifest
│   │   ├── processed_res
│   │   ├── proguard-files
│   │   ├── proguard-rules
│   │   ├── res
│   │   ├── res_stripped
│   │   ├── shader_assets
│   │   ├── signing_config
│   │   ├── stripped_native_libs
│   │   ├── symbols
│   │   ├── transforms
│   │   └── validate_signing_config
│   ├── outputs
│   │   ├── apk
│   │   ├── flutter-apk
│   │   ├── logs
│   │   └── mapping
│   ├── reports
│   └── tmp
│       ├── compileDebugJavaWithJavac
│       ├── compileProfileJavaWithJavac
│       ├── compileReleaseJavaWithJavac
│       ├── packLibsflutterBuildDebug
│       ├── packLibsflutterBuildProfile
│       └── packLibsflutterBuildRelease
├── fa37e11e55641aa14c7c94ec233f11be
└── ios
    ├── Debug-iphoneos
    │   └── Runner.app
    ├── Profile-iphoneos
    │   ├── Runner.app
    │   └── Runner.app.dSYM
    ├── Runner.build
    │   └── Release-iphoneos
    └── iphoneos
        └── Runner.app

without counting ios-frameworks, aars etc. We don't have to fix anything here but it's a question for the next comment.

packages/flutter_tools/lib/src/application_package.dart Outdated Show resolved Hide resolved
'export-options-plist',
valueHelp: 'ExportOptions.plist',
help:
'Path to the "-exportOptionsPlist" file passed to Xcode. See "man xcodebuild" for available keys.',
Copy link
Member

Choose a reason for hiding this comment

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

For first timers (which we will have), we might need to link to a Flutter page to describe what it is. Since we'll certainly have Flutter developers shipping for the first time to the app store without having shipped an ios app in the past.

Feel free to come back in a later PR too instead of leaving a placeholder.

Copy link
Member Author

Choose a reason for hiding this comment

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

For sure, we'll need to point to some docs. But we can pretty much just point to Apple's docs since this command is a really thin wrapper and doesn't process any plist values.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a TODO to update the new option help text.

packages/flutter_tools/lib/src/commands/build_ipa.dart Outdated Show resolved Hide resolved
final FileSystemEntityType type = _fileSystem.typeSync(exportOptionsPlist);
if (type == FileSystemEntityType.notFound) {
throwToolExit(
'"$exportOptionsPlist" property list does not exist. See "man xcodebuild" for available keys.');
Copy link
Member

Choose a reason for hiding this comment

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

I don't actually see the available keys in the manual (which is really long). We might need to do a bit more hand-holding for pure-flutter users.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, it's in the xcrun xcodebuild -h but not the man page.

packages/flutter_tools/lib/src/commands/build_ipa.dart Outdated Show resolved Hide resolved
packages/flutter_tools/lib/src/commands/build_ipa.dart Outdated Show resolved Hide resolved
packages/flutter_tools/lib/src/commands/build_ipa.dart Outdated Show resolved Hide resolved
@jmagman
Copy link
Member Author

jmagman commented Oct 20, 2020

I think I'm being too pedantic differentiating between building xcarchive and ipa. I am going to combine the commands so it's not a two-step that can get out of sync.

@xster
Copy link
Member

xster commented Oct 20, 2020

+1. We should replicate Apple's capabilities but we don't have to replicate their exact APIs if there are opportunities to make it clearer.

@jmagman
Copy link
Member Author

jmagman commented Oct 22, 2020

Reworked this based on feedback.

Renamed flutter build xcarchive to flutter build ipa (but aliased to xcarchive because I like it better and already said it was happening). Add a --export-options-plist option that takes the .xcarchive bundle that was just built (with all the flavor, dart flags, build info, etc) and additionally creates an IPA out of it, so it's no longer two commands.

I will update the publishing iOS docs when this lands and update the help text to link to something more useful than xcodebuild -h.

Copy link
Member

@xster xster left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -56,14 +57,27 @@ class BuildIOSCommand extends _BuildIOSSubCommand {
bool get shouldCodesign => boolArg('codesign');
}

/// Builds an .xcarchive for an iOS app to be generated for App Store submission.
/// Builds an .xcarchive and optionally .ipa for an iOS app to be generated for
Copy link
Member

Choose a reason for hiding this comment

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

sounds like it's the other way around now?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it always builds an .xcarchive, and only builds an IPA if you pass in the plist flag.

final FileSystemEntityType type = globals.fs.typeSync(exportOptionsPlist);
if (type == FileSystemEntityType.notFound) {
throwToolExit(
'"$exportOptionsPlist" property list does not exist. See "man xcodebuild" for available keys.');
Copy link
Member

Choose a reason for hiding this comment

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

The second sentence here and below seem like non sequiturs? I assume you're just trying to say file x you specified doesn't exist?

Also be consistent with man xcodebuild vs xcodebuild -h

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry I saw this after it was merged. Will follow up.

Copy link
Member Author

Choose a reason for hiding this comment

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

// Error Domain=IDEFoundationErrorDomain Code=1 "exportOptionsPlist error for key 'method': expected one of {app-store, ad-hoc, enterprise, development, validation}, but found developmentasdasd" ...
LineSplitter.split(result.stderr)
.where((String line) => line.contains('error: '))
.forEach(errorMessage.writeln);
Copy link
Member

Choose a reason for hiding this comment

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

nice use of tear-offs!

Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

@shinriyo
Copy link

Woooow!!! nice feature! THANK YOU!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform-ios iOS applications specifically t: xcode "xcodebuild" on iOS and general Xcode project management tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create iOS IPA from command line
4 participants