Skip to content

api-compatibility needs to check [Register] attribute values. #1118

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

Closed
jonpryor opened this issue Dec 15, 2017 · 2 comments
Closed

api-compatibility needs to check [Register] attribute values. #1118

jonpryor opened this issue Dec 15, 2017 · 2 comments
Assignees
Labels
Area: xamarin-android Build Issues building the xamarin-android repo *itself*.

Comments

@jonpryor
Copy link
Member

Related: PR #930
Related: https://bugzilla.xamarin.com/show_bug.cgi?id=60069

We need an API compatibility check that not only ensure that we don't break API -- which tests/api-compatibility does -- but also that we don't break semantics. The problem with Bug #60069 was that the semantics of Build.Serial changed from a Java field read to a Java method invocation, and since the Build.getSerial() method that was invoked only existed in API-26, the result was that the formerly working expression Build.Serial now threw everywhere.

We need to check for and prevent this in the future.

My current idea is that we improve mono-api-html to check the values of custom attributes, and warn if they change. For example, with Build.Serial the [Register] attribute contents changed:

		// API < 26, field use:
		[Register ("SERIAL", ApiSince = 9)]
		public static string Serial {
			get {
				const string __id = "SERIAL.Ljava/lang/String;";

				var __v = _members.StaticFields.GetObjectValue (__id);
				return JNIEnv.GetString (__v.Handle, JniHandleOwnership.TransferLocalRef);
			}
		}

		// API >= 26, method invocation:
		[Register ("getSerial", "()Ljava/lang/String;", "", ApiSince = 26)]
		public static string Serial {
			get {
				const string __id = "getSerial.()Ljava/lang/String;";

				try {
					var __rm = _members.StaticMethods.InvokeObjectMethod (__id, null);
					return JNIEnv.GetString (__rm.Handle, JniHandleOwnership.TransferLocalRef);
				} finally {
				}
			}
		}
@jonpryor jonpryor added this to the d16-1 milestone Dec 15, 2018
@jonpryor jonpryor self-assigned this Dec 15, 2018
@jonpryor
Copy link
Member Author

jonpryor commented Dec 15, 2018

Given the various issues we've had around mono-api-html-based API checking -- see also xamarin/xamarin-android-api-compatibility@a773887 -- I think the "fix" here is to start using Microsoft.DotNet.GenAPI to generate "reference sources" for Mono.Android.dll/etc. and use those "reference sources" for API breakage checking.

The api-snapshot repo contains prebuilt binaries: xamarin/xamarin-android-api-compatibility@a773887

This will make things "more brittle" -- any API breakage, including member additions, will thus be reported -- but it will also capture custom attribute values, so this is quite likely a net win.

@jonpryor jonpryor added the Area: xamarin-android Build Issues building the xamarin-android repo *itself*. label Dec 15, 2018
@pjcollins pjcollins modified the milestones: d16-1, Under Consideration Jun 20, 2019
@jonpryor
Copy link
Member Author

jonpryor pushed a commit that referenced this issue Oct 28, 2020
Fixes: xamarin/monodroid#1121
Fixes: xamarin/monodroid#1123

Changes: http://github.com/xamarin/monodroid/compare/767f647151936303c294d154d0d0a4da8b601464...04b0423ea298eda7263ba23e64df63c940ebddad

  * xamarin/monodroid@04b0423ea: Bump android-sdk-installer, androidtools, xamarin-android (#1128)
  * xamarin/monodroid@b0f824253: [tools/msbuild] Update RunActivity to use Async methods. (#1127)
  * xamarin/monodroid@ad6ea2a3c: [tools/msbuild] changes not deployed in some cases (#1125)
  * xamarin/monodroid@dfa0cba44: [tools/msbuild] Add check for 'ro.boot.disable_runas' (#1124)
  * xamarin/monodroid@3f2ca1173: [tools/msbuild] Add additional timing information for FastDeploy. (#1126)
  * xamarin/monodroid@d47db99c7: [tools/msbuild] fix _Run target for .NET 6 (#1118)
  * xamarin/monodroid@49a6dd572: Bump to xamarin/xamarin-android/master@e0999a43 (#1120)
@ghost ghost locked as resolved and limited conversation to collaborators Jun 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Area: xamarin-android Build Issues building the xamarin-android repo *itself*.
Projects
None yet
Development

No branches or pull requests

4 participants