Skip to content

Conversation

@jonpryor
Copy link
Contributor

@jonpryor jonpryor commented Sep 15, 2021

Context: https://developer.android.com/studio/releases/platform-tools

Changes: http://github.com/xamarin/xamarin-android-tools/compare/9b658b29bd41157151f5515619d0d90dc062563d...a5194e93498e7f12225d87e2811415a45f742116

One of the (undocumented) changes between Android SDK Platform-tools
package r30.0.4 and r31.0.3 is that
platform-tools/api/annotations.zip was removed. This removal
would cause the _GenerateBinding target in Mono.Android.targets
to fail, as it's used by generator.exe --annotations:

mono --debug=casts "/Users/builder/azdo/_work/2/s/xamarin-android/bin/Release/lib/xamarin.android/xbuild/Xamarin/Android/generator.exe" \
	--annotations="/Users/builder/Library/Android/sdk/platform-tools/api/annotations.zip" \
	…
…
error BG0000: System.IO.DirectoryNotFoundException: Could not find a part of the path "/Users/builder/Library/Android/sdk/platform-tools/api/annotations.zip".
  at System.IO.FileStream..ctor (System.String path, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share, System.Int32 bufferSize, System.Boolean anonymous, System.IO.FileOptions options)
  at System.IO.FileStream..ctor (System.String path, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share)
  at (wrapper remoting-invoke-with-check) System.IO.FileStream..ctor(string,System.IO.FileMode,System.IO.FileAccess,System.IO.FileShare)
  at System.IO.File.OpenRead (System.String path)
  at Xamarin.AndroidTools.AnnotationSupport.AndroidAnnotationsSupport.ParseArchive (System.String file)
  at Xamarin.AndroidTools.AnnotationSupport.AndroidAnnotationsSupport.Load (System.String annotationsZipFile)
  at Xamarin.Android.Binder.CodeGenerator.GenerateAnnotationAttributes (System.Collections.Generic.List`1[T] gens, System.Collections.Generic.IEnumerable`1[T] zips)
  at Xamarin.Android.Binder.CodeGenerator.Run (Xamarin.Android.Binder.CodeGeneratorOptions options, Java.Interop.Tools.Cecil.DirectoryAssemblyResolver resolver)
  at Xamarin.Android.Binder.CodeGenerator.Run (Xamarin.Android.Binder.CodeGeneratorOptions options)
  at Xamarin.Android.Binder.CodeGenerator.Main (System.String[] args)

annotations.zip has apparently moved into the platform package, and
has been there since API-26:

% ls -1tr ~/android-toolchain/sdk/platforms/*/data/annotations.zip
$HOME/android-toolchain/sdk/platforms/android-26/data/annotations.zip
$HOME/android-toolchain/sdk/platforms/android-27/data/annotations.zip
$HOME/android-toolchain/sdk/platforms/android-28/data/annotations.zip
$HOME/android-toolchain/sdk/platforms/android-29/data/annotations.zip
$HOME/android-toolchain/sdk/platforms/android-30/data/annotations.zip
$HOME/android-toolchain/sdk/platforms/android-31/data/annotations.zip

Update the _GenerateBinding target so that the "new" platform copy
of annotations.zip is used, if present.

@jonpryor jonpryor force-pushed the jonp-bump-xat-440e6bee branch from fd53efa to e658f03 Compare September 15, 2021 23:41
@jonpryor
Copy link
Contributor Author

Guess what else was changed in the bump? The Android SDK platform-tools package r31.0.3 no longer contains api/annotations.zip, which means src/Mono.Android fails to build:

  error BG0000: System.IO.DirectoryNotFoundException: Could not find a part of the path "/Users/builder/Library/Android/sdk/platform-tools/api/annotations.zip". (TaskId:3661)
    at System.IO.FileStream..ctor (System.String path, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share, System.Int32 bufferSize, System.Boolean anonymous, System.IO.FileOptions options) [0x0015e] in /Users/builder/jenkins/workspace/build-package-osx-mono/2020-02/external/bockbuild/builds/mono-x64/mcs/class/corlib/System.IO/FileStream.cs:223  (TaskId:3661)
    at System.IO.FileStream..ctor (System.String path, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share) [0x00000] in /Users/builder/jenkins/workspace/build-package-osx-mono/2020-02/external/bockbuild/builds/mono-x64/mcs/class/corlib/System.IO/FileStream.cs:91  (TaskId:3661)
    at (wrapper remoting-invoke-with-check) System.IO.FileStream..ctor(string,System.IO.FileMode,System.IO.FileAccess,System.IO.FileShare) (TaskId:3661)
    at System.IO.File.OpenRead (System.String path) [0x00000] in /Users/builder/jenkins/workspace/build-package-osx-mono/2020-02/external/bockbuild/builds/mono-x64/external/corefx/src/System.IO.FileSystem/src/System/IO/File.cs:266  (TaskId:3661)
    at Xamarin.AndroidTools.AnnotationSupport.AndroidAnnotationsSupport.ParseArchive (System.String file) [0x00000] in /Users/builder/azdo/_work/2/s/xamarin-android/external/Java.Interop/src/Xamarin.Android.Tools.AnnotationSupport/AndroidAnnotationsSupport.cs:19  (TaskId:3661)
    at Xamarin.AndroidTools.AnnotationSupport.AndroidAnnotationsSupport.Load (System.String annotationsZipFile) [0x00000] in /Users/builder/azdo/_work/2/s/xamarin-android/external/Java.Interop/src/Xamarin.Android.Tools.AnnotationSupport/AndroidAnnotationsSupport.cs:95  (TaskId:3661)
    at Xamarin.Android.Binder.CodeGenerator.GenerateAnnotationAttributes (System.Collections.Generic.List`1[T] gens, System.Collections.Generic.IEnumerable`1[T] zips) [0x00038] in /Users/builder/azdo/_work/2/s/xamarin-android/external/Java.Interop/tools/generator/CodeGenerator.cs:346  (TaskId:3661)
    at Xamarin.Android.Binder.CodeGenerator.Run (Xamarin.Android.Binder.CodeGeneratorOptions options, Java.Interop.Tools.Cecil.DirectoryAssemblyResolver resolver) [0x005d0] in /Users/builder/azdo/_work/2/s/xamarin-android/external/Java.Interop/tools/generator/CodeGenerator.cs:204  (TaskId:3661)
    at Xamarin.Android.Binder.CodeGenerator.Run (Xamarin.Android.Binder.CodeGeneratorOptions options) [0x0001b] in /Users/builder/azdo/_work/2/s/xamarin-android/external/Java.Interop/tools/generator/CodeGenerator.cs:47  (TaskId:3661)
    at Xamarin.Android.Binder.CodeGenerator.Main (System.String[] args) [0x0000e] in /Users/builder/azdo/_work/2/s/xamarin-android/external/Java.Interop/tools/generator/CodeGenerator.cs:30  (TaskId:3661)

Context: https://developer.android.com/studio/releases/platform-tools

Changes: http://github.com/xamarin/xamarin-android-tools/compare/9b658b29bd41157151f5515619d0d90dc062563d...a5194e93498e7f12225d87e2811415a45f742116

  * dotnet/android-tools@a5194e9: [Xamarin.Android.Tools.AndroidSdk] Downgrade build-tools to API-30
  * dotnet/android-tools@440e6be: [Xamarin.Android.Tools.AndroidSdk] Update SDK component for API-31 (dotnet#134)

One of the (undocumented) changes between Android SDK Platform-tools
package r30.0.4 and r31.0.3 is that
`platform-tools/api/annotations.zip` was *removed*.  This removal
would cause the `_GenerateBinding` target in `Mono.Android.targets`
to fail, as it's used by `generator.exe --annotations`:

	mono --debug=casts "/Users/builder/azdo/_work/2/s/xamarin-android/bin/Release/lib/xamarin.android/xbuild/Xamarin/Android/generator.exe" \
		--annotations="/Users/builder/Library/Android/sdk/platform-tools/api/annotations.zip" \
		…
	…
	error BG0000: System.IO.DirectoryNotFoundException: Could not find a part of the path "/Users/builder/Library/Android/sdk/platform-tools/api/annotations.zip".
	  at System.IO.FileStream..ctor (System.String path, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share, System.Int32 bufferSize, System.Boolean anonymous, System.IO.FileOptions options)
	  at System.IO.FileStream..ctor (System.String path, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share)
	  at (wrapper remoting-invoke-with-check) System.IO.FileStream..ctor(string,System.IO.FileMode,System.IO.FileAccess,System.IO.FileShare)
	  at System.IO.File.OpenRead (System.String path)
	  at Xamarin.AndroidTools.AnnotationSupport.AndroidAnnotationsSupport.ParseArchive (System.String file)
	  at Xamarin.AndroidTools.AnnotationSupport.AndroidAnnotationsSupport.Load (System.String annotationsZipFile)
	  at Xamarin.Android.Binder.CodeGenerator.GenerateAnnotationAttributes (System.Collections.Generic.List`1[T] gens, System.Collections.Generic.IEnumerable`1[T] zips)
	  at Xamarin.Android.Binder.CodeGenerator.Run (Xamarin.Android.Binder.CodeGeneratorOptions options, Java.Interop.Tools.Cecil.DirectoryAssemblyResolver resolver)
	  at Xamarin.Android.Binder.CodeGenerator.Run (Xamarin.Android.Binder.CodeGeneratorOptions options)
	  at Xamarin.Android.Binder.CodeGenerator.Main (System.String[] args)

`annotations.zip` has apparently moved into the platform package, and
has been there since API-26:

	% ls -1tr ~/android-toolchain/sdk/platforms/*/data/annotations.zip
	$HOME/android-toolchain/sdk/platforms/android-26/data/annotations.zip
	$HOME/android-toolchain/sdk/platforms/android-27/data/annotations.zip
	$HOME/android-toolchain/sdk/platforms/android-28/data/annotations.zip
	$HOME/android-toolchain/sdk/platforms/android-29/data/annotations.zip
	$HOME/android-toolchain/sdk/platforms/android-30/data/annotations.zip
	$HOME/android-toolchain/sdk/platforms/android-31/data/annotations.zip

Update the `_GenerateBinding` target so that the "new" platform copy
of `annotations.zip` is used, if present.
@jonpryor jonpryor force-pushed the jonp-bump-xat-440e6bee branch from e658f03 to 9048eef Compare September 16, 2021 01:27
@jonpryor jonpryor requested a review from jpobst September 16, 2021 01:28
@jonpryor
Copy link
Contributor Author

jonpryor added a commit to jonpryor/java.interop that referenced this pull request Sep 16, 2021
Context: dotnet/android#6300

A confluence of several "funny" things happened with
dotnet/android#6300, which attempts to use the Android SDK
platform-tools 31.0.3 package, up from 30.0.2:

 1. The new platform-tools package *removes* the file
    `platform-tools/api/annotations.zip`.  To work around that, we
    need to instead use the `data/annotations.zip` file from the
    Android platform directory, e.g.
    `$ANDROID_SDK_ROOT/platforms/android-30/data/annotations.zip`.

 2. Between API-28 and API-29, `annotations.zip` changes the package
    name used for annotations, from e.g.
    `android.support.annotation.RequiresPermission` to
    `androidx.annotation.RequiresPermission`.

In isolation, okay, but then we hit:

 3. `AnnotationData` *removes* the "known" package-prefix of
    `android.support.annotation.`, and all use of
    `AnnotationData.Name` is via `string.operator==`, *not*
    `string.Contains()` or `string.IndexOf()`.

The result of all these changes together is API breakage in
`Mono.Android.dll` between API-28 and API-29:

	…/Mono.Android.targets(257,5): error :
	CannotRemoveAttribute : Attribute 'Android.Runtime.RequiresPermissionAttribute' exists on
	'Android.Accounts.AccountManager.AddAccount(System.String, System.String, System.String[], Android.OS.Bundle, Android.App.Activity, Android.Accounts.IAccountManagerCallback, Android.OS.Handler)'
	in the contract but not the implementation.

Or, in C# `diff -u` terms:

	--- obj/Debug/monoandroid10/android-28/mcw/Android.Accounts.AccountManager.cs	2021-09-16 09:00:53.000000000 -0400
	+++ obj/Debug/monoandroid10/android-29/mcw/Android.Accounts.AccountManager.cs	2021-09-16 10:41:15.000000000 -0400
	@@ -217,7 +217,6 @@

	 		// Metadata.xml XPath method reference: path="/api/package[@name='android.accounts']/class[@name='AccountManager']/method[@name='addAccount' and count(parameter)=7 and parameter[1][@type='java.lang.String'] and parameter[2][@type='java.lang.String'] and parameter[3][@type='java.lang.String[]'] and parameter[4][@type='android.os.Bundle'] and parameter[5][@type='android.app.Activity'] and parameter[6][@type='android.accounts.AccountManagerCallback<android.os.Bundle>'] and parameter[7][@type='android.os.Handler']]"
	 		[Register ("addAccount", "(Ljava/lang/String;Ljava/lang/String;[Ljava/lang/String;Landroid/os/Bundle;Landroid/app/Activity;Landroid/accounts/AccountManagerCallback;Landroid/os/Handler;)Landroid/accounts/AccountManagerFuture;", "GetAddAccount_Ljava_lang_String_Ljava_lang_String_arrayLjava_lang_String_Landroid_os_Bundle_Landroid_app_Activity_Landroid_accounts_AccountManagerCallback_Landroid_os_Handler_Handler")]
	-		[global::Android.Runtime.RequiresPermission ("android.permission.MANAGE_ACCOUNTS")]
	 		public virtual unsafe Android.Accounts.IAccountManagerFuture? AddAccount (string? accountType, string? authTokenType, string[]? requiredFeatures, Android.OS.Bundle? addAccountOptions, Android.App.Activity? activity, Android.Accounts.IAccountManagerCallback? @callback, Android.OS.Handler? handler)
	 		{
	 			const string __id = "addAccount.(Ljava/lang/String;Ljava/lang/String;[Ljava/lang/String;Landroid/os/Bundle;Landroid/app/Activity;Landroid/accounts/AccountManagerCallback;Landroid/os/Handler;)Landroid/accounts/AccountManagerFuture;";

Fix this breakage by updating `AnnotationData` to remove the package
name for `androidx.annotation` as well.

Additionally, add an `AnnotatedItem.ToString()` override to simplify
future printf-style debugging.
jonpryor added a commit to dotnet/java-interop that referenced this pull request Sep 16, 2021
Context: dotnet/android#6300

A confluence of several "funny" things happened with
dotnet/android#6300, which attempts to use the Android SDK
platform-tools 31.0.3 package, up from 30.0.2:

 1. The new platform-tools package *removes* the file
    `platform-tools/api/annotations.zip`.  To work around that, we
    need to instead use the `data/annotations.zip` file from the
    Android platform directory, e.g.
    `$ANDROID_SDK_ROOT/platforms/android-30/data/annotations.zip`.

 2. Between API-28 and API-29, `annotations.zip` changes the package
    name used for annotations, from e.g.
    `android.support.annotation.RequiresPermission` to
    `androidx.annotation.RequiresPermission`.

In isolation, okay, but then we hit:

 3. `AnnotationData` *removes* the "known" package-prefix of
    `android.support.annotation.`, and all use of
    `AnnotationData.Name` is via `string.operator==`, *not*
    `string.Contains()` or `string.IndexOf()`.

The result of all these changes together is API breakage in
`Mono.Android.dll` between API-28 and API-29:

	…/Mono.Android.targets(257,5): error :
	CannotRemoveAttribute : Attribute 'Android.Runtime.RequiresPermissionAttribute' exists on
	'Android.Accounts.AccountManager.AddAccount(System.String, System.String, System.String[], Android.OS.Bundle, Android.App.Activity, Android.Accounts.IAccountManagerCallback, Android.OS.Handler)'
	in the contract but not the implementation.

Or, in C# `diff -u` terms:

	--- obj/Debug/monoandroid10/android-28/mcw/Android.Accounts.AccountManager.cs	2021-09-16 09:00:53.000000000 -0400
	+++ obj/Debug/monoandroid10/android-29/mcw/Android.Accounts.AccountManager.cs	2021-09-16 10:41:15.000000000 -0400
	@@ -217,7 +217,6 @@

	 		// Metadata.xml XPath method reference: path="/api/package[@name='android.accounts']/class[@name='AccountManager']/method[@name='addAccount' and count(parameter)=7 and parameter[1][@type='java.lang.String'] and parameter[2][@type='java.lang.String'] and parameter[3][@type='java.lang.String[]'] and parameter[4][@type='android.os.Bundle'] and parameter[5][@type='android.app.Activity'] and parameter[6][@type='android.accounts.AccountManagerCallback<android.os.Bundle>'] and parameter[7][@type='android.os.Handler']]"
	 		[Register ("addAccount", "(Ljava/lang/String;Ljava/lang/String;[Ljava/lang/String;Landroid/os/Bundle;Landroid/app/Activity;Landroid/accounts/AccountManagerCallback;Landroid/os/Handler;)Landroid/accounts/AccountManagerFuture;", "GetAddAccount_Ljava_lang_String_Ljava_lang_String_arrayLjava_lang_String_Landroid_os_Bundle_Landroid_app_Activity_Landroid_accounts_AccountManagerCallback_Landroid_os_Handler_Handler")]
	-		[global::Android.Runtime.RequiresPermission ("android.permission.MANAGE_ACCOUNTS")]
	 		public virtual unsafe Android.Accounts.IAccountManagerFuture? AddAccount (string? accountType, string? authTokenType, string[]? requiredFeatures, Android.OS.Bundle? addAccountOptions, Android.App.Activity? activity, Android.Accounts.IAccountManagerCallback? @callback, Android.OS.Handler? handler)
	 		{
	 			const string __id = "addAccount.(Ljava/lang/String;Ljava/lang/String;[Ljava/lang/String;Landroid/os/Bundle;Landroid/app/Activity;Landroid/accounts/AccountManagerCallback;Landroid/os/Handler;)Landroid/accounts/AccountManagerFuture;";

Fix this breakage by updating `AnnotationData` to remove the package
name for `androidx.annotation` as well.

Additionally, add an `AnnotatedItem.ToString()` override to simplify
future printf-style debugging.
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request Sep 20, 2021
Context: dotnet#6300
Context: dotnet/java-interop#883

Changes: http://github.com/xamarin/xamarin-android-tools/compare/9b658b29bd41157151f5515619d0d90dc062563d...a5194e93498e7f12225d87e2811415a45f742116

  * dotnet/android-tools@a5194e9: [Xamarin.Android.Tools.AndroidSdk] Downgrade build-tools to API-30
  * dotnet/android-tools@440e6be: [Xamarin.Android.Tools.AndroidSdk] Update SDK component for API-31 (dotnet#134)

Note: while this xamarin-android-tools bump updates the Android SDK
[Platform Tools][0] version to 31.0.3, we are *not* updating
`$(XAPlatformToolsVersion)` to 31.0.3.  As such, there will be a
mismatch between what we build xamarin-android against, vs. the
default suggested platform-tools package potentially installed
via the `/t:InstallAndroidDependencies` target.

The reason to *not* bump `$(XAPlatformToolsVersion)` is that
attempting to do so breaks the `src/Mono.Android` build:

 1. platform-tools r31.0.3 doesn't contain
    `platform-tools/api/annotations.zip`, which is used by
    `generator` to emit certain custom attributes such as
    `RequiresPermission`.

 2. While (1) can be worked around by instead using
    `$(AndroidSdkDirectory)/platforms/android-*/data/annotations.zip`,
    this introduces API changes reported by the
    `_CheckApiCompatibility` target, in particular changes due to
    custom attribute string changes.  These string changes happen
    because Google ships *invalid XML* in `data/annotations.zip` for
    API-29+ (?!):

        <!-- annotations.zip!android/accounts/annotations.xml -->
        -  <item name="android.accounts.AccountManager android.accounts.AccountManagerFuture&lt;android.os.Bundle&gt; addAccount(java.lang.String, java.lang.String, java.lang.String[], android.os.Bundle, android.app.Activity, android.accounts.AccountManagerCallback&lt;android.os.Bundle&gt;, android.os.Handler)">
        +  <item name="android.accounts.AccountManager android.accounts.AccountManagerFuture&lt;android.os.Bundle&gt; addAccount(java.lang.String, java.lang.String, java.lang.String[], android.os.Bundle, android.app.Activity, android.accounts.AccountManagerCallback<android.os.Bundle>, android.os.Handler)">

    `AccountManagerCallback<android.os.Bundle>` is not a valid value
    within an XML attribute.

    This change doesn't break the build, but instead causes
    [`AndroidAnnotationSupport`][1] to hit a "fallback" codepath
    which uses HtmlAgilityPack, and this causes attribute values
    to "gain" `quot;`:

        -[global::Android.Runtime.RequiresPermission ("android.permission.MANAGE_ACCOUNTS")]
        +[global::Android.Runtime.RequiresPermission ("quot;android.permission.MANAGE_ACCOUNTS&quot")]

    See also: dotnet/java-interop#883

After manual review, no other file removals appear to be problematic,
so @jonpryor asserts that it should be acceptable for
`$(XAPlatformToolsVersion)` and `$(AndroidSdkPlatformToolsVersion)`
to be inconsistent with each other.

[0]: https://developer.android.com/studio/releases/platform-tools
[1]: https://github.com/xamarin/java.interop/blob/1e8f5137345db3160c99265ff3a56c43a132194f/src/Xamarin.Android.Tools.AnnotationSupport/AndroidAnnotationsSupport.cs#L58-L87
jonpryor added a commit that referenced this pull request Sep 20, 2021
Context: #6300
Context: dotnet/java-interop#883

Changes: http://github.com/xamarin/xamarin-android-tools/compare/9b658b29bd41157151f5515619d0d90dc062563d...a5194e93498e7f12225d87e2811415a45f742116

  * dotnet/android-tools@a5194e9: [Xamarin.Android.Tools.AndroidSdk] Downgrade build-tools to API-30
  * dotnet/android-tools@440e6be: [Xamarin.Android.Tools.AndroidSdk] Update SDK component for API-31 (#134)

Note: while this xamarin-android-tools bump updates the Android SDK
[Platform Tools][0] version to 31.0.3, we are *not* updating
`$(XAPlatformToolsVersion)` to 31.0.3.  As such, there will be a
mismatch between what we build xamarin-android against, vs. the
default suggested platform-tools package potentially installed
via the `/t:InstallAndroidDependencies` target.

The reason to *not* bump `$(XAPlatformToolsVersion)` is that
attempting to do so breaks the `src/Mono.Android` build:

 1. platform-tools r31.0.3 doesn't contain
    `platform-tools/api/annotations.zip`, which is used by
    `generator` to emit certain custom attributes such as
    `RequiresPermission`.

 2. While (1) can be worked around by instead using
    `$(AndroidSdkDirectory)/platforms/android-*/data/annotations.zip`,
    this introduces API changes reported by the
    `_CheckApiCompatibility` target, in particular changes due to
    custom attribute string changes.  These string changes happen
    because Google ships *invalid XML* in `data/annotations.zip` for
    API-29+ (?!):

        <!-- annotations.zip!android/accounts/annotations.xml -->
        -  <item name="android.accounts.AccountManager android.accounts.AccountManagerFuture&lt;android.os.Bundle&gt; addAccount(java.lang.String, java.lang.String, java.lang.String[], android.os.Bundle, android.app.Activity, android.accounts.AccountManagerCallback&lt;android.os.Bundle&gt;, android.os.Handler)">
        +  <item name="android.accounts.AccountManager android.accounts.AccountManagerFuture&lt;android.os.Bundle&gt; addAccount(java.lang.String, java.lang.String, java.lang.String[], android.os.Bundle, android.app.Activity, android.accounts.AccountManagerCallback<android.os.Bundle>, android.os.Handler)">

    `AccountManagerCallback<android.os.Bundle>` is not a valid value
    within an XML attribute.

    This change doesn't break the build, but instead causes
    [`AndroidAnnotationSupport`][1] to hit a "fallback" codepath
    which uses HtmlAgilityPack, and this causes attribute values
    to "gain" `quot;`:

        -[global::Android.Runtime.RequiresPermission ("android.permission.MANAGE_ACCOUNTS")]
        +[global::Android.Runtime.RequiresPermission ("quot;android.permission.MANAGE_ACCOUNTS&quot")]

    See also: dotnet/java-interop#883

After manual review, no other file removals appear to be problematic,
so @jonpryor asserts that it should be acceptable for
`$(XAPlatformToolsVersion)` and `$(AndroidSdkPlatformToolsVersion)`
to be inconsistent with each other.

[0]: https://developer.android.com/studio/releases/platform-tools
[1]: https://github.com/xamarin/java.interop/blob/1e8f5137345db3160c99265ff3a56c43a132194f/src/Xamarin.Android.Tools.AnnotationSupport/AndroidAnnotationsSupport.cs#L58-L87
jonathanpeppers pushed a commit that referenced this pull request Sep 21, 2021
Context: #6300
Context: dotnet/java-interop#883

Changes: http://github.com/xamarin/xamarin-android-tools/compare/9b658b29bd41157151f5515619d0d90dc062563d...a5194e93498e7f12225d87e2811415a45f742116

  * dotnet/android-tools@a5194e9: [Xamarin.Android.Tools.AndroidSdk] Downgrade build-tools to API-30
  * dotnet/android-tools@440e6be: [Xamarin.Android.Tools.AndroidSdk] Update SDK component for API-31 (#134)

Note: while this xamarin-android-tools bump updates the Android SDK
[Platform Tools][0] version to 31.0.3, we are *not* updating
`$(XAPlatformToolsVersion)` to 31.0.3.  As such, there will be a
mismatch between what we build xamarin-android against, vs. the
default suggested platform-tools package potentially installed
via the `/t:InstallAndroidDependencies` target.

The reason to *not* bump `$(XAPlatformToolsVersion)` is that
attempting to do so breaks the `src/Mono.Android` build:

 1. platform-tools r31.0.3 doesn't contain
    `platform-tools/api/annotations.zip`, which is used by
    `generator` to emit certain custom attributes such as
    `RequiresPermission`.

 2. While (1) can be worked around by instead using
    `$(AndroidSdkDirectory)/platforms/android-*/data/annotations.zip`,
    this introduces API changes reported by the
    `_CheckApiCompatibility` target, in particular changes due to
    custom attribute string changes.  These string changes happen
    because Google ships *invalid XML* in `data/annotations.zip` for
    API-29+ (?!):

        <!-- annotations.zip!android/accounts/annotations.xml -->
        -  <item name="android.accounts.AccountManager android.accounts.AccountManagerFuture&lt;android.os.Bundle&gt; addAccount(java.lang.String, java.lang.String, java.lang.String[], android.os.Bundle, android.app.Activity, android.accounts.AccountManagerCallback&lt;android.os.Bundle&gt;, android.os.Handler)">
        +  <item name="android.accounts.AccountManager android.accounts.AccountManagerFuture&lt;android.os.Bundle&gt; addAccount(java.lang.String, java.lang.String, java.lang.String[], android.os.Bundle, android.app.Activity, android.accounts.AccountManagerCallback<android.os.Bundle>, android.os.Handler)">

    `AccountManagerCallback<android.os.Bundle>` is not a valid value
    within an XML attribute.

    This change doesn't break the build, but instead causes
    [`AndroidAnnotationSupport`][1] to hit a "fallback" codepath
    which uses HtmlAgilityPack, and this causes attribute values
    to "gain" `quot;`:

        -[global::Android.Runtime.RequiresPermission ("android.permission.MANAGE_ACCOUNTS")]
        +[global::Android.Runtime.RequiresPermission ("quot;android.permission.MANAGE_ACCOUNTS&quot")]

    See also: dotnet/java-interop#883

After manual review, no other file removals appear to be problematic,
so @jonpryor asserts that it should be acceptable for
`$(XAPlatformToolsVersion)` and `$(AndroidSdkPlatformToolsVersion)`
to be inconsistent with each other.

[0]: https://developer.android.com/studio/releases/platform-tools
[1]: https://github.com/xamarin/java.interop/blob/1e8f5137345db3160c99265ff3a56c43a132194f/src/Xamarin.Android.Tools.AnnotationSupport/AndroidAnnotationsSupport.cs#L58-L87
jpobst pushed a commit to dotnet/java-interop that referenced this pull request Sep 30, 2021
Context: dotnet/android#6300

A confluence of several "funny" things happened with
dotnet/android#6300, which attempts to use the Android SDK
platform-tools 31.0.3 package, up from 30.0.2:

 1. The new platform-tools package *removes* the file
    `platform-tools/api/annotations.zip`.  To work around that, we
    need to instead use the `data/annotations.zip` file from the
    Android platform directory, e.g.
    `$ANDROID_SDK_ROOT/platforms/android-30/data/annotations.zip`.

 2. Between API-28 and API-29, `annotations.zip` changes the package
    name used for annotations, from e.g.
    `android.support.annotation.RequiresPermission` to
    `androidx.annotation.RequiresPermission`.

In isolation, okay, but then we hit:

 3. `AnnotationData` *removes* the "known" package-prefix of
    `android.support.annotation.`, and all use of
    `AnnotationData.Name` is via `string.operator==`, *not*
    `string.Contains()` or `string.IndexOf()`.

The result of all these changes together is API breakage in
`Mono.Android.dll` between API-28 and API-29:

	…/Mono.Android.targets(257,5): error :
	CannotRemoveAttribute : Attribute 'Android.Runtime.RequiresPermissionAttribute' exists on
	'Android.Accounts.AccountManager.AddAccount(System.String, System.String, System.String[], Android.OS.Bundle, Android.App.Activity, Android.Accounts.IAccountManagerCallback, Android.OS.Handler)'
	in the contract but not the implementation.

Or, in C# `diff -u` terms:

	--- obj/Debug/monoandroid10/android-28/mcw/Android.Accounts.AccountManager.cs	2021-09-16 09:00:53.000000000 -0400
	+++ obj/Debug/monoandroid10/android-29/mcw/Android.Accounts.AccountManager.cs	2021-09-16 10:41:15.000000000 -0400
	@@ -217,7 +217,6 @@

	 		// Metadata.xml XPath method reference: path="/api/package[@name='android.accounts']/class[@name='AccountManager']/method[@name='addAccount' and count(parameter)=7 and parameter[1][@type='java.lang.String'] and parameter[2][@type='java.lang.String'] and parameter[3][@type='java.lang.String[]'] and parameter[4][@type='android.os.Bundle'] and parameter[5][@type='android.app.Activity'] and parameter[6][@type='android.accounts.AccountManagerCallback&lt;android.os.Bundle&gt;'] and parameter[7][@type='android.os.Handler']]"
	 		[Register ("addAccount", "(Ljava/lang/String;Ljava/lang/String;[Ljava/lang/String;Landroid/os/Bundle;Landroid/app/Activity;Landroid/accounts/AccountManagerCallback;Landroid/os/Handler;)Landroid/accounts/AccountManagerFuture;", "GetAddAccount_Ljava_lang_String_Ljava_lang_String_arrayLjava_lang_String_Landroid_os_Bundle_Landroid_app_Activity_Landroid_accounts_AccountManagerCallback_Landroid_os_Handler_Handler")]
	-		[global::Android.Runtime.RequiresPermission ("android.permission.MANAGE_ACCOUNTS")]
	 		public virtual unsafe Android.Accounts.IAccountManagerFuture? AddAccount (string? accountType, string? authTokenType, string[]? requiredFeatures, Android.OS.Bundle? addAccountOptions, Android.App.Activity? activity, Android.Accounts.IAccountManagerCallback? @callback, Android.OS.Handler? handler)
	 		{
	 			const string __id = "addAccount.(Ljava/lang/String;Ljava/lang/String;[Ljava/lang/String;Landroid/os/Bundle;Landroid/app/Activity;Landroid/accounts/AccountManagerCallback;Landroid/os/Handler;)Landroid/accounts/AccountManagerFuture;";

Fix this breakage by updating `AnnotationData` to remove the package
name for `androidx.annotation` as well.

Additionally, add an `AnnotatedItem.ToString()` override to simplify
future printf-style debugging.
@jonpryor jonpryor marked this pull request as draft October 1, 2021 01:42
@jonpryor jonpryor changed the title Bump to xamarin/xamarin-android-tools/main@440e6bee [WIP] platform-tools r31 support Oct 1, 2021
@jonpryor
Copy link
Contributor Author

jonpryor commented Oct 25, 2021

Superseded by PR #6424. (I couldn't easily rebase.)

@jonpryor jonpryor closed this Oct 25, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Jan 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant