-
Notifications
You must be signed in to change notification settings - Fork 59
[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
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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. |
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.
See also: dotnet/android#6424 |
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.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
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.