Skip to content

[generator] Fix for fixing invalid annotation XML. #897

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

Merged
merged 1 commit into from
Oct 26, 2021
Merged

Conversation

jpobst
Copy link
Contributor

@jpobst jpobst commented Oct 21, 2021

Fixes #883

Due to invalid XML provided in Google's annotations.zip file, we run it through the more forgiving HtmlAgilityPack to attempt to fix it to valid XML.

However, given this snippet:

<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)">
  <annotation name="androidx.annotation.RequiresPermission">
    <val name="value" val="&quot;android.permission.MANAGE_ACCOUNTS&quot;" />
    <val name="apis" val="&quot;..22&quot;" />
  </annotation>
</item>

The invalid unescaped < and > characters in the <item name='' attribute seem to tell HtmlAgilityPack not to expect any attribute strings to be properly escaped. Thus when it gets to the <val val='' attributes, it treats &quot; as unescaped as well, thinking we want to value to be the literal string "&quot;". When it writes out the valid XML, it realizes it needs to escape the ampersand, and writes out &amp;quot;, which breaks our usage of these annotations.

The fix is to "unescape" every &quot; to a " so that it will be escaped correctly when saved as valid XML.

@jonpryor
Copy link
Contributor

This works, but I'd like "more". Testing locally, I see errors such as:

CannotChangeAttribute : Attribute 'Android.Runtime.RequiresPermissionAttribute' on 'Android.App.WallpaperManager.Clear(Android.App.WallpaperManagerFlags)' changed from '[RequiresPermissionAttribute("android.permission.SET_WALLPAPER")]' in the contract to '[RequiresPermissionAttribute("android.permission.SET_WALLPAPER")]' in the implementation.

which is kinda non-sensical: i have an attribute, and have the same attribute? (We dearly need do submit some UX improvements to that tool!)

So, what changed?

$ diff -u obj/Debug/monoandroid10/android-{27,28}/mcw/Android.App.WallpaperManager.cs
@@ -553,7 +553,7 @@
                // Metadata.xml XPath method reference: path="/api/package[@name='android.app']/class[@name='WallpaperManager']/method[@name='clear' and count(parameter)=1 and parameter[1][@type='int']]"
                [global::System.Runtime.Versioning.SupportedOSPlatformAttribute ("android24.0")]
                [Register ("clear", "(I)V", "GetClear_IHandler", ApiSince = 24)]
-               [global::Android.Runtime.RequiresPermission ("android.permission.SET_WALLPAPER")]
+               [global::Android.Runtime.RequiresPermission ("android.permission.SET_WALLPAPER")][global::Android.Runtime.RequiresPermission ("android.permission.SET_WALLPAPER")]
                public virtual unsafe void Clear ([global::Android.Runtime.GeneratedEnum] Android.App.WallpaperManagerFlags which)
                {
                        const string __id = "clear.(I)V";

We went from one attribute to two attributes, both with the same value.

Ideally, this wouldn't happen: we should only emit a given attribute once.

jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request Oct 25, 2021
Context: dotnet/java-interop#897
Context: f6011d9

Commit f6011d9 updated external/xamarin-android-tools so that the
Android SDK platform-tools r31.0.3 package would be preferred, but
*didn't* update `$(XAPlatformToolsVersion)` to cause the
xamarin-android build to use platform-tools r31.0.3, because:

> 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+ (?!):

dotnet/java-interop#897 is a proposed fix for (2), and should allow
us to use platform-tools r31.0.3 for the xamarin-android build.

WIP WIP WIP WIP

Context: dotnet/java-interop#897 (comment)

This does not fully build for me locally, because "duplicate" custom
attributes are being emitted:

	$ diff -u obj/Debug/monoandroid10/android-{27,28}/mcw/Android.App.WallpaperManager.cs
	@@ -553,7 +553,7 @@
	                // Metadata.xml XPath method reference: path="/api/package[@name='android.app']/class[@name='WallpaperManager']/method[@name='clear' and count(parameter)=1 and parameter[1][@type='int']]"
	                [global::System.Runtime.Versioning.SupportedOSPlatformAttribute ("android24.0")]
	                [Register ("clear", "(I)V", "GetClear_IHandler", ApiSince = 24)]
	-               [global::Android.Runtime.RequiresPermission ("android.permission.SET_WALLPAPER")]
	+               [global::Android.Runtime.RequiresPermission ("android.permission.SET_WALLPAPER")][global::Android.Runtime.RequiresPermission ("android.permission.SET_WALLPAPER")]
	                public virtual unsafe void Clear ([global::Android.Runtime.GeneratedEnum] Android.App.WallpaperManagerFlags which)
	                {
	                        const string __id = "clear.(I)V";

resulting in errors such as:

	CannotChangeAttribute : Attribute 'Android.Runtime.RequiresPermissionAttribute' on 'Android.App.WallpaperManager.Clear(Android.App.WallpaperManagerFlags)' changed from '[RequiresPermissionAttribute("android.permission.SET_WALLPAPER")]' in the contract to '[RequiresPermissionAttribute("android.permission.SET_WALLPAPER")]' in the implementation.

We should similarly fixup `generator` so that these duplicate
custom attributes are *not* emitted.
@jonpryor
Copy link
Contributor

See also: dotnet/android#6424

@jpobst jpobst marked this pull request as ready for review October 26, 2021 18:13
@jonpryor jonpryor merged commit 79744f6 into main Oct 26, 2021
@jonpryor jonpryor deleted the annotation-fix branch October 26, 2021 18:36
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request Oct 27, 2021
Context: dotnet/java-interop#897
Context: f6011d9

Commit f6011d9 updated external/xamarin-android-tools so that the
Android SDK platform-tools r31.0.3 package would be preferred, but
*didn't* update `$(XAPlatformToolsVersion)` to cause the
xamarin-android build to use platform-tools r31.0.3, because:

> 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+ (?!):

dotnet/java-interop#897 is a proposed fix for (2), and should allow
us to use platform-tools r31.0.3 for the xamarin-android build.

WIP WIP WIP WIP

Context: dotnet/java-interop#897 (comment)

This does not fully build for me locally, because "duplicate" custom
attributes are being emitted:

	$ diff -u obj/Debug/monoandroid10/android-{27,28}/mcw/Android.App.WallpaperManager.cs
	@@ -553,7 +553,7 @@
	                // Metadata.xml XPath method reference: path="/api/package[@name='android.app']/class[@name='WallpaperManager']/method[@name='clear' and count(parameter)=1 and parameter[1][@type='int']]"
	                [global::System.Runtime.Versioning.SupportedOSPlatformAttribute ("android24.0")]
	                [Register ("clear", "(I)V", "GetClear_IHandler", ApiSince = 24)]
	-               [global::Android.Runtime.RequiresPermission ("android.permission.SET_WALLPAPER")]
	+               [global::Android.Runtime.RequiresPermission ("android.permission.SET_WALLPAPER")][global::Android.Runtime.RequiresPermission ("android.permission.SET_WALLPAPER")]
	                public virtual unsafe void Clear ([global::Android.Runtime.GeneratedEnum] Android.App.WallpaperManagerFlags which)
	                {
	                        const string __id = "clear.(I)V";

resulting in errors such as:

	CannotChangeAttribute : Attribute 'Android.Runtime.RequiresPermissionAttribute' on 'Android.App.WallpaperManager.Clear(Android.App.WallpaperManagerFlags)' changed from '[RequiresPermissionAttribute("android.permission.SET_WALLPAPER")]' in the contract to '[RequiresPermissionAttribute("android.permission.SET_WALLPAPER")]' in the implementation.

We should similarly fixup `generator` so that these duplicate
custom attributes are *not* emitted.
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request Oct 28, 2021
Context: dotnet/java-interop#897
Context: f6011d9

Commit f6011d9 updated external/xamarin-android-tools so that the
Android SDK platform-tools r31.0.3 package would be preferred, but
*didn't* update `$(XAPlatformToolsVersion)` to cause the
xamarin-android build to use platform-tools r31.0.3, because:

> 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+ (?!):

dotnet/java-interop#897 is a proposed fix for (2), and should allow
us to use platform-tools r31.0.3 for the xamarin-android build.

WIP WIP WIP WIP

Context: dotnet/java-interop#897 (comment)

This does not fully build for me locally, because "duplicate" custom
attributes are being emitted:

	$ diff -u obj/Debug/monoandroid10/android-{27,28}/mcw/Android.App.WallpaperManager.cs
	@@ -553,7 +553,7 @@
	                // Metadata.xml XPath method reference: path="/api/package[@name='android.app']/class[@name='WallpaperManager']/method[@name='clear' and count(parameter)=1 and parameter[1][@type='int']]"
	                [global::System.Runtime.Versioning.SupportedOSPlatformAttribute ("android24.0")]
	                [Register ("clear", "(I)V", "GetClear_IHandler", ApiSince = 24)]
	-               [global::Android.Runtime.RequiresPermission ("android.permission.SET_WALLPAPER")]
	+               [global::Android.Runtime.RequiresPermission ("android.permission.SET_WALLPAPER")][global::Android.Runtime.RequiresPermission ("android.permission.SET_WALLPAPER")]
	                public virtual unsafe void Clear ([global::Android.Runtime.GeneratedEnum] Android.App.WallpaperManagerFlags which)
	                {
	                        const string __id = "clear.(I)V";

resulting in errors such as:

	CannotChangeAttribute : Attribute 'Android.Runtime.RequiresPermissionAttribute' on 'Android.App.WallpaperManager.Clear(Android.App.WallpaperManagerFlags)' changed from '[RequiresPermissionAttribute("android.permission.SET_WALLPAPER")]' in the contract to '[RequiresPermissionAttribute("android.permission.SET_WALLPAPER")]' in the implementation.

We should similarly fixup `generator` so that these duplicate
custom attributes are *not* emitted.
@github-actions github-actions bot locked and limited conversation to collaborators Apr 13, 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.

AndroidAnnotationSupport needs better invalid XML fixup
2 participants