Skip to content

Conversation

@gugavaro
Copy link
Contributor

@gugavaro gugavaro commented Feb 11, 2020

We are marking as obsolete error all const fields that already have an enum and related interfaceconsts.

Reference: dotnet/java-interop#568

Additionally this remove the API Compatibility job from our build pipeline YAML. This step has been superseded by the api checker added in #3884.

Comment on lines 1273 to 1274
CannotChangeAttribute : Attribute 'Android.Runtime.RegisterAttribute' on 'Android.Widget.ViewFlipper.FlipInterval.get()' changed from '[RegisterAttribute("getFlipInterval", "()I", "GetGetFlipIntervalHandler")]' in the contract to '[RegisterAttribute("getFlipInterval", "()I", "GetGetFlipIntervalHandler", ApiSince=29)]' in the implementation. No newline at end of file
CannotChangeAttribute : Attribute 'Android.Runtime.RegisterAttribute' on 'Android.Widget.ViewFlipper.FlipInterval.get()' changed from '[RegisterAttribute("getFlipInterval", "()I", "GetGetFlipIntervalHandler")]' in the contract to '[RegisterAttribute("getFlipInterval", "()I", "GetGetFlipIntervalHandler", ApiSince=29)]' in the implementation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What changed here? These appear the same to me.

@gugavaro gugavaro force-pushed the gugavaro_xa_deprecatedfields branch 2 times, most recently from 07861f4 to eac90a5 Compare February 12, 2020 21:17
jonpryor pushed a commit to dotnet/java-interop that referenced this pull request Feb 12, 2020
Context: #509
Context: dotnet/android#4248

We are generating fields that have been marked as `[Obsolete]` for a
long time.

In the effort to remove all fields that now we believe should not be
part of our API, we are marking them as `[Obsolete(..., error:true)]`
so we can remove them in a future release.

Here are some scenarios these changes are addressing:

We should `[Obsolete(error:true)]` *fields* on "interface proxy" types
such as [`Android.Content.ComponentCallbacks2`][0], which contains
fields for the [`android.content.ComponentCallbacks2`][1] interface
and lives "alongside" the [Android.Content.IComponentCallbacks2][2]
interface.  Additionally, all of these fields should *eventually* be
moved into the appropriate C# interface via Default Interface Members,
e.g. every field within `Android.Content.ComponentCallbacks2` should
*instead* reside in the `Android.Content.IComponentCallbacks2` type.

TODO:

  * We should `[Obsolete(error:true)]` the interface proxy types
    themselves.

  * We also `[Obsolete(error:true)]` all of the nested
    `InterfaceConsts` types, such as
    `Android.OS.Binder.InterfaceConsts`.

  * We should also `[Obsolete(error:true)]` all members which are
    already [Obsolete].

[0]: https://docs.microsoft.com/en-us/dotnet/api/android.content.componentcallbacks2?view=xamarin-android-sdk-9
[1]: https://developer.android.com/reference/android/content/ComponentCallbacks2
[2]: https://docs.microsoft.com/en-us/dotnet/api/android.content.icomponentcallbacks2?view=xamarin-android-sdk-9
@gugavaro gugavaro force-pushed the gugavaro_xa_deprecatedfields branch from eac90a5 to 72a1c7c Compare February 12, 2020 22:07
@gugavaro gugavaro force-pushed the gugavaro_xa_deprecatedfields branch from 72a1c7c to 73a88eb Compare February 12, 2020 22:26
@jpobst
Copy link
Contributor

jpobst commented Feb 14, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jonpryor
Copy link
Contributor

jonpryor commented Feb 16, 2020

Please remove the external/xamarin-android-api-compatibility submodule reference as well, to further indicate that it isn't used anymore.

@jpobst
Copy link
Contributor

jpobst commented Feb 18, 2020

👍 Removed submodule.

@jonpryor
Copy link
Contributor

Squash-and-merge "title":

Bump to xamarin/Java.Interop/master@8f30933a (#4248)

Squash-and-merge "body":

Context: https://github.com/xamarin/java.interop/commit/8f30933a89b2ad3259740f21587f50dc8ea1a0e2

Changes: https://github.com/xamarin/java.interop/compare/3226a4b57ad84574a69a151a310b077cfe69ee19...8f30933a89b2ad3259740f21587f50dc8ea1a0e2

  * xamarin/Java.Interop@8f30933: [generator] Mark some Obsolete fields as errors (#568)
  * xamarin/Java.Interop@47201c4: [generator] Change protected members in final class to private (#569)
  * xamarin/Java.Interop@c5815fb: [generator] Be smarter about when members with same names need "new" (#567)
  * xamarin/Java.Interop@bfc0273: [generator] Ensure property setter parameter name is "value" (#566)

This Java.Interop bump includes two changes which "break" API:

  * Certain fields are changed from `[Obsolete]` to
    `[Obsolete(error:true)]`, turning *use* of the field within C#
    code into a CS0619 error.

  * The C# compiler has long warned about the presence of `protected`
    members within `sealed` types, as they cannot be used.  However,
    they were still emitted, resulting in CS0628 warnings when
    compiling `Mono.Android.dll`.

We consider changing `[Obsolete]` to `[Obsolete(error:true)]` to be
acceptable because this is being done to `const` fields, so this
won't break ABI, as there are no IL references to `const` fields, and
because (1) there are "replacement" members which can be used, and
(2) these fields have been obsolete for *years*.

The "problem" here is that `Microsoft.DotNet.ApiCompat.exe`, which
the `<CheckApiCompatibility/>` task uses from
`src/Mono.Android/Mono.Android.targets`, sees every such change as
an API break.  This in and of itself is plausibly sensible, but the
error message *itself* is bananas, e.g.:

	namespace Android.AccessibilityServices {
	  public abstract partial class AccessibilityService : Android.App.Service {
	    [Register ("GESTURE_SWIPE_DOWN", ApiSince = 16)]
	    [Obsolete ("This constant will be removed in the future version. Use Android.AccessibilityServices.AccessibilityGesture enum directly instead of this field.", error: true)]
	    public const Android.AccessibilityServices.AccessibilityGesture GestureSwipeDown = (Android.AccessibilityServices.AccessibilityGesture) 2;
	  }
	}

results in:

	CannotRemoveAttribute : Attribute 'System.ObsoleteAttribute' exists on 'Android.AccessibilityServices.AccessibilityGesture Android.AccessibilityServices.AccessibilityService.GestureSwipeDown' in the contract but not the implementation.

The message implies that `[Obsolete]` was removed, which isn't
strictly true!  `[Obsolete]` is still there!  It's just that the
constructor parameters have changed.

This results in a ginormous change to
`tests/api-compatibility/acceptable-breakages-vReference.txt`.

The "`protected` members within `sealed` types" fix for CS0628 also
resulted in errors from the Tests > API Compatibility job, which runs
via `external/xamarin-android-api-compatibility`.

Given that commit 07e74771 was intended to *replace* the
Tests > API Compatibility job, *remove* the 
`external/xamarin-android-api-compatibility` submodule reference.
We will no longer be using that submodule for API compat checks.

Co-authored-by: Jonathan Pobst <monkey@jpobst.com>

@jonpryor jonpryor merged commit 771ee81 into master Feb 19, 2020
jonpryor pushed a commit to dotnet/java-interop that referenced this pull request Feb 19, 2020
Context: #509
Context: dotnet/android#4248

We are generating fields that have been marked as `[Obsolete]` for a
long time.

In the effort to remove all fields that now we believe should not be
part of our API, we are marking them as `[Obsolete(..., error:true)]`
so we can remove them in a future release.

Here are some scenarios these changes are addressing:

We should `[Obsolete(error:true)]` *fields* on "interface proxy" types
such as [`Android.Content.ComponentCallbacks2`][0], which contains
fields for the [`android.content.ComponentCallbacks2`][1] interface
and lives "alongside" the [Android.Content.IComponentCallbacks2][2]
interface.  Additionally, all of these fields should *eventually* be
moved into the appropriate C# interface via Default Interface Members,
e.g. every field within `Android.Content.ComponentCallbacks2` should
*instead* reside in the `Android.Content.IComponentCallbacks2` type.

TODO:

  * We should `[Obsolete(error:true)]` the interface proxy types
    themselves.

  * We also `[Obsolete(error:true)]` all of the nested
    `InterfaceConsts` types, such as
    `Android.OS.Binder.InterfaceConsts`.

  * We should also `[Obsolete(error:true)]` all members which are
    already [Obsolete].

[0]: https://docs.microsoft.com/en-us/dotnet/api/android.content.componentcallbacks2?view=xamarin-android-sdk-9
[1]: https://developer.android.com/reference/android/content/ComponentCallbacks2
[2]: https://docs.microsoft.com/en-us/dotnet/api/android.content.icomponentcallbacks2?view=xamarin-android-sdk-9
jonpryor pushed a commit that referenced this pull request Feb 19, 2020
Context: dotnet/java-interop@8f30933

Changes: dotnet/java-interop@423e27f...fefe0ca

  * dotnet/java-interop@fefe0ca: [generator] Mark some Obsolete fields as errors (#568)
  * dotnet/java-interop@d969a0c: [generator] Change protected members in final class to private (#569)
  * dotnet/java-interop@3c4accf: [generator] Be smarter about when members with same names need "new" (#567)
  * dotnet/java-interop@41b87d0: [generator] Ensure property setter parameter name is "value" (#566)

This Java.Interop bump includes two changes which "break" API:

  * Certain fields are changed from `[Obsolete]` to
    `[Obsolete(error:true)]`, turning *use* of the field within C#
    code into a CS0619 error.

  * The C# compiler has long warned about the presence of `protected`
    members within `sealed` types, as they cannot be used.  However,
    they were still emitted, resulting in CS0628 warnings when
    compiling `Mono.Android.dll`.

We consider changing `[Obsolete]` to `[Obsolete(error:true)]` to be
acceptable because this is being done to `const` fields, so this
won't break ABI, as there are no IL references to `const` fields, and
because (1) there are "replacement" members which can be used, and
(2) these fields have been obsolete for *years*.

The "problem" here is that `Microsoft.DotNet.ApiCompat.exe`, which
the `<CheckApiCompatibility/>` task uses from
`src/Mono.Android/Mono.Android.targets`, sees every such change as
an API break.  This in and of itself is plausibly sensible, but the
error message *itself* is bananas, e.g.:

	namespace Android.AccessibilityServices {
	  public abstract partial class AccessibilityService : Android.App.Service {
	    [Register ("GESTURE_SWIPE_DOWN", ApiSince = 16)]
	    [Obsolete ("This constant will be removed in the future version. Use Android.AccessibilityServices.AccessibilityGesture enum directly instead of this field.", error: true)]
	    public const Android.AccessibilityServices.AccessibilityGesture GestureSwipeDown = (Android.AccessibilityServices.AccessibilityGesture) 2;
	  }
	}

results in:

	CannotRemoveAttribute : Attribute 'System.ObsoleteAttribute' exists on 'Android.AccessibilityServices.AccessibilityGesture Android.AccessibilityServices.AccessibilityService.GestureSwipeDown' in the contract but not the implementation.

The message implies that `[Obsolete]` was removed, which isn't
strictly true!  `[Obsolete]` is still there!  It's just that the
constructor parameters have changed.

This results in a ginormous change to
`tests/api-compatibility/acceptable-breakages-vReference.txt`.

The "`protected` members within `sealed` types" fix for CS0628 also
resulted in errors from the Tests > API Compatibility job, which runs
via `external/xamarin-android-api-compatibility`.

Given that commit 07e7477 was intended to *replace* the
Tests > API Compatibility job, *remove* the
`external/xamarin-android-api-compatibility` submodule reference.
We will no longer be using that submodule for API compat checks.

Co-authored-by: Jonathan Pobst <monkey@jpobst.com>
@jpobst jpobst deleted the gugavaro_xa_deprecatedfields branch February 20, 2020 22:08
@github-actions github-actions bot locked and limited conversation to collaborators Jan 28, 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.

5 participants