-
Couldn't load subscription status.
- Fork 63
[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
Conversation
|
This works, but I'd like "more". Testing locally, I see errors such as: 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. |
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.
|
See also: dotnet/android#6424 |
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.
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.
Fixes #883
Due to invalid XML provided in Google's
annotations.zipfile, we run it through the more forgiving HtmlAgilityPack to attempt to fix it to valid XML.However, given this snippet:
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"as unescaped as well, thinking we want to value to be the literal string""". When it writes out the valid XML, it realizes it needs to escape the ampersand, and writes out&quot;, which breaks our usage of these annotations.The fix is to "unescape" every
"to a"so that it will be escaped correctly when saved as valid XML.