[build] Use $(AndroidPlatformId) when appropriate

[jonpryor]

Context: 485e39b
Context: eb08bb4
...and doubtless others...

Three MSBuild properties control the android.jar which is bound and
the $(TargetFrameworkVersion) of Mono.Android.dll:

• $(AndroidApiLevel): The API level that is bound. Must be an int.
• $(AndroidFrameworkVersion): The $(TargetFrameworkVersion) of
  the generated Mono.Android.dll. Must be mostly parseable by
  System.Version except with a leading v, e.g. v10.0.
• $(AndroidPlatformId): The "ID" of the API level.

Most of the time, $(AndroidApiLevel) and $(AndroidPlatformId)
will be identical: for API-29, they're both 29.

Where they differ is for new preview API levels, such as API-R:
$(AndroidApiLevel) will be 30, but $(AndroidPlatformId) is R.
The distinction is important because various filesystem paths within
the Android SDK use the "id" and not the API level when they differ,
e.g. the API-R android.jar is installed into:

    $(AndroidSdkDirectory)/platforms/android-R/android.jar

We thus need to be careful when distinguishing between
$(AndroidApiLevel) and $(AndroidPlatformId), using the former when
an integer is required, and using the latter whenever it refers to
filesystem paths.

Unfortunately, we haven't been careful, because these values really
only differ for ~4 months out of the year, and for only one
$(TargetFrameworkVersion) version.

Start bringing some sanity...and finding bugs while we do so:

api-xml-adjuster.targets should use %(AndroidApiInfo.Id) and not
%(AndroidApiLevel.Level), as it references filesystem locations.
Consequently, src/Mono.Android/Profiles/api-30.params.txt must be
renamed to src/Mono.Android/Profiles/api-R.params.txt so that it
correctly embeds the $(AndroidPlatformId) value.

Mono.Android.targets should likewise use $(AndroidPlatformId) and
not $(AndroidApiLevel) when using filesystem paths from the SDK.

For good measure, Mono.Android.csproj now overrides
$(IntermediateOutputPath) to contain $(AndroidPlatformId), because
why not (MOAR CONSISTENCY!).

These changes, unfortunately, introduce breakage, which will need to
be addressed:

Because API-R was installed into
$(AndroidSdkDirectory)/platforms/android-R, api-versions.xml
was not previously used because Mono.Android.targets was using
$(AndroidApiLevel), and platforms/android-30/data/api-version.xml
does not yet exist. (It will come June! But not now.) As it didn't
exist, it hit the fallback path and used
platform-tools/api/api-versions.xml (4cd2060). You would think
this wouldn't be a problem, but the API-R api-versions.xml is
missing members relative to platform-tools, resulting in members
missing RegisterAttribute.ApiSince values, which
Microsoft.DotNet.ApiCompat.exe reports, e.g.:

    CannotChangeAttribute : Attribute 'Android.Runtime.RegisterAttribute' on 'Java.Lang.StringBuilder.TrimToSize()' changed from '[RegisterAttribute("trimToSize", "()V", "", ApiSince=9)]' in the contract to '[RegisterAttribute("trimToSize", "()V", "")]' in the implementation

dotnet/java-interop@568d24ac added support to allow
generator --apiversions to be specified multiple times. Take
advantage of this new support to pass in the api-versions.xml files
from both platforms/android-R and platform-tools/api when
binding API levels > API-29. (Attempting to do this for all
versions which have both resulted in bizarre API compat errors, as
the RegisterAttribute.ApiSince value was cleared. ?!)
This works around the deficiency in API-R's api-versions.xml and
allows us to retain correct RegisterAttribute.ApiSince values.

Aside: to manually build the API-R binding, use:

    msbuild /p:AndroidPlatformId=R /p:AndroidApiLevel=30 /p:AndroidFrameworkVersion=v10.0.99 src/Mono.Android/Mono.Android.csproj /v:diag > b.txt

[jonpryor]

Perhaps unsurprisingly, it doesn't build: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=3524234&view=results

Because of the API Compat checks:

error : CompatApi command: /Volumes/Xamarin-Work/xamarin-android/packages/microsoft.dotnet.apicompat/5.0.0-beta.20078.1/tools/net472/Microsoft.DotNet.ApiCompat.exe "/Volumes/Xamarin-Work/xamarin-android/bin/Debug/lib/xamarin.android/xbuild-frameworks/MonoAndroid/v10.0/ApiCompatTemp" -i "/Volumes/Xamarin-Work/xamarin-android/bin/Debug/lib/xamarin.android/xbuild-frameworks/MonoAndroid/v10.0.99/ApiCompatTemp" --exclude-attributes ../../tests/api-compatibility/api-compat-exclude-attributes.txt 
error : CheckApiCompatibility found nonacceptable Api breakages for ApiLevel: v10.0.99.
error : There is an invalid issue listed on the acceptable breakages file: CannotAddAbstractMembers : Member 'Android.Telephony.CellInfo.CellIdentity' is abstract in the implementation but is missing in the contract.
error : There is an invalid issue listed on the acceptable breakages file: CannotAddAbstractMembers : Member 'Android.Telephony.CellInfo.CellSignalStrength' is abstract in the implementation but is missing in the contract.
error : There is an invalid issue listed on the acceptable breakages file: CannotAddAbstractMembers : Member 'Android.Telephony.CellInfo.CellIdentity.get()' is abstract in the implementation but is missing in the contract.
error : There is an invalid issue listed on the acceptable breakages file: CannotAddAbstractMembers : Member 'Android.Telephony.CellInfo.CellSignalStrength.get()' is abstract in the implementation but is missing in the contract.
error : Compat issues with assembly Mono.Android:
error : MembersMustExist : Member 'Android.Telephony.CellInfoCdma.CellIdentity.get()' does not exist in the implementation but it does exist in the contract.
error : MembersMustExist : Member 'Android.Telephony.CellInfoCdma.CellSignalStrength.get()' does not exist in the implementation but it does exist in the contract.
error : MembersMustExist : Member 'Android.Telephony.CellInfoGsm.CellIdentity.get()' does not exist in the implementation but it does exist in the contract.
error : MembersMustExist : Member 'Android.Telephony.CellInfoGsm.CellSignalStrength.get()' does not exist in the implementation but it does exist in the contract.
error : MembersMustExist : Member 'Android.Telephony.CellInfoLte.CellIdentity.get()' does not exist in the implementation but it does exist in the contract.
error : MembersMustExist : Member 'Android.Telephony.CellInfoLte.CellSignalStrength.get()' does not exist in the implementation but it does exist in the contract.
error : MembersMustExist : Member 'Android.Telephony.CellInfoTdscdma.CellIdentity.get()' does not exist in the implementation but it does exist in the contract.
error : MembersMustExist : Member 'Android.Telephony.CellInfoTdscdma.CellSignalStrength.get()' does not exist in the implementation but it does exist in the contract.
error : MembersMustExist : Member 'Android.Telephony.CellInfoWcdma.CellIdentity.get()' does not exist in the implementation but it does exist in the contract.
error : MembersMustExist : Member 'Android.Telephony.CellInfoWcdma.CellSignalStrength.get()' does not exist in the implementation but it does exist in the contract.
error : Total Issues: 15

Looks like the managedReturns aren't being used?

https://github.com/xamarin/xamarin-android/blob/1a6fcff8a5a68bc2b53afa948176dea9681ff0d0/src/Mono.Android/metadata#L1538-L1548

[jonpryor]

So, "funny thing", the api/api-versions.xml file included with API-R is missing certain members!

$ diff -u ~/android-toolchain/sdk/platforms/android-29/data/api-versions.xml ~/android-toolchain/sdk/platforms/android-R/data/api-versions.xml
...
-               <method name="trimToSize()V"/>
...

$ diff -u ~/android-toolchain/sdk/platforms/android-29/data/api-versions.xml ~/android-toolchain/sdk/platforms/android-R/data/api-versions.xml | grep '^-' | wc -l
     312

Related: #4012 (comment)

😭

SO!

We could update generator to look at merge.SourceFile, as suggested in the comment on PR #4012, but in the interest of...assuming future Google sanity? (roflmao?)... we should update generator --apiversions=FILE to be usable multiple times, so that we can give it both android-R/data/api-versions.xml and platform-tools/api/api-versions.xml.

(I still fear we'll need to do the "use merge.SourceFile approach, but that's not usable by anything outside of xamarin-android...)

[jonpryor]

Looks like the managedReturns aren't being used?

I'm wrong/wasn't interpreting things correctly. They are being used.

I am thus further confused by Microsoft.DotNet.ApiCompat.exe.

Background: API-R adds CellInfo.getCellIdentity(), and CellInfoCdma.getCellIdentity() overrides it.

These do not have the same return type:

public class CellInfo {
    public CellIdentity getCellIdentity();
}
public class CellInfoCdma extends CellInfo {
    public CellIdentityCdma getCellIdentity();
}

This is fine in Java because of covariant return types. This isn't fine in C#, which is why we have those metadata rules so that CellInfoCdma.CellIdentity has the same property type as CellInfo.CellIdentity.

This in turn means that the property types changed, which is a compatibility break:

API-29:

namespace Android.Telephony {
	public sealed partial class CellInfoCdma : Android.Telephony.CellInfo, Android.OS.IParcelable {
		public unsafe Android.Telephony.CellIdentityCdma CellIdentity {
...
	}
}

API-R:

namespace Android.Telephony {
	public sealed partial class CellInfoCdma : Android.Telephony.CellInfo, Android.OS.IParcelable {
		public unsafe Android.Telephony.CellIdentity CellIdentity {
...
	}
}

In a One Mono.Android.dll World Order...I'm not sure how we actually support this. We'll need to figure that out later.

In the meantime, we have a property type change between API-29/v10.0 and API-R/v10.0.99! This is clearly a compatibility break!

...and it's exactly this type change which the metadata rules were performing. Consequently the managedReturns are being used.

Yet Microsoft.DotNet.ApiCompat.exe doesn't even mention this breakage:

https://github.com/xamarin/xamarin-android/blob/1a6fcff8a5a68bc2b53afa948176dea9681ff0d0/tests/api-compatibility/acceptable-breakages-v10.0.99.txt

Why aren't the altered property types of CellInfoCdma.CellIdentity mentioned at all?

[jonpryor]

Additionally, the Microsoft.DotNet.ApiCompat.exe errors from #4356 (comment) don't make any sense to me. Consider:

error : MembersMustExist : Member 'Android.Telephony.CellInfoGsm.CellIdentity.get()' does not exist in the implementation but it does exist in the contract.

This says to me that Android.Telephony.CellInfoCdma.get_CellIdentity() does not exist. I get the same error message locally.

Yet it does exist:

$ monodis --method bin/Debug/lib/xamarin.android/xbuild-frameworks/MonoAndroid/v10.0.99/Mono.Android.dll
...
########## Android.Telephony.CellInfoGsm
44223: default class Android.OS.IParcelableCreator get_Creator ()  (param: 51479 impl_flags: cil managed )
44224: default native int get_class_ref ()  (param: 51479 impl_flags: cil managed )
44225: failed to parse due to Could not load file or assembly 'Java.Interop, Version=0.1.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065' or one of its dependencies.
44226: instance default native int get_ThresholdClass ()  (param: 51479 impl_flags: cil managed )
44227: instance default class [mscorlib]System.Type get_ThresholdType ()  (param: 51479 impl_flags: cil managed )
44228: instance default void '.ctor' (native int javaReference, valuetype Android.Runtime.JniHandleOwnership transfer)  (param: 51479 impl_flags: cil managed )
44229: instance default class Android.Telephony.CellIdentity get_CellIdentity ()  (param: 51481 impl_flags: cil managed )
44230: instance default class Android.Telephony.CellSignalStrength get_CellSignalStrength ()  (param: 51481 impl_flags: cil managed )

monodis --method shows Android.Telephony.CellInfoGsm.get_CellIdentity(). Why then does Microsoft.DotNet.ApiCompat.exe claim that it doesn't exist in the implementation?

[jonpryor]

A fix to allow multiple generator --apiversions options: dotnet/java-interop#593

Relevant patch to apply here once that exists:

diff --git a/src/Mono.Android/Mono.Android.targets b/src/Mono.Android/Mono.Android.targets
index 8735c2ae..baae759b 100644
--- a/src/Mono.Android/Mono.Android.targets
+++ b/src/Mono.Android/Mono.Android.targets
@@ -78,10 +78,13 @@
       Inputs="metadata;enumflags;map.csv;methodmap.csv;$(IntermediateOutputPath)mcw\api.xml"
       Outputs="$(IntermediateOutputPath)mcw\Mono.Android.projitems">
     <MakeDir Directories="$(IntermediateOutputPath)mcw" />
-    <PropertyGroup>
-      <_ApiVersions Condition="Exists('$(AndroidSdkDirectory)\platforms\android-$(AndroidPlatformId)\data\api-versions.xml')">"$(AndroidSdkDirectory)\platforms\android-$(AndroidPlatformId)\data\api-versions.xml"</_ApiVersions>
-      <_ApiVersions Condition="'$(_ApiVersions)'==''">"$(AndroidSdkDirectory)\platform-tools\api\api-versions.xml"</_ApiVersions>
-    </PropertyGroup>
+    <ItemGroup>
+      <_ApiVersion Include="$(AndroidSdkDirectory)\platform-tools\api\api-versions.xml" />
+      <_ApiVersion
+          Condition="Exists('$(AndroidSdkDirectory)\platforms\android-$(AndroidPlatformId)\data\api-versions.xml')"
+          Include="$(AndroidSdkDirectory)\platforms\android-$(AndroidPlatformId)\data\api-versions.xml"
+      />
+    </ItemGroup>
     <PropertyGroup>
       <Generator>"$(XAInstallPrefix)xbuild\Xamarin\Android\generator.exe"</Generator>
       <_GenFlags>--public --product-version=7</_GenFlags>
@@ -91,7 +94,7 @@
       <_Fixup>--fixup=metadata</_Fixup>
       <_Enums1>--preserve-enums --enumflags=enumflags --enumfields=map.csv --enummethods=methodmap.csv</_Enums1>
       <_Enums2>--enummetadata=$(IntermediateOutputPath)mcw\enummetadata</_Enums2>
-      <_Versions>--apiversions=$(_ApiVersions)</_Versions>
+      <_Versions>@(_ApiVersion->'--apiversions="%(Identity)"', ' ')</_Versions>
       <_Annotations>--annotations="$(AndroidSdkDirectory)\platform-tools\api\annotations.zip"</_Annotations>
       <_Assembly>--assembly="Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null"</_Assembly>
       <_TypeMap>--type-map-report=$(IntermediateOutputPath)mcw\type-mapping.txt</_TypeMap>

[jonpryor]

Earlier I asked:

Additionally, the Microsoft.DotNet.ApiCompat.exe errors from #4356 (comment) don't make any sense to me. Consider:

error : MembersMustExist : Member 'Android.Telephony.CellInfoGsm.CellIdentity.get()' does not exist in the implementation but it does exist in the contract.

Turns Out™, that's the message which is reported when property types change, as also noted earlier.

These error messages are so meaningful and useful!

[gugavaro]

Additionally, the Microsoft.DotNet.ApiCompat.exe errors from #4356 (comment) don't make any sense to me. Consider:

error : MembersMustExist : Member 'Android.Telephony.CellInfoGsm.CellIdentity.get()' does not exist in the implementation but it does exist in the contract.

This says to me that Android.Telephony.CellInfoCdma.get_CellIdentity() does not exist. I get the same error message locally.

Yet it does exist:

$ monodis --method bin/Debug/lib/xamarin.android/xbuild-frameworks/MonoAndroid/v10.0.99/Mono.Android.dll
...
########## Android.Telephony.CellInfoGsm
44223: default class Android.OS.IParcelableCreator get_Creator ()  (param: 51479 impl_flags: cil managed )
44224: default native int get_class_ref ()  (param: 51479 impl_flags: cil managed )
44225: failed to parse due to Could not load file or assembly 'Java.Interop, Version=0.1.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065' or one of its dependencies.
44226: instance default native int get_ThresholdClass ()  (param: 51479 impl_flags: cil managed )
44227: instance default class [mscorlib]System.Type get_ThresholdType ()  (param: 51479 impl_flags: cil managed )
44228: instance default void '.ctor' (native int javaReference, valuetype Android.Runtime.JniHandleOwnership transfer)  (param: 51479 impl_flags: cil managed )
44229: instance default class Android.Telephony.CellIdentity get_CellIdentity ()  (param: 51481 impl_flags: cil managed )
44230: instance default class Android.Telephony.CellSignalStrength get_CellSignalStrength ()  (param: 51481 impl_flags: cil managed )

monodis --method shows Android.Telephony.CellInfoGsm.get_CellIdentity(). Why then does Microsoft.DotNet.ApiCompat.exe claim that it doesn't exist in the implementation?

The issue here is that it does not exist with the same return type, would bbe nice ifthe tool, would report the message showing the name but also the return type.