Skip to content

Build xcarchive command #67598

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.

Sign up for GitHub

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

Jump to bottom
Merged
jmagman merged 3 commits into from
Oct 9, 2020
Merged

Build xcarchive command #67598

jmagman merged 3 commits into from
Oct 9, 2020

Conversation

@jmagman
Copy link
Member

@jmagman jmagman commented Oct 8, 2020 •
edited
Loading

Description

Create a new flutter build xcarchive command. It's close to the same as flutter build ios but with archive tacked onto the end of the xcodebuild command.

Related Issues

Fixes #66619
Part of #13065
Follow up documentation at flutter/website#4852.

Tests

build_xcarchive_test, added a check to ios_content_validation_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 8, 2020
@jmagman jmagman self-assigned this Oct 8, 2020
@flutter-dashboard flutter-dashboard bot added c: contributor-productivity Team-specific productivity, code health, technical debt. tool Affects the "flutter" command-line tool. See also t: labels. labels Oct 8, 2020
@google-cla google-cla bot added the cla: yes label Oct 8, 2020
jmagman
jmagman commented Oct 8, 2020
View reviewed changes
packages/flutter_tools/lib/src/commands/build_ios.dart Outdated
Copy link
Member Author

@jmagman jmagman Oct 8, 2020

Choose a reason for hiding this comment

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

Archive also doesn't need config-only, simulator, or codesign flags.

packages/flutter_tools/lib/src/ios/mac.dart Outdated
Copy link
Member Author

@jmagman jmagman Oct 8, 2020

Choose a reason for hiding this comment

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

This hunk is mostly whitespace changes. The copy logic and watch companion adjustments were moved into the buildAction == XcodeBuildAction.build condition.

packages/flutter_tools/lib/src/ios/mac.dart Outdated
Comment on lines +442 to +445
Copy link
Member Author

@jmagman jmagman Oct 8, 2020

Choose a reason for hiding this comment

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

This bit is new.

Copy link
Contributor

@jonahwilliams jonahwilliams Oct 8, 2020

Choose a reason for hiding this comment

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

What circumstance would lead to the archive missing but no build errors?

Copy link
Member Author

@jmagman jmagman Oct 8, 2020

Choose a reason for hiding this comment

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

I don't think it should happen, but we have the same check right above it for a missing Runner.app on build.
That was introduced in #14663 but I don't quite know why, it wasn't present in the previous versions that were reverted. Possibly related to #14661?

Copy link
Contributor

@jonahwilliams jonahwilliams Oct 8, 2020

Choose a reason for hiding this comment

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

FYI @xster do you have any more context here?

packages/flutter_tools/test/commands.shard/hermetic/build_xcarchive_test.dart Outdated
Copy link
Member Author

@jmagman jmagman Oct 8, 2020

Choose a reason for hiding this comment

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

Mostly cribbed from build_macos_test.dart

jonahwilliams
jonahwilliams reviewed Oct 8, 2020
View reviewed changes
packages/flutter_tools/lib/src/ios/mac.dart Outdated
Copy link
Contributor

@jonahwilliams jonahwilliams Oct 8, 2020

Choose a reason for hiding this comment

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

nit, document a little bit

Copy link
Contributor

@jonahwilliams jonahwilliams Oct 8, 2020

Choose a reason for hiding this comment

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

also, if you never switch over this consider using String constants

Copy link
Member Author

@jmagman jmagman Oct 8, 2020

Choose a reason for hiding this comment

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

I prefer comparing enums to strings whether it's in a switch or an if conditional, strings are for human readability (and for wrapping a bunch of shelled out commands, as we do...). It also adds some pseudo type checking when being passed around. And not that it matters in this context, but the comparison is faster (at least in most languages, maybe dart does something as fast with a const string?). The only time when you have to think about it a bit is when you're serializing it to make sure if you insert one into the middle of the list it doesn't change the order of the enumeration symbols. But I'd rather serialize an int than a string in that case anyway.

dart-lang/language#158 would be nice.

Copy link
Contributor

@jonahwilliams jonahwilliams Oct 8, 2020

Choose a reason for hiding this comment

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

I don't really think the performance will matter for this code, but ack on the usability.

@jmagman jmagman mentioned this pull request Oct 8, 2020
Merged
@jmagman jmagman force-pushed the xcarchive branch 2 times, most recently from 11022d8 to a06832e Compare October 8, 2020 23:27
jonahwilliams
jonahwilliams approved these changes Oct 9, 2020
View reviewed changes
Copy link
Contributor

@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

Can you create a follow up ticket for website documentation?

@jmagman jmagman mentioned this pull request Oct 9, 2020
Closed
@jmagman
Copy link
Member Author

jmagman commented Oct 9, 2020

Can you create a follow up ticket for website documentation?

flutter/website#4852

@jmagman jmagman merged commit 98aeef2 into flutter:master Oct 9, 2020
@jmagman jmagman deleted the xcarchive branch October 9, 2020 02:51
This was referenced Oct 9, 2020
Closed
Merged
Closed
@jmagman jmagman mentioned this pull request Dec 15, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@christopherfujino christopherfujino Awaiting requested review from christopherfujino

1 more reviewer

@jonahwilliams jonahwilliams jonahwilliams approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jmagman jmagman

Labels

c: contributor-productivity Team-specific productivity, code health, technical debt. 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add "flutter build xcarchive" command"

2 participants

@jmagman @jonahwilliams