Adding new api break workflow that uses Microsoft.DotNet.ApiCompat.dll #3884

gugavaro · 2019-11-06T15:13:11Z

Our current api check workflow is not able to detect some api breaks or is flagging api breaks that actually should not be raised.

examples are:
If we change Attributes current check is unable to validate that.
Also, if we add abstract methods to an abstract class, it also does not flag as break

Some instances of false positives are:
When a base class change.
from: ClassA : Object, ClassB: Object, ClassC: ClassA
to: ClassA : Object, ClassB: ClassA, ClassC: ClassB

this should not be considered a break, but our current check flags this as a break.

Also, using Microsoft.DotNet.ApiCompat.dll in our workflow makes sure we are following the same workflow .net team and azure team are using.

Another change is related to make the check as part of the project build step, so we can catch breakages much earlier and without any extra step.

original issue: #1118

here is an example of how the build would report the breakage:

AfterBuild:
CheckApiCompatibility for ApiLevel: v10.0
Compat issues with assembly Mono.Android:
CannotRemoveBaseTypeOrInterface : Type 'Java.Util.Concurrent.Atomic.LongAdder' does not inherit from base type 'Java.Util.Concurrent.Atomic.Striped64' in the implementation but it does in the contract.
Total Issues: 1
/home/gugavaro/Work/microsoft/Xamarin/xamarin-android-1/src/Mono.Android/Mono.Android.csproj(384,5): error : CheckApiCompatibility found nonacceptable Api breakages for ApiLevel: v10.0.
Done Building Project "/home/gugavaro/Work/microsoft/Xamarin/xamarin-android-1/src/Mono.Android/Mono.Android.csproj" (default targets) -- FAILED.

Build FAILED.

"/home/gugavaro/Work/microsoft/Xamarin/xamarin-android-1/src/Mono.Android/Mono.Android.csproj" (default target) (1) ->
(AfterBuild target) ->
/home/gugavaro/Work/microsoft/Xamarin/xamarin-android-1/src/Mono.Android/Mono.Android.csproj(384,5): error : CheckApiCompatibility found nonacceptable Api breakages for ApiLevel: v10.0.

141 Warning(s)
1 Error(s)

...n.Android.Tools.BootstrapTasks/Xamarin.Android.Tools.BootstrapTasks/CheckApiCompatibility.cs

src/Mono.Android/Mono.Android.csproj

...n.Android.Tools.BootstrapTasks/Xamarin.Android.Tools.BootstrapTasks/CheckApiCompatibility.cs

pjcollins · 2019-11-07T15:00:55Z

@gugavaro to fix the NuGet restore issue on the test jobs I think we'll need to update this YAML template to include feedsToUse and nugetConfigPath params:

- task: NuGetCommand@2
  displayName: nuget restore Xamarin.Android solutions
  inputs:
    restoreSolution: '**/Xamarin.Android*.sln'
    feedsToUse: config
    nugetConfigPath:  $(System.DefaultWorkingDirectory)/NuGet.config

Reference: https://docs.microsoft.com/en-us/azure/devops/pipelines/tasks/package/nuget?view=azure-devops#yaml-snippet

gugavaro · 2019-11-07T17:15:25Z

Thanks @pjcollins, I have updated the yaml file.

NuGet.config

...n.Android.Tools.BootstrapTasks/Xamarin.Android.Tools.BootstrapTasks/CheckApiCompatibility.cs

jonpryor · 2019-11-10T18:31:50Z

Given a previously stated intention of "merging" the xamarin-android and xamarin-android-api-compatibility repos -- because the new "reference .cs file" will be much smaller than the mono-api-info-generated XML file, making it sane to have everything within this repo -- why are the new "inter-API diff" files going into xamarin-android-api-compatibility ?

Why not place them into tests\api-compatibility?

src/Mono.Android/Mono.Android.csproj

...n.Android.Tools.BootstrapTasks/Xamarin.Android.Tools.BootstrapTasks/CheckApiCompatibility.cs

jonpryor · 2019-11-10T18:43:23Z

We should review the App Bundle crash: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=3223480&view=ms.vss-test-web.build-test-results-tab&runId=9669612&resultId=100000&paneView=debug

See if it's fixed by PR #3888, which fixes:

11-04 12:27:55.722  3339  3339 E mono    : Unhandled Exception:
11-04 12:27:55.722  3339  3339 E mono    : System.DllNotFoundException: __Internal assembly:<unknown assembly> type:<unknown type> member:(null)
11-04 12:27:55.722  3339  3339 E mono    :   at (wrapper managed-to-native) Android.Runtime.JNIEnv.monodroid_timing_start(string)

jonpryor · 2019-11-10T18:44:18Z

The PR message mentions an error that doesn't appear to be fixed in the PR itself, and also doesn't error on a PR build:

AfterBuild:
CheckApiCompatibility for ApiLevel: v10.0
Compat issues with assembly Mono.Android:
CannotRemoveBaseTypeOrInterface : Type 'Java.Util.Concurrent.Atomic.LongAdder' does not inherit from base type 'Java.Util.Concurrent.Atomic.Striped64' in the implementation but it does in the contract.
Total Issues: 1
/home/gugavaro/Work/microsoft/Xamarin/xamarin-android-1/src/Mono.Android/Mono.Android.csproj(384,5): error : CheckApiCompatibility found nonacceptable Api breakages for ApiLevel: v10.0.
Done Building Project "/home/gugavaro/Work/microsoft/Xamarin/xamarin-android-1/src/Mono.Android/Mono.Android.csproj" (default targets) -- FAILED.

Am I misunderstanding something?

gugavaro · 2019-11-11T15:35:59Z

Given a previously stated intention of "merging" the xamarin-android and xamarin-android-api-compatibility repos -- because the new "reference .cs file" will be much smaller than the mono-api-info-generated XML file, making it sane to have everything within this repo -- why are the new "inter-API diff" files going into xamarin-android-api-compatibility ?

Why not place them into tests\api-compatibility?

DotNet.ApiCompat does not use .cs, we will not be able to get rid of xamarin-android-api-compatibility repos yet.
I took the decision of using DotNet.ApiCompat instead of GenApi after I talked with the .net team and they don't recommend we use the .cs (Microsoft.DotNet.GenApi) approach we initially had, instead they asked us to use Microsoft.DotNet.ApiCompat

Microsoft.DotNet.ApiCompat is the tool .net and azure uses and it does a lot of further checks that is not possible to do with a simple text comparison.

gugavaro · 2019-11-11T15:40:51Z

The PR message mentions an error that doesn't appear to be fixed in the PR itself, and also doesn't error on a PR build:

AfterBuild:
CheckApiCompatibility for ApiLevel: v10.0
Compat issues with assembly Mono.Android:
CannotRemoveBaseTypeOrInterface : Type 'Java.Util.Concurrent.Atomic.LongAdder' does not inherit from base type 'Java.Util.Concurrent.Atomic.Striped64' in the implementation but it does in the contract.
Total Issues: 1
/home/gugavaro/Work/microsoft/Xamarin/xamarin-android-1/src/Mono.Android/Mono.Android.csproj(384,5): error : CheckApiCompatibility found nonacceptable Api breakages for ApiLevel: v10.0.
Done Building Project "/home/gugavaro/Work/microsoft/Xamarin/xamarin-android-1/src/Mono.Android/Mono.Android.csproj" (default targets) -- FAILED.

Am I misunderstanding something?

I am not sure I understand the question. This message is an example of what we will see if there is a breakage. The check happens when we build Mono.Android, and if a break happens the build will fail with similar message.

The error does not happen on this PR because that error is an acceptable one based on the rules added on xamarin-android-api-compatibility repo
https://github.com/xamarin/xamarin-android-api-compatibility/blob/gugavaro_apicompat/v10.0.txt

jonpryor · 2019-11-12T16:05:49Z

The error does not happen on this PR because that error is an acceptable one based on the rules added on xamarin-android-api-compatibility repo

Ah. Which specific rule disables the warnings that were mentioned?

jonpryor · 2019-11-12T16:13:12Z

we will not be able to get rid of xamarin-android-api-compatibility repos yet.

We may not be able to remove them yet, but if the intention is to eventually remove them, why increase our use of them, via these new v*.txt files?

Additionally, v*.txt is not a good semantic name; what are they for? They're "for" Microsoft.DotNet.ApiCompat, but that's not at all obvious from the name.

Thus, let's look at usage:

var acceptableIssuesFile = Path.Combine (ApiCompatContractPath, $"{ApiLevel}.txt");

In context, these appear to be "contracts". Thus, instead of v*.txt, contract-v*.txt would be more meaningful.

Though looking at the file contents:

xamarin/xamarin-android-api-compatibility@a61271e...a9247ed#diff-3cc026f88bf794495bd1a9ddb20a6643R1

contract-issues-v10.txt or contract-rules-v10.txt may be "better" names.

Thus, instead of e.g. external/xamarin-android-api-compatibility/v10.txt, I would instead suggest that these new files be placed in in tests/api-compatibility/contract-rules-v10.txt. This is a more semantically meaningful name, and won't increase our dependency on the xamarin-android-api-compatibility repo.

gugavaro · 2019-11-13T14:59:18Z

The error does not happen on this PR because that error is an acceptable one based on the rules added on xamarin-android-api-compatibility repo

Ah. Which specific rule disables the warnings that were mentioned?

for this specific error, this line will allow that specific breakage
https://github.com/xamarin/xamarin-android-api-compatibility/blob/a9247ed847e039ec30467be9768a033288bd3ac2/v10.0.txt#L14

gugavaro · 2019-11-13T15:00:06Z

we will not be able to get rid of xamarin-android-api-compatibility repos yet.

We may not be able to remove them yet, but if the intention is to eventually remove them, why increase our use of them, via these new v*.txt files?

Additionally, v*.txt is not a good semantic name; what are they for? They're "for" Microsoft.DotNet.ApiCompat, but that's not at all obvious from the name.

Thus, let's look at usage:
var acceptableIssuesFile = Path.Combine (ApiCompatContractPath, $"{ApiLevel}.txt");
In context, these appear to be "contracts". Thus, instead of v*.txt, contract-v*.txt would be more meaningful.

Though looking at the file contents:

xamarin/xamarin-android-api-compatibility@a61271e...a9247eddiff-3cc026f88bf794495bd1a9ddb20a6643R1

contract-issues-v10.txt or contract-rules-v10.txt may be "better" names.

Thus, instead of e.g. external/xamarin-android-api-compatibility/v10.txt, I would instead suggest that these new files be placed in in tests/api-compatibility/contract-rules-v10.txt. This is a more semantically meaningful name, and won't increase our dependency on the xamarin-android-api-compatibility repo.

I agree with all your points and will fix all of them. �Thanks!!!

tests/api-compatibility/acceptable-breakages-v10.0.txt

src/Xamarin.Android.Build.Tasks/packages.config

...n.Android.Tools.BootstrapTasks/Xamarin.Android.Tools.BootstrapTasks/CheckApiCompatibility.cs

jonpryor · 2019-12-06T02:58:49Z

tests/api-compatibility/acceptable-breakages-v10.0.txt

@@ -0,0 +1,40 @@
+Compat issues with assembly Mono.Android:
+MembersMustExist : Member 'Android.Media.AudioRecord.add_RoutingChanged(System.EventHandler<Android.Media.AudioRecord.RoutingChangedEventArgs>)' does not exist in the implementation but it does exist in the contract.


...and I find this both horrifying and confusing: this sounds like a breaking change, yet it's not listed in https://github.com/xamarin/xamarin-android-api-compatibility/blob/master/inter-api-extra-v9.0-v10.0.txt nor in xamarin/xamarin-android-api-compatibility@336eeda (which must be viewed locally, as it's too big for GitHub to display), and I don't remember it coming up in the context of breaking changes previously.

$ git show 336eedaa51883d90100e30c95a007d8922ab19f0 | grep -i RoutingChanged # no output

Yet, when I diff, it's there:

$ diff -u src/Mono.Android/obj/Debug/android-28/mcw/Android.Media.AudioRecord.cs src/Mono.Android/obj/Debug/android-29/mcw/Android.Media.AudioRecord.cs - public event EventHandler<Android.Media.AudioRecord.RoutingChangedEventArgs> RoutingChanged { + public event EventHandler<Android.Media.AudioRoutingOnRoutingChangedEventArgs> RoutingChanged {

Why didn't our previous tooling find this?

For that matter, this finds nothing within the xamarin-android-api-compat repo:

$ grep '<event.*RoutingChanged' reference/Mono.Android.xml # no matches

(Feeling of horror increases...)

It took me quite a bit to remember this, but that's expected!
We were wrongly not generating that event, so that's why previous tool never complained about it.

see this: bcae941#diff-14048fd4cb6595cd0ad8cf1b41175ef5L1301

After my fix, we started generating that but only until api 28

We were wrongly not generating that event

OK, so it didn't previously exist

After my fix, we started generating that but only until api 28

...and this is where we made a mistake. If we weren't previously emitting it, we should have continued not emitting it, so that when we "introduced" it in API-29, it wouldn't be incompatible with previous versions.

😞

I gave you a wrong explanation, we are not adding it, we are actually only removing it from api 24 to api 28
we replace lines like:

<remove-node path="/api/package[@name='android.media']/interface[@name='AudioRouting']" />

to

<remove-node path="/api/package[@name='android.media']/interface[@name='AudioRouting']" api-since="24" api-until="28"/>

the issue was introduced when we fixed: dotnet/java-interop@7a31216

gugavaro · 2019-12-06T19:19:23Z

This PR now can only be merged after PR: #3997 be merged.

Context: #3884 Commit 91467bb updated the build system so that instead of trying to do "everthing" within a single `.sln` file -- which would result in various file share exceptions when attempting to build `Xamarin.Android.sln` within Visual Studio on Windows -- the build tree would instead become "stateful": 1. Build `Xamarin.Android.BootstrapTasks.sln`. 2. Then `Xamarin.Android.sln` can be built. 3. Then `Xamarin.Android-Tests.sln` can be built. (1) was handled "internally" via `msbuild /t:Prepare`. A result of 91467bb is that if e.g. `build-tools/Xamarin.Android.Tools.BootstrapTasks` were changed, building `src/Xamarin.Android.Build.Tasks/Xamarin.Android.Build.Tasks.csproj` would no longer cause `Xamarin.Android.Tools.BootstrapTasks` to be rebuilt. (This was the point, as rebuilding `Xamarin.Android.Tools.BootstrapTasks`/etc. is what caused the file sharing issues that we wanted fixed.) Overlooked in 91467bb is that `xaprepare` (invoked by `msbuild /t:Prepare`) is responsible for restoring NuGet packages on `.sln` files, but it would only restore packages for `Xamarin.Android.sln`. This restriction caused problems in PR #3884, which attempted to add NuGet packages to `build-tools/Xamarin.Android.BootstrapTasks`, but because that solution never had its NuGet packages restored, attempting to use the expected *outputs* of NuGet restore would fail: $ cat build-tools/Xamarin.Android.BootstrapTasks/packages.config <?xml version="1.0" encoding="utf-8"?> <packages> <package id="Microsoft.DotNet.ApiCompat" version="5.0.0-beta.19602.1" targetFramework="net472" /> <package id="Microsoft.DotNet.GenAPI" version="5.0.0-beta.19602.1" targetFramework="net472" /> <package id="Mono.Posix.NETStandard" version="1.0.0" targetFramework="net472" /> <package id="Xamarin.LibZipSharp" version="1.0.6" targetFramework="net472" /> </packages> $ msbuild -t:restore build-tools/Xamarin.Android.BootstrapTasks/Xamarin.Android.Tools.BootstrapTask.csproj … $ ls packages/Microsoft.D* ls: cannot access 'packages/Microsoft.D*': No such file or directory Improve build system sanity: when `xaprepare` restores NuGet packages, it should restore the following solutions: * `Xamarin.Android.BootstrapTasks.sln` * `Xamarin.Android.Build.Tasks.sln` * `Xamarin.Android.sln` This will allow PR #3884 to work as intended.

gugavaro · 2019-12-06T22:54:21Z

Update the changes with mono.android.dll from Release 10.1.1

jonpryor · 2019-12-08T01:24:11Z

/azp run

azure-pipelines · 2019-12-08T01:24:28Z

Azure Pipelines successfully started running 1 pipeline(s).

jonpryor · 2019-12-08T01:24:44Z

Commit message for squash-and-merge:

Fixes: https://github.com/xamarin/xamarin-android/issues/1118

Context: https://bugzilla.xamarin.com/show_bug.cgi?id=60069
Context: https://github.com/xamarin/xamarin-android/pull/930

Our current API check workflow is not able to detect some API breakage
or is flagging non-breaking API changes as breaking.

Examples of breakages not previously reported:

  * Changing the values of `[Register]` attributes may be a breaking
    change but is not flagged by `mono-html-html`.

  * Adding `abstract` methods to an `abstract` class

Examples of acceptable changes which elicit warnings:

  * Changing a base class in a compatible manner.

Instead of using `mono-api-info` + `mono-api-html`, use
[Microsoft.DotNet.ApiCompat][0] to perform API compatibility
validation, which is what the .NET and Azure teams use.

Microsoft.DotNet.ApiCompat reports `[Register]` changes:

	CannotChangeAttribute : Attribute 'Android.Runtime.RegisterAttribute' on 'Android.Database.Sqlite.SQLiteDatabase.Path.get()' changed from '[RegisterAttribute("getPath", "()Ljava/lang/String;", "")]' in the contract to '[RegisterAttribute("getPath", "()Ljava/lang/String;", "GetGetPathHandler")]' in the implementation.

and thus would have caught the `Build.SERIAL` breakage in #930.

To override the warnings produced by Microsoft.DotNet.ApiCompat,
the message can be copied into an appropriate
`tests/api-compatibility/acceptable-breakages-*.txt` file.

See `tests/api-compatibility/README.md` for details.

[0]: https://github.com/dotnet/arcade/tree/master/src/Microsoft.DotNet.ApiCompat

gugavaro requested review from dellis1972, jonathanpeppers, jonpryor, jpobst and pjcollins November 6, 2019 15:13

pjcollins reviewed Nov 6, 2019

View reviewed changes

...n.Android.Tools.BootstrapTasks/Xamarin.Android.Tools.BootstrapTasks/CheckApiCompatibility.cs Outdated Show resolved Hide resolved

jonathanpeppers reviewed Nov 6, 2019

View reviewed changes

src/Mono.Android/Mono.Android.csproj Outdated Show resolved Hide resolved

gugavaro requested review from jonathanpeppers and pjcollins November 6, 2019 21:31

jonathanpeppers reviewed Nov 6, 2019

View reviewed changes

...n.Android.Tools.BootstrapTasks/Xamarin.Android.Tools.BootstrapTasks/CheckApiCompatibility.cs Outdated Show resolved Hide resolved

gugavaro force-pushed the gugavaro_api-compat branch from 3b6154f to fa3d164 Compare November 6, 2019 22:31

gugavaro requested a review from jonathanpeppers November 6, 2019 22:39

jonathanpeppers approved these changes Nov 6, 2019

View reviewed changes

jonpryor reviewed Nov 10, 2019

View reviewed changes

NuGet.config Show resolved Hide resolved

jonpryor reviewed Nov 10, 2019

View reviewed changes

...n.Android.Tools.BootstrapTasks/Xamarin.Android.Tools.BootstrapTasks/CheckApiCompatibility.cs Outdated Show resolved Hide resolved

jonpryor reviewed Nov 10, 2019

View reviewed changes

src/Mono.Android/Mono.Android.csproj Outdated Show resolved Hide resolved

jonpryor reviewed Nov 10, 2019

View reviewed changes

...n.Android.Tools.BootstrapTasks/Xamarin.Android.Tools.BootstrapTasks/CheckApiCompatibility.cs Outdated Show resolved Hide resolved

gugavaro force-pushed the gugavaro_api-compat branch 2 times, most recently from cb26368 to 3c80c5b Compare November 12, 2019 15:22

jonpryor reviewed Dec 2, 2019

View reviewed changes

tests/api-compatibility/acceptable-breakages-v10.0.txt Show resolved Hide resolved

gugavaro force-pushed the gugavaro_api-compat branch 4 times, most recently from 315ba11 to 2e1532f Compare December 3, 2019 22:43

gugavaro requested a review from jonpryor December 4, 2019 01:51

jonpryor reviewed Dec 6, 2019

View reviewed changes

src/Xamarin.Android.Build.Tasks/packages.config Outdated Show resolved Hide resolved

jonpryor reviewed Dec 6, 2019

View reviewed changes

...n.Android.Tools.BootstrapTasks/Xamarin.Android.Tools.BootstrapTasks/CheckApiCompatibility.cs Outdated Show resolved Hide resolved

jonpryor reviewed Dec 6, 2019

View reviewed changes

gugavaro force-pushed the gugavaro_api-compat branch from 2e1532f to 0ffb294 Compare December 6, 2019 19:14

gugavaro force-pushed the gugavaro_api-compat branch from 0ffb294 to 3fa2d8b Compare December 6, 2019 22:53

Adding new api break workflow that uses Microsoft.DotNet.ApiCompat.dll

8ba6bfa

gugavaro force-pushed the gugavaro_api-compat branch from 3fa2d8b to 8ba6bfa Compare December 7, 2019 01:53

gugavaro requested a review from jonpryor December 7, 2019 15:57

jonpryor added 3 commits December 7, 2019 20:11

Copy-edit README

4faf295

Fix formatting

a79da52

Merge branch 'master' into gugavaro_api-compat

1f3411f

jonpryor merged commit 07e7477 into master Dec 8, 2019

jonpryor mentioned this pull request Dec 12, 2019

[build] Sanitize Microsoft.Build projects #3995

Merged

gugavaro deleted the gugavaro_api-compat branch January 31, 2020 17:50

gugavaro mentioned this pull request Feb 13, 2020

Fixing acceptable breakages file after set error=true on obsolete items #4248

Merged

brendanzagaeski added this to the In Preview: 10.3 (d16-6) milestone Mar 17, 2020

jpobst mentioned this pull request Apr 24, 2020

Need to review our mechanism for ensuring API compatibility #2068

Closed

github-actions bot locked and limited conversation to collaborators Jan 28, 2024

		@@ -0,0 +1,40 @@
		Compat issues with assembly Mono.Android:
		MembersMustExist : Member 'Android.Media.AudioRecord.add_RoutingChanged(System.EventHandler<Android.Media.AudioRecord.RoutingChangedEventArgs>)' does not exist in the implementation but it does exist in the contract.

Adding new api break workflow that uses Microsoft.DotNet.ApiCompat.dll #3884

Adding new api break workflow that uses Microsoft.DotNet.ApiCompat.dll #3884

Uh oh!

Conversation

gugavaro commented Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pjcollins commented Nov 7, 2019

Uh oh!

gugavaro commented Nov 7, 2019

Uh oh!

Uh oh!

Uh oh!

jonpryor commented Nov 10, 2019

Uh oh!

Uh oh!

Uh oh!

jonpryor commented Nov 10, 2019

Uh oh!

jonpryor commented Nov 10, 2019

Uh oh!

gugavaro commented Nov 11, 2019

Uh oh!

gugavaro commented Nov 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonpryor commented Nov 12, 2019

Uh oh!

jonpryor commented Nov 12, 2019

Uh oh!

gugavaro commented Nov 13, 2019

Uh oh!

gugavaro commented Nov 13, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonpryor Dec 6, 2019

Choose a reason for hiding this comment

Uh oh!

gugavaro Dec 6, 2019

Choose a reason for hiding this comment

Uh oh!

jonpryor Dec 6, 2019

Choose a reason for hiding this comment

Uh oh!

gugavaro Dec 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gugavaro commented Dec 6, 2019

Uh oh!

gugavaro commented Dec 6, 2019

Uh oh!

jonpryor commented Dec 8, 2019

Uh oh!

azure-pipelines bot commented Dec 8, 2019

Uh oh!

jonpryor commented Dec 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gugavaro commented Nov 6, 2019 •

edited

Loading

gugavaro commented Nov 11, 2019 •

edited

Loading

gugavaro Dec 6, 2019 •

edited

Loading