Repository navigation
Build number (part after +) is documented as optional, use entire app version if not present - #37036
Conversation
… version is not present
|
@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); |
There was a problem hiding this comment.
| 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; |
There was a problem hiding this comment.
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', |
There was a problem hiding this comment.
Shouldn't this be null or ""?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"?
There was a problem hiding this comment.
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 Report
@@ 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
Continue to review full report at Codecov.
|
|
@gspencergoog I narrowed the scope of this change to just iOS. |
|
@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. |
Description
Generated pubspec says:
However build name and number were not being parsed unless it contained a "+"
Related Issues
Fixes #34477
See logic introduced in #27743.
Tests
Checklist
///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change