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 number (part after +) is documented as optional, use entire app version if not present #37036

Merged
merged 3 commits into from Jul 31, 2019

Conversation

@jmagman
Copy link
Member

commented Jul 26, 2019

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.
… version is not present
@jmagman jmagman added the tool label Jul 26, 2019
@jmagman jmagman added this to the August 2019 milestone Jul 26, 2019
@jmagman jmagman requested a review from gspencergoog Jul 26, 2019
@googlebot googlebot added the cla: yes label Jul 26, 2019
@jmagman jmagman self-assigned this Jul 26, 2019
@jmagman

This comment has been minimized.

Copy link
Member Author

commented Jul 26, 2019

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

@@ -108,10 +108,9 @@ class FlutterManifest {
/// Can be null if version isn't set or has a wrong format.
String get buildNumber {
if (appVersion != null && appVersion.contains('+')) {
final String value = appVersion.split('+')?.elementAt(1);
return value;
return appVersion.split('+')?.elementAt(1);

This comment has been minimized.

Copy link
@gspencergoog

gspencergoog Jul 26, 2019

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

} else {
return null;
return appVersion;

This comment has been minimized.

Copy link
@gspencergoog

gspencergoog Jul 26, 2019

Contributor

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',

This comment has been minimized.

Copy link
@gspencergoog

gspencergoog Jul 26, 2019

Contributor

Shouldn't this be null or ""?

This comment has been minimized.

Copy link
@jmagman

jmagman Jul 27, 2019

Author Member

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.

This comment has been minimized.

Copy link
@gspencergoog

gspencergoog Jul 30, 2019

Contributor

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?

This comment has been minimized.

Copy link
@jmagman

jmagman Jul 30, 2019

Author Member

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?

This comment has been minimized.

Copy link
@gspencergoog

gspencergoog Jul 30, 2019

Contributor

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?

This comment has been minimized.

Copy link
@gspencergoog

gspencergoog Jul 30, 2019

Contributor

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?

This comment has been minimized.

Copy link
@jmagman

jmagman Jul 30, 2019

Author Member

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.

This comment has been minimized.

Copy link
@jmagman

jmagman Jul 30, 2019

Author Member

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.

This comment has been minimized.

Copy link
@gspencergoog

gspencergoog Jul 31, 2019

Contributor

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"?

This comment has been minimized.

Copy link
@jmagman

jmagman Jul 31, 2019

Author Member

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

This comment has been minimized.

Copy link

commented Jul 27, 2019

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 Jul 29, 2019
@jmagman

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2019

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

@jmagman

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2019

@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
77 checks passed
77 checks passed
WIP Ready for review
Details
add2app-macos Task Summary
Details
add2app-macos
Details
analyze Task Summary
Details
analyze
Details
build_tests-linux Task Summary
Details
build_tests-linux
Details
build_tests-macos Task Summary
Details
build_tests-macos
Details
build_tests-windows Task Summary
Details
build_tests-windows
Details
cla/google All necessary CLAs are signed
codecov/patch 100% of diff hit (target 54.21%)
Details
codecov/project Absolute coverage decreased by -0.21% but relative coverage increased by +45.78% compared to 3068fc4
Details
customer_testing-linux Task Summary
Details
customer_testing-linux
Details
customer_testing-macos Task Summary
Details
customer_testing-macos
Details
customer_testing-windows Task Summary
Details
customer_testing-windows
Details
deploy_gallery Task Summary
Details
deploy_gallery
Details
deploy_gallery-macos Task Summary
Details
deploy_gallery-macos
Details
docs Task Summary
Details
docs
Details
flutter-build
Details
integration_tests-linux Task Summary
Details
integration_tests-linux
Details
integration_tests-macos Task Summary
Details
integration_tests-macos
Details
integration_tests-windows Task Summary
Details
integration_tests-windows
Details
integration_tests_gradle1-linux Task Summary
Details
integration_tests_gradle1-linux
Details
integration_tests_gradle2-linux Task Summary
Details
integration_tests_gradle2-linux
Details
release_smoke_tests Task Summary
Details
release_smoke_tests
Details
tests_extras-linux Task Summary
Details
tests_extras-linux
Details
tests_extras-macos Task Summary
Details
tests_extras-macos
Details
tests_extras-windows Task Summary
Details
tests_extras-windows
Details
tests_framework_other-linux Task Summary
Details
tests_framework_other-linux
Details
tests_framework_other-macos Task Summary
Details
tests_framework_other-macos
Details
tests_framework_other-windows Task Summary
Details
tests_framework_other-windows
Details
tests_widgets-linux Task Summary
Details
tests_widgets-linux
Details
tests_widgets-macos Task Summary
Details
tests_widgets-macos
Details
tests_widgets-windows Task Summary
Details
tests_widgets-windows
Details
tool_coverage-linux Task Summary
Details
tool_coverage-linux
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
tool_tests_create-linux Task Summary
Details
tool_tests_create-linux
Details
tool_tests_create-macos Task Summary
Details
tool_tests_create-macos
Details
tool_tests_create-windows Task Summary
Details
tool_tests_create-windows
Details
tool_tests_integration-linux Task Summary
Details
tool_tests_integration-linux
Details
tool_tests_integration-macos Task Summary
Details
tool_tests_integration-macos
Details
tool_tests_integration-windows Task Summary
Details
tool_tests_integration-windows
Details
@jmagman jmagman deleted the jmagman:build-number branch Jul 31, 2019
@jmagman jmagman referenced this pull request Sep 16, 2019
8 of 9 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.