Skip to content
This repository was archived by the owner on Mar 8, 2021. It is now read-only.

Commit a773887

Browse files
committed
Improve the data-is-breaking check
Context: https://github.com/mono/mono/blob/449e36638fc0b7188b2e9fbd9734a5b263ba9d22/mcs/tools/mono-api-html/HtmlFormatter.cs Context: https://jenkins.mono-project.com/view/Xamarin.Android/job/xamarin-android-pr-builder-release/143/ Context: mono/mono@c32af89 Every `xamarin-android` PR since [the mono/2018-08 bump][0] has been **UNSTABLE** because of [API Compatibility Check failures][1]. The "funny" thing is that *there are no API breakages reported*: The assembly being reported as breaking API -- `Mono.Android.Export.dll` -- contains *no* public API (!), and thus cannot possibly break it. Consequently, the [reported API breakage][2] doesn't show anything. This was quite confusing. The "cause" of the API breakage reports was the bump to mono/2018-08, which causes xamarin-android to use a new(er) `mono-api-html` for `mono-api-info` XML comparison purposes. The subsequent "cause" is that the "template HTML" which `mono-api-html` produces is now always generated when processing `Mono.Android.Export.xml`, seemingly because `Mono.Android.Export.xml` contains no types at all: $ mono ../../bin/BuildDebug/mono-api-html.exe --ignore-changes-parameter-names --ignore-nonbreaking reference/Mono.Android.xml temp/Mono.Android.xml # no output $ mono ../../bin/BuildDebug/mono-api-html.exe --ignore-changes-parameter-names --ignore-nonbreaking reference/Mono.Android.Export.xml temp/Mono.Android.Export.xml # LOTS of output, including lines like var any_breaking = element.hasAttribute ('data-is-breaking'); This change was introduced by commit mono/mono@c32af890. How do we fix this? A manual review of all locations that `data-is-breaking` can be generated within `HtmlFormatter.cs` shows that it will *always* be emitted as the last attribute within an element, for example: <span class='removed removed-method breaking' data-is-breaking>public static void GlClearBufferfi (int, int, float, int);</span> The simplest fix is to alter the **grep**(1) invocation to check for `\<data-is-breaking>`, *requiring* the final `>` be matched. That's the good news. The bad news is that I've lost faith in using `mono-api-html.exe` as a mechanism for ensuring that we don't break API. Events such as this show that it's *incredibly* brittle -- see also 2c689ef -- and, worse, *it **still** doesn't work properly*. Case in point: what happens if we *don't* ignore changes in parameter names and ignore non-breaking changes in `Mono.Android.xml`? What else appears in the output? <h3>Type Changed: Android.Media.MediaScannerConnection.IMediaScannerConnectionClient</h3> <div> <p>Added interface:</p> <pre> <span class='added added-interface breaking' data-is-breaking>MediaScannerConnection.IOnScanCompletedListener</span> </pre> </div> ***WHAT?!*** The ***entire reason*** to use `mono-api-html.exe` -- to at least *know* when we break API -- does not catch the above breakage. Altering an interface to implement a new interface is at least a *possible* breaking change, and does *not* involve parameter names, yet *it wasn't shown*. I am not happy to discover this. This won't be fixed *now*, but a possible idea for future implementation is to instead use [`Microsoft.DotNet.GenAPI.exe`][3] from the [mono/api-snapshot][4] module, which is a tool to produce C# source code for "reference assemblies", which can be compared using **diff**(1) to see if *anything* changed. [0]: dotnet/android@3897e86 [1]: https://jenkins.mono-project.com/view/Xamarin.Android/job/xamarin-android-pr-builder-release/143/API_20Compatibility_20Checks/ [2]: https://jenkins.mono-project.com/view/Xamarin.Android/job/xamarin-android-pr-builder-release/143/API_20Compatibility_20Checks/Mono.Android.Export.html [3]: https://github.com/mono/api-snapshot/tree/master/tools/genapi [4]: https://github.com/mono/api-snapshot/
1 parent 73d45d0 commit a773887

File tree

1 file changed

+2
-2
lines changed

1 file changed

+2
-2
lines changed

Makefile

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ check: check-inter-api-level
9494
$(call BUILD_API_INFO,temp,$(XA_FRAMEWORK_DIR))
9595
failed=0 ; \
9696
for file in $(CORE_ASSEMBLIES) $(TFV_ASSEMBLIES) ; do \
97-
if $(MONO_API_HTML) $(REFERENCE_DIR)/$$file.xml temp/$$file.xml --ignore-changes-parameter-names --ignore-nonbreaking | grep '\<data-is-breaking\>' > /dev/null 2>&1 ; then \
97+
if $(MONO_API_HTML) $(REFERENCE_DIR)/$$file.xml temp/$$file.xml --ignore-changes-parameter-names --ignore-nonbreaking | grep '\<data-is-breaking>' > /dev/null 2>&1 ; then \
9898
echo "ABI BREAK IN: $$file.dll" ; \
9999
$(MONO_API_HTML) $(REFERENCE_DIR)/$$file.xml temp/$$file.xml --ignore-changes-parameter-names --ignore-nonbreaking \
100100
$(if $(HTML_OUTPUT_DIR),$(HTML_OUTPUT_DIR)/$$file.html); \
@@ -136,7 +136,7 @@ check-inter-api-level: -create-inter-api-infos
136136
command="$(MONO_API_HTML) \"$$prev\" \"$$cur\" --ignore-changes-parameter-names --ignore-changes-virtual --ignore-changes-property-setters --ignore-nonbreaking $$extra"; \
137137
echo $$command; \
138138
eval $$command > "$$out" 2>&1; \
139-
if grep '\<data-is-breaking\>' $$out > /dev/null 2>&1 ; then \
139+
if grep '\<data-is-breaking>' $$out > /dev/null 2>&1 ; then \
140140
echo "<h1>### API BREAK BETWEEN $${_frameworks[$$prev_framework]} and $${_frameworks[$$i]}</h1>" ; \
141141
cat $$out; \
142142
if [ -n "$(HTML_OUTPUT_DIR)" ]; then \

0 commit comments

Comments
 (0)