[Xamarin.Android.Build.Tasks] Implement a new process for defining versionCode.

[dellis1972]

Context https://bugzilla.xamarin.com/show_bug.cgi?id=51620
Context https://bugzilla.xamarin.com/show_bug.cgi?id=51618
Context https://bugzilla.xamarin.com/show_bug.cgi?id=51145

Our current version system for multiple apk's for each Abi
is a bit broken [1]. If a user for example has a versionCode
set which is 123 the final version code for an x86_64 build
ends up as 327803.
This is completely transparent to the user and also does not
follow the guidance in the documentation at [1] and [2].

So we need a new system :) but as usual we have to support the
old system. So we are introducing a new system which is more
flexible. This will only apply when the $(AndroidCreatePackagePerAbi)
is set to True.

The new system has two new properties

<AndroidVersionCodePattern/>
<AndroidVersionCodeProperties/>

The first allows the developer to define the Pattern to be used
for the versonCode. The pattern will be made up of a format string
which will contain keys. These keys will be replaced with values
form one of the known keys or a custom user defined one.

We define a few known key values

• abi : The current target abi converted to an int where

  • 'armeabi' = 1,
  • 'armeabi-v7a' = 2,
  • 'x86' = 3,
  • 'arm64-v8a' = 4,
  • 'x86_64' = 5,

• minSDK : The minSDK value from the manifest or 11 if not present.

• versionCode : The versionCode from the manifest.

With these keys the user can define a pattern of

{abi}{minSDK}{versionCode}

or if they way to include zero padding they can use

{abi}{minSDK}{versionCode:D4}

similar to the left padding formats used in string.Format ().

Users can also use the $(AndroidVersionCodeProperties) property
to define new custom keys. This string will be in the form of a
semi-colon delimited key=value pairs. For example

foo=12;bar=$(SomeBuildProperty)

when can then be used in the pattern.

{abi}{foo}{bar}{versionCode}

Lets work through an example. The user defines a version code of '123'
in the manifest and enables $(AndroidCreatePackagePerAbi). They define
a $(AndroidVersionCodePattern) of {abi}{versionCode:D5}.
This will result in the following version code being produced for the
'x86' build.

300123

The first 3 is the {abi} value. The rest is the left zero padded
versionCode.
A slightly more complex pattern would be {abi}{minSDK:D2}{versionCode:D4}
which would produce

3140123

if the minimumSdk value was set to API 14.

A more real life example mgiht be as follows. A user wants to use the Build value
from the AssemblyInfo.cs . They define the following target

<Target Name="_GetBuild" AfterTargets="Compile">
  <GetAssemblyIdentity AssemblyFiles="Foo.dll">
      <Output
          TaskParameter="Assemblies"
          ItemName="MasterVersion"/>
    </GetAssemblyIdentity>
    <PropertyGroup>
       <BuildVersion>$([System.Version]::Parse(%(MasterVersion.Version)).Build)</BuildVersion>
    </PropertyGroup>
</Target>

This extracts the build version from the built assembly. They can then define a
pattern of

{abi}{minSDK}{build:D4}

and set the properties to

build=$(BuildVersion)

Given similar properties from the previous example e.g abi=x86 and minSDk=14,
this will result in the follwing output (assuming the Build value was 3421).

3143421

[1] https://developer.xamarin.com/guides/android/advanced_topics/build-abi-specific-apks/
[2] https://developer.android.com/google/play/publishing/multiple-apks.html#Rules

[jonpryor]

This should split on both ; and :, or just :.

; can't be specified on the command-line as part of a property value, as it's the property separator:

msbuild /p:Foo=Bar;Baz=Qux

[jonpryor]

Do we really want (?<key>([A-Za-z]*))? That would match no characters, allowing a match of {}. Is that sensible?

Additionally, does the 0* make sense? That looks like it should be any format string, so what would be wrong with e.g. {versionCode:x8}?

[dellis1972]

After thinking a bit more I think we should only support integer format padding strings.. Since the version code MUST be an integer it makes no sense to support anything else.

So we should only support

{0:00000}
{0:D4}

for padding.

[jonpryor]

The problem is that there are multiple format strings which result in an integer, e.g. g, and I don't think that whitelisting is sane.

I'm also not sure that it matters, necessarily? If they specify e.g. x4 and get a letter in the resulting versionCode, then aapt/etc. will complain later about the invalid version. So long as the resulting error message is sensible, it shouldn't matter where it errors, so long as it does.

[jonpryor]

This line shouldn't begin/end with ````` (backtick), unless you want the property to contain backticks...

msbuild '/p:AndroidVersionCodePattern=`{abi}{versionCode}`'

I assume you don't, so this line and the line ~10 lines down likewise shouldn't begin/end with backticks. These lines are already indented extra, which makes them formatted as code.

[jonpryor]

There should be no space between the ] and the (.

[jonpryor]

This should be –, not &ndsh;; note the missing a.

[jonpryor]

This shouldn't contain _s:

See [Creating the Version Code for the APK](https://developer.xamarin.com/guides/android/advanced_topics/build-abi-specific-apks/#Creating_the_Version_Code_for_the_APK)

[jonpryor]

Ditto here; no space between ] and (.

[jonpryor]

This is documented, but I don't see any tests which exercise this property.

[dellis1972]

I think we should restrict it only to "0" and "Dx" as they are the ones that make sense (and document that).

…

On 13 July 2017 at 14:47, Jonathan Pryor ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/Xamarin.Android.Build.Tasks/Utilities/ManifestDocument.cs <#692 (comment)> : > } code |= GetAbiCode (abi) << 16; VersionCode = code.ToString (); } + + public void CalculateVersionCode (string currentAbi, string versionCodePattern, string versionCodeProperties) + { + var regex = new Regex ("\\{(?<key>([A-Za-z]*)):?0*[\\}]"); The problem is that there are multiple format strings which result in an integer, e.g. g, and I don't think that whitelisting is sane. I'm also not sure that it matters, necessarily? If they specify e.g. x4 and get a letter in the resulting versionCode, then aapt/etc. will complain later about the invalid version. So long as the resulting error message is sensible, it shouldn't matter *where* it errors, so long as it does. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#692 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAxeefzM4TshWx7zaDXOEnmZK-CKXFKLks5sNh_ygaJpZM4OV5Cm> .

[jonpryor]

Given that you actually want the minSdkVersion attribute value, why not look for that instead of the element?

var minAttr = doc.Root.Element ("uses-sdk")?.Attribute (androidNs + "minSdkVersion");
if (minAttr == null) {
    ...
}
return minAttr.Value;

[jonpryor]

Perhaps this should be a GetMinimumSdk() method, not a property, given that this property isn't just a field access? It would inform callers that they may want to cache the value instead of calling the property repeatedly.

[jonpryor]

This should be:

...will produce [sic] a versionCode of 11123 when $(AndroidCreatePackagePerAbi) is True, otherwise will produce a value of 123.

[jonpryor]

This similarly should mention $(AndroidCreatePackagePerAbi).

[jonpryor]

Looks like a prior documentation repro which moved this to be a list instead of a table broke the hyperlinks, or, hyperlinks now require post-processing/hosting on developer.xamarin.com

Specifically, this link doesn't go anywhere when simply rendered as HTML.

Perhaps this should add <a name="..." /> so that these links work?