Skip to content

Warn when build number and version can't be parsed on iOS - #40611

Merged
jmagman merged 3 commits into
flutter:masterfrom
jmagman:build-number
Sep 18, 2019
Merged

Warn when build number and version can't be parsed on iOS#40611
jmagman merged 3 commits into
flutter:masterfrom
jmagman:build-number

Conversation

@jmagman

@jmagman jmagman commented Sep 16, 2019

Copy link
Copy Markdown
Member

Description

Build number and build version are required on iOS. Fall back to default values if one can't be parsed from the pubspec version, but warn that they need to set one before submitting to the App Store.

Related Issues

Fixes #38610.
See also #37036.

Tests

Updated xcodeproj_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 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

  • Yes, this is a breaking change (Please read [Handling breaking changes]). Replace this with a link to the e-mail where you asked for input on this proposed change.
  • No, this is not a breaking change.

@jmagman jmagman added platform-ios iOS applications specifically tool Affects the "flutter" command-line tool. See also t: labels. labels Sep 16, 2019
@jmagman jmagman self-assigned this Sep 16, 2019
@jmagman jmagman changed the title Warn when build number can't be parsed Warn when build number and version can't be parsed on iOS Sep 16, 2019
@jmagman jmagman mentioned this pull request Sep 16, 2019
Closed
@codecov

codecov Bot commented Sep 16, 2019 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #40611 into master will increase coverage by 0.83%.
The diff coverage is 87.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #40611      +/-   ##
==========================================
+ Coverage   58.53%   59.36%   +0.83%     
==========================================
  Files         192      192              
  Lines       18672    18683      +11     
==========================================
+ Hits        10929    11092     +163     
+ Misses       7743     7591     -152
Flag Coverage Δ
#flutter_tool 59.36% <87.5%> (+0.83%) ⬆️
Impacted Files Coverage Δ
packages/flutter_tools/lib/src/build_info.dart 76.75% <100%> (-0.3%) ⬇️
packages/flutter_tools/lib/src/ios/xcodeproj.dart 88.2% <85.71%> (+0.03%) ⬆️
...ges/flutter_tools/lib/src/build_runner/web_fs.dart 0.57% <0%> (-22.86%) ⬇️
...ackages/flutter_tools/lib/src/commands/daemon.dart 17.71% <0%> (-16.32%) ⬇️
packages/flutter_tools/lib/src/version.dart 91.03% <0%> (-1.42%) ⬇️
packages/flutter_tools/lib/src/cache.dart 45.31% <0%> (-0.67%) ⬇️
packages/flutter_tools/lib/src/device.dart 59.41% <0%> (ø) ⬆️
packages/flutter_tools/lib/src/project.dart 83.91% <0%> (+0.31%) ⬆️
.../flutter_tools/lib/src/fuchsia/fuchsia_device.dart 55.39% <0%> (+0.37%) ⬆️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63e096b...c3a7322. Read the comment docs.

stuartmorgan-g
stuartmorgan-g approved these changes Sep 16, 2019
View reviewed changes

@stuartmorgan-g stuartmorgan-g left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM; one optional nit.

Comment thread packages/flutter_tools/lib/src/ios/xcodeproj.dart Outdated

@stuartmorgan-g stuartmorgan-g Sep 16, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is is useful to handle this separately? It seems like making the below non-else ifs and just having two lines of output would be as easy to understand, and less code.

@jmagman jmagman Sep 17, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I had that at first because it was much easier to write, but I decided to combine it to give a nicer message. Here's the difference in output. Current code:

Signing iOS app for device deployment using developer identity: "..."
Warning: Missing build name (CFBundleShortVersionString), defaulting to 1.0.0.
Missing build number (CFBundleVersion), defaulting to 1.
Set a build name and number in the pubspec.yaml version before submitting to the App Store.

Screen Shot 2019-09-16 at 5 20 52 PM

vs your suggestion:

Signing iOS app for device deployment using developer identity: "..."
Warning: Missing build name (CFBundleShortVersionString), defaulting to 1.0.0.
Set a build name in the pubspec.yaml version before submitting to the App Store.
Warning: Missing build number (CFBundleVersion), defaulting to 1.
Set a build number in the pubspec.yaml version before submitting to the App Store.

Screen Shot 2019-09-16 at 5 25 28 PM

What do you think?

@stuartmorgan-g stuartmorgan-g Sep 17, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I actually prefer the second; I think it makes it clearer that there are two distinct issues that need to be resolved. It's clearly subjective though, so your call.

@jmagman jmagman Sep 17, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@zanderso Do you have a preference?

@zanderso zanderso Sep 17, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How about:

if (buildNameIsMissing) {
  printError('Warning: Missing build name (CFBundleShortVersionString), defaulting to $defaultBuildName.');
}
if (buildNumberIsMissing) {
  printError('Warning: Missing build number (CFBundleVersion), defaulting to $defaultBuildNumber.\n');
}
if (buildNameIsMissing || buildNumberIsMissing) {
  printError('Action Required: You must set a build name and number in the pubspec.yaml '
             'file before submitting to the App Store.');
}

@jmagman jmagman Sep 17, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sounds good. I added "version field" just so they knew how to add the name and number:

    printError('Action Required: You must set a build name and number in the pubspec.yaml '
               'file version field before submitting to the App Store.');

@jmagman jmagman mentioned this pull request Sep 17, 2019
Closed
@jmagman
jmagman merged commit 9a68b0d into flutter:master Sep 18, 2019
@jmagman
jmagman deleted the build-number branch September 18, 2019 00:07
This was referenced Sep 18, 2019
Closed
Merged
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Sep 30, 2019
@jmagman @Inconnu08
Warn when build number and version can't be parsed on iOS (flutter#40611
5184bf9 
)
This was referenced Oct 4, 2019
Closed
Closed
@jmagman jmagman mentioned this pull request Nov 21, 2019
Closed
This was referenced Jan 15, 2020
Closed
Open
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Aug 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stuartmorgan-g stuartmorgan-g stuartmorgan-g approved these changes
@zanderso zanderso zanderso left review comments

Assignees

@jmagman jmagman

Labels

platform-ios iOS applications specifically 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.

Crash on compiling iPhone when missing pubspec.yaml version: xcrun exited abnormally

4 participants

@jmagman @stuartmorgan-g @zanderso @googlebot