Skip to content
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

Decorate all APIs with correct SupportedOSPlatform attributes #5338

Closed
marek-safar opened this issue Nov 27, 2020 · 9 comments
Closed

Decorate all APIs with correct SupportedOSPlatform attributes #5338

marek-safar opened this issue Nov 27, 2020 · 9 comments
Assignees
Labels
Area: Mono.Android Issues with the Android API binding (Mono.Android.dll).
Milestone

Comments

@marek-safar
Copy link
Contributor

As part of netcore migration, we need to correctly annotate all APIs with .NET 6 to work correctly with existing tooling (primary existing analyzers). The attributes already exist in .NET5 and could be extended if necessary.

Available attributes are listed at https://github.com/dotnet/designs/blob/main/accepted/2020/platform-checks/platform-checks.md#attributes

@AmrAlSayed0
Copy link

After reading the Platform compatibility analyzer doc page I saw attributes like [SupportedOSPlatform("Android6.0")]. I'm curious if the effort for this issue is going to include the android only API? For example:

//This is the C# class that is a binding of android.view.View
//(https://developer.android.com/reference/android/view/View.html)
public class View
{
    [SupportedOSPlatform("Android8.0")] // This means that this property can only be used on Android 8.0 or above
    //which corresponds to API 26 or above since this is the version it was added in
    //(https://developer.android.com/reference/android/view/View.html#getTooltipText())
    public string TooltipText { get; set; }
}

Or is only the net5.0 API going to be annotated?

@marek-safar
Copy link
Contributor Author

This issue is for Android APIs only

@pjcollins pjcollins added this to the One .NET milestone Dec 1, 2020
@jonathanpeppers
Copy link
Member

I think the attributes will actually be the API level such as:

[SupportedOSPlatform("android30")]

See: dotnet/runtime#43531

We'll have to see if we have to see if we have to put android30.0, I but I think we should try leaving off the .0.

@jonathanpeppers
Copy link
Member

Correction: https://docs.microsoft.com/dotnet/standard/analyzers/platform-compat-analyzer#how-the-analyzer-determines-platform-dependency

major.minor is required

So I think we will have to use android30.0, etc.

@jonpryor
Copy link
Member

jonpryor commented Dec 2, 2020

Additionally, SupportedOSPlatform is only available in .NET 5+; thus, generator output will need to wrap it into a #if NET5 block:

	// Metadata.xml XPath class reference: path="/api/package[@name='android.app']/class[@name='Activity']"
	[global::Android.Runtime.Register ("android/app/Activity", DoNotGenerateAcw=true)]
#if NET5_0_OR_GREATER
	[global: System.Runtime.Versioning.SupportedOSPlatform ("android")]
#endif  // NET5_0_OR_GREATER
	public partial class Activity {
		public virtual unsafe bool IsActivityTransitionRunning {
			// Metadata.xml XPath method reference: path="/api/package[@name='android.app']/class[@name='Activity']/method[@name='isActivityTransitionRunning' and count(parameter)=0]"
			[Register ("isActivityTransitionRunning", "()Z", "GetIsActivityTransitionRunningHandler", ApiSince = 26)]
#if NET5_0_OR_GREATER
			[global::System.Runtime.Versioning.SupportedOSPlatform ("android26.0")]
#endif  // NET5_0_OR_GREATER
			get {}
		}
	}

Related: https://github.com/dotnet/designs/blob/main/accepted/2020/or-greater-defines/or-greater-defines.md#doc-changes

@jpobst
Copy link
Contributor

jpobst commented Dec 2, 2020

I think it's probably easier to define our own [SupportedOSPlatform] attribute and include it if not targeting net5.0+.

@marek-safar
Copy link
Contributor Author

I think it's probably easier to define our own [SupportedOSPlatform] attribute and include it if not targeting net5.0+.

You could public [Conditional("NEVER")] on the local version of SupportedOSPlatformAttribute to be always removed by compiler for non-netcore builds

@jpobst
Copy link
Contributor

jpobst commented Dec 2, 2020

Implementation details aside, the good news is we do already have this data and already put it in our bindings, just in a different place (ApiSince), so it won't be hard to add the new annotations.

[global::Android.Runtime.Register ("android/accessibilityservice/FingerprintGestureController$FingerprintGestureCallback", DoNotGenerateAcw=true, ApiSince = 26)]
public abstract partial class FingerprintGestureCallback : Java.Lang.Object
{
  [Register (".ctor", "()V", "", ApiSince = 26)]
  public unsafe FingerprintGestureCallback () : base (IntPtr.Zero, JniHandleOwnership.DoNotTransfer)
  {
    ...
  }
}

We will probably need to manually audit our hand-bound code also.

jonpryor pushed a commit to dotnet/java-interop that referenced this issue Jan 11, 2021
)

Context: dotnet/android#5338

.NET 5 provides a new
[`System.Runtime.Versioning.SupportedOSPlatformAttribute`][0] custom
attribute which is used to specify on which platforms and platform
versions an API is available.  This is used to build analyzers to give
users warnings if they are trying to use an API when it will not be
available on their target platform.

`SupportedOSPlatformAttribute` is fundamentally the same as our
existing `RegisterAttribute.ApiSince` property, except tooling has
actually been built to consume the information.

As such, we need `generator` support to put this information into
`Mono.Android.dll`:

	partial class Activity {
	    // Metadata.xml XPath method reference: path="/api/package[@name='android.app']/class[@name='Activity']/method[@name='dismissKeyboardShortcutsHelper' and count(parameter)=0]"
	    [global::System.Runtime.Versioning.SupportedOSPlatformAttribute ("android24.0")]
	    [Register ("dismissKeyboardShortcutsHelper", "()V", "", ApiSince = 24)]
	    public unsafe void DismissKeyboardShortcutsHelper () { … }
	}


Some interesting notes:

  - `SupportedOSPlatformAttribute` is only available in .NET 5+, so
    we include a local version for earlier frameworks so we can
    compile without needing to `#ifdef` every attribute use.

  - The local version is marked as `[Conditional ("NEVER")]` so the
    attributes will not actually get compiled into the resulting
    pre-NET5.0 assembly.

  - The attribute cannot be placed on interfaces or fields, so we
    aren't able to annotate them.

  - Our minimum supported API for .NET 6 is 21, so we only write
    attributes for API added in versions newer than 21, as API added
    earlier are always available.

[0]: https://docs.microsoft.com/en-us/dotnet/api/system.runtime.versioning.supportedosplatformattribute?view=net-5.0
jonpryor pushed a commit to dotnet/java-interop that referenced this issue May 17, 2021
…)" (#841)

Context: dotnet/android#5338

This reverts commit 00862ad.

Now that we are properly building with `$(TargetFramework)`=net6.0
(4d0cba6), we can re-enable the `[SupportedOSPlatform]` support,
as originally introduced in da12df4.

Emit the [`System.Runtime.Versioning.SupportedOSPlatformAttribute`][0]
custom attribute which is used to specify on which platforms and
platform versions an API is available.  This is used to build analyzers
to give users warnings if they are trying to use an API when it will
not be available on their target platform.

[0]: https://docs.microsoft.com/en-us/dotnet/api/system.runtime.versioning.supportedosplatformattribute?view=net-5.0
@jpobst
Copy link
Contributor

jpobst commented May 26, 2021

This has been implemented in Java.Interop:
dotnet/java-interop@412e974

And bumped in XA:
#5945

@jpobst jpobst closed this as completed May 26, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Area: Mono.Android Issues with the Android API binding (Mono.Android.dll).
Projects
None yet
Development

No branches or pull requests

6 participants