Skip to content

Build number (part after +) is documented as optional, use entire app version if not present - #37036

Merged
jmagman merged 3 commits into
flutter:masterfrom
jmagman:build-number
Jul 31, 2019
Merged

jmagman merged 3 commits into
flutter:masterfrom
jmagman:build-number

Conversation

@jmagman

@jmagman jmagman commented Jul 26, 2019

Copy link
Copy Markdown
Member

Description

Generated pubspec says:

A version number is three numbers separated by dots, like 1.2.43 followed by an optional build number separated by a +.

However build name and number were not being parsed unless it contained a "+"

Related Issues

Fixes #34477

See logic introduced in #27743.

Tests

  • flutter_manifest_test 'parses major.minor.patch with no build version'
  • Added 0-prefix case to build_info_test, though that would not have failed before this change

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 the tool Affects the "flutter" command-line tool. See also t: labels. label Jul 26, 2019
@jmagman jmagman added this to the August 2019 milestone Jul 26, 2019
@jmagman
jmagman requested a review from gspencergoog July 26, 2019 21:17
@jmagman jmagman self-assigned this Jul 26, 2019
@jmagman

jmagman commented Jul 26, 2019

Copy link
Copy Markdown
Member Author

@kangwang1988 refactored this a few months ago, let me know if you have any concerns.

if (appVersion != null && appVersion.contains('+')) {
final String value = appVersion.split('+')?.elementAt(1);
return value;
return appVersion.split('+')?.elementAt(1);

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.

Suggested change
return appVersion.split('+')?.elementAt(1);
return appVersion.split('+')[1];

as far as I can tell, split can't return null, so there's no reason to check for it.

return appVersion.split('+')?.elementAt(1);
} else {
return null;
return appVersion;

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.

So, if the app version is "1.0+1", then this function returns "1", but if it's "1.0", then it returns "1.0"?

Seems like you want to return null here, not the appVersion.

manifest: manifest,
expectedAppVersion: '0.0.1',
expectedBuildName: '0.0.1',
expectedBuildNumber: '0.0.1',

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.

Shouldn't this be null or ""?

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.

That it's null now is what's problematic on iOS. The pubspec says build number after the + is optional, but on iOS and macOS, that build number is used as the bundle version, which is required for the app to even run or be submitted to the App Store. So when people have:

version: 1.0.0

the app can't be submitted to the App Store.

Additionally, the bundle version on iOS is a semantic version, not a single number, so it's spiritually closer to the way build name is expected to be formatted. https://developer.apple.com/documentation/bundleresources/information_property_list/cfbundleversion
It's a shame it's overloaded for every platform since they seem to need them to be formatted differently.
Let me narrow the scope to iOS/macOS usage of CFBundleVersion so it doesn't impact other platforms.

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.

It seems odd to me to have a build name of "1.0.0" and a build number (presumably a differentiator for builds with that name) of "1.0.0" as well. What happens if someone switches their app version from "1.0.0" to "1.0.0+1"? Would it switch from a build number of "1.0.0" to "1" on iOS? Could you substitute "0" for the empty string/null on iOS?

@jmagman jmagman Jul 30, 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.

If the build number is optional, as we document in the pubspec, then I (and the people who filed this issue) would expect version: 1.0.0 to use that version in the app's Info.plist. I would find a version of 0 surprising and incorrect, particularly when my iOS users didn't get my version: 2.0.0 upgrade.
What do you suggest? Making the build number required and erroring out if it's not present on an iOS build?

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.

Yes, I agree that if you specify "1.0.0" then it should use that in the info.plist. However, If the value in the info.plist is the combination of the build name and the build number (which is an assumption on my part, maybe I'm confused about that), then it seems weird to combine "1.0.0" and "1.0.0" when I supply "1.0.0" as the app version, and "1.0.0" and "1" in the case where I supply "1.0.0+1" as the app version. I wouldn't expect that, whereas combining "1.0.0" and "0" in the first case seems to be relatively consistent. Although it might be surprising to get "1.0.0-0" instead of "1.0.0" in my info.plist, it's not as surprising as "1.0.0-1.0.0".

How do build number and build name get combined in the info.plist?

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.

Thanks for the complete explanation!

So, before this change, we would only set the CFBundleVersion if there was a build number in the pubspec version string (e.g. "1.0.0+1"), and we'd set CFBundleVersion to the build number (e.g. "1"), but otherwise it'd be null and fail to publish because it didn't have a CFBundleVersion. Am I getting that right?

OK, so when "1.0.0" is supplied, then we should use "1.0.0" for the CFBundleVersion, but if "1.0.0+1" is supplied, we should have a value that is "larger" than "1.0.0", but still has just three dot-separated integers.

Right now, it seems like "1.0.0+1" would just fail our validation in validatedBuildNameForPlatform, because it isn't a valid value for CFBundleVersion, and the validation just removes the "+" instead of converting it to a ".". It's not exactly clear what they mean by "You can include more integers but the system ignores them", but presumably it means additional dot-separated integers, not literally just more integers (e.g. "1.0.01"). That doesn't help us though (we can't do "1.0.0.1") because if it truly ignores it, then it doesn't differentiate those builds.

It seems like the problem with your change will be that if someone uses version "1.0.0" for a release, and then "1.0.0+1" for the second, that in the first release we'll use "1.0.0" for the CFBundleVersion because there's not build number, but shorten that to "1" again for the second release, since it had a build number, and then go back to "2.0.0" for the "2.0.0" version.

We need a different mapping from app version to bundle version, so that it maps to something new (and higher) each time the app version and/or build number is incremented. (e.g. "1.0.0" -> "1.0.10000", "1.0.1" => "1.0.20000", "1.0.0+1" -> "1.0.20001", "1.0.1+5" -> "1.0.20005", "1.0.2+4" -> "1.0.30004", etc.). That might get annoying, since the human visible name and the build version would not match, and (while not a horrible limitation), it would limit the number of valid build numbers to 9999 per release. But maybe something like that would be better?

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.

So, before this change, we would only set the CFBundleVersion if there was a build number in the pubspec version string (e.g. "1.0.0+1"), and we'd set CFBundleVersion to the build number (e.g. "1"), but otherwise it'd be null and fail to publish because it didn't have a CFBundleVersion. Am I getting that right?

Yup!

OK, so when "1.0.0" is supplied, then we should use "1.0.0" for the CFBundleVersion,

Yup!

but if "1.0.0+1" is supplied, we should have a value that is "larger" than "1.0.0", but still has just three dot-separated integers.

No, they can be totally independent and one doesn't need to be larger or smaller than the other. The important thing is that the CFBundleVersion must be larger than the last CFBundleVersion submitted to the App Store.
So:
version: 1.0.0 -> version: 1.0.2
version: 1.0.0+50 -> version: 1.0.1+250
should both be considered valid.

The CFBundleVersion can be incremented as a single number.
In fact, Apple's own agvtool to do this on the command line just increments the CFBundleVersion by one. 1->2->3
It's not necessarily a semantic version, but it can be.
https://developer.apple.com/library/archive/qa/qa1827/_index.html

Also, I believe 1.0.0 and 1 would both be considered equal versions.

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 don't think we need to validate the versions being incremented. The App Store submission should complain if they don't increment correctly. I want the flutter tooling to respect the build number being optional if developers want to keep the two in lock-step, which is also valid. Or if they are distributing it outside the App Store, like as an enterprise app.

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.

OK, but what about the fact that if I specify "1.0.0" the first time and "1.0.0+1" the second, then this code will use "1.0.0" for the first and "1" for the second, which are both equivalent to "1.0.0"?

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.

They would get a "The binary you uploaded was invalid. The key CFBundleVersion in the Info.plist file must contain a higher version than that of the previously uploaded version." error on submission and would have to use the documentation links in the pubspec to do a little research (though of those Apple docs aren't clear). They can try to submit again with "1.0.1" or "1.0.1+2"

I'd like the default to just work, the tool to be flexible enough to accommodate various versioning patterns, and enough info in the documentation to help them out of a jam.

We can't really prevent "1.0.0"-->"1.0.0+1" because flutter tooling has no idea when it gets submitted to the App Store. So if in minute 2 of development they change it to "1.0.0" and minute 3 they change it to "1.0.0+1" and then minute 3 "0.0.1" because they are figuring out how the versions work, the tool doesn't know if they submitted "1.0.0" or not.

@codecov

codecov Bot commented Jul 27, 2019 •

Copy link
Copy Markdown

Codecov Report

Merging #37036 into master will decrease coverage by 0.21%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #37036      +/-   ##
==========================================
- Coverage   54.21%      54%   -0.22%     
==========================================
  Files         191      191              
  Lines       17788    17790       +2     
==========================================
- Hits         9644     9607      -37     
- Misses       8144     8183      +39
Flag Coverage Δ
#flutter_tool 54% <100%> (-0.22%) ⬇️
Impacted Files Coverage Δ
packages/flutter_tools/lib/src/ios/xcodeproj.dart 85.07% <100%> (+0.22%) ⬆️
packages/flutter_tools/lib/src/commands/run.dart 22.96% <0%> (-37.8%) ⬇️
packages/flutter_tools/lib/src/run_cold.dart 0% <0%> (-10.39%) ⬇️
...ages/flutter_tools/lib/src/linux/linux_device.dart 39.13% <0%> (-4.35%) ⬇️
packages/flutter_tools/lib/src/version.dart 90.73% <0%> (-1.96%) ⬇️
.../flutter_tools/lib/src/runner/flutter_command.dart 78.16% <0%> (-1.75%) ⬇️
packages/flutter_tools/lib/src/cache.dart 41.83% <0%> (-0.77%) ⬇️
packages/flutter_tools/lib/src/project.dart 81.84% <0%> (-0.34%) ⬇️
packages/flutter_tools/lib/src/vmservice.dart 37.3% <0%> (+0.15%) ⬆️
packages/flutter_tools/lib/src/base/process.dart 80.46% <0%> (+0.78%) ⬆️
... and 3 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 3068fc4...43a6f13. Read the comment docs.

@jmagman
jmagman requested a review from dnfield July 29, 2019 22:42
@jmagman

jmagman commented Jul 30, 2019

Copy link
Copy Markdown
Member Author

@gspencergoog I narrowed the scope of this change to just iOS.

@jmagman

jmagman commented Jul 31, 2019

Copy link
Copy Markdown
Member Author

@gspencergoog and I spoke in person; we're going to use this solution as a stop-gap until we come up with a proposal to split out the overloaded build number into its different meanings across dart and various platforms.

@jmagman
jmagman merged commit dd1fb3b into flutter:master Jul 31, 2019
@jmagman
jmagman deleted the build-number branch July 31, 2019 23:18
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Aug 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Run flutter plugin On iOS Simulator Failed

3 participants