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.

Sign up for GitHub

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

Jump to bottom

[enumification] fix NotificationVisibility enumification. #771

Merged
merged 1 commit into from
Aug 24, 2017
Merged

[enumification] fix NotificationVisibility enumification. #771

merged 1 commit into from
Aug 24, 2017

Conversation

atsushieno
Copy link
Contributor

It was getLockscreenVisibility, not getLockScreenVisibility...

Do not merge it yet, let's see if run-api-compatibility-check actually reports the ABI breakage. It didn't for me...

@jonpryor
Copy link
Member

The tests are being run, but no errors are reported:

make -C external/xamarin-android-api-compatibility check \
                MONO_API_HTML="mono --debug /Users/builder/jenkins/workspace/xamarin-android-pr-builder/xamarin-android/bin/BuildDebug/mono-api-html.exe" \
                MONO_API_INFO="mono --debug /Users/builder/jenkins/workspace/xamarin-android-pr-builder/xamarin-android/bin/BuildDebug/mono-api-info.exe" \
                XA_FRAMEWORK_DIR="/Users/builder/jenkins/workspace/xamarin-android-pr-builder/xamarin-android/bin/Debug/lib/xamarin.android/xbuild-frameworks/MonoAndroid"
...

@jonpryor
Copy link
Member

🤦‍♂️ #facepalm

I manually downloaded oss-xamarin.android_v7.4.99.120_Darwin-x86_64_HEAD_b4b1473d.zip from the build, and the normal mono-api-info + mono-api-html comparison results in:

<h3>Type Changed: Android.App.NotificationChannel</h3>
<p>Modified properties:</p>
<pre>
<div data-is-breaking>	public <span class='removed removed-inline removed-breaking-inline'>int</span> <span class='added '>NotificationVisibility</span> LockscreenVisibility { get; set; }
</div></pre>

Compare to what we're looking for:

	$(MONO_API_HTML) $(REFERENCE_DIR)/$$file.xml temp/$$file.xml --ignore-changes-parameter-names --ignore-nonbreaking | grep '>Removed'

Historically (IIRC), an enumification-related change would result in >Removed and >Added sections. Now it results in a >Modified section.

Our API compatibility check doesn't catch the API breakage.

🤦‍♂️

@atsushieno
Copy link
Contributor Author

That's the exact problem what I was facing when I ran the check.

@jonpryor
Copy link
Member

What should we do to address this? Checking for >Removed is insufficient. Perhaps we should change the grep '>Removed' to:

$(MONO_API_HTML) $(REFERENCE_DIR)/$$file.xml temp/$$file.xml --ignore-changes-parameter-names --ignore-nonbreaking | grep "class='[^']*removed[^']*'"

@atsushieno
Copy link
Contributor Author

I thought we were checking something like data-is-breaking

@jonpryor
Copy link
Member

I thought we were checking something like data-is-breaking

No, but perhaps we should. We could instead use grep '\<data-is-breaking\>'.

@eno: In the interest of expediency, could you please submit a xamarin-android-api-compatibility PR which includes the Android.App.NotificationChannel.NotificationVisibility change along with one of the previously suggested grep changes within xamarin-android-api-compatibility/Makefile?

jonpryor added a commit to xamarin/xamarin-android-api-compatibility that referenced this pull request Aug 23, 2017
@jonpryor
Fix member removal check
e353872 
Context: dotnet/android#771 (comment)

Checking for `>Removed` is insufficient for checking for API
compatibility breakage, as sometimes `mono-api-html` will emit
`>Modified`.

For example, [xamarin-android PR #771][xa771] changes the type of the
`Android.App.NotificationChannel.LockscreenVisibility` property from
`int` to `NotificationVisibility`. This was expected to result in a
build failure, due to API breakage.

[xa771]: dotnet/android#771 (comment)

It didn't.

Update the member removal check to instead check for the existence of
the `data-is-breaking` tag. This will hopefully improve compatibility
checks in the future.
@jonpryor jonpryor mentioned this pull request Aug 23, 2017
Merged
@jonpryor
Copy link
Member

@eno: PR to use data-is-breaking instead of >Removed: xamarin/xamarin-android-api-compatibility#7

@atsushieno atsushieno mentioned this pull request Aug 24, 2017
Merged
@atsushieno atsushieno changed the title [DO NOT MERGE] [enumification] fix NotificationVisibility enumification. [enumification] fix NotificationVisibility enumification. Aug 24, 2017
@jonpryor
Copy link
Member

@eno: Please update this PR to include a bump to xamarin/xamarin-android-api-compatibility@b26743f

@atsushieno
[enumification] fix NotificationVisibility enumification.
38fc674 
It was getLockscreenVisibility, not getLockScreenVisibility...
@jonpryor jonpryor merged commit 420f7e1 into dotnet:master Aug 24, 2017
jonpryor pushed a commit to jonpryor/xamarin-android that referenced this pull request Aug 25, 2017
@atsushieno @jonpryor
[Mono.Android] Fix NotificationVisibility enumification. (dotnet#771)
786b0df 
It was `getLockscreenVisibility`, not `getLockScreenVisibility`...
jonpryor added a commit to xamarin/xamarin-android-api-compatibility that referenced this pull request Feb 22, 2018
@jonpryor
Fix xamarin-android run-api-compatibility-tests
2c689ef 
Context: dotnet/android#1089

What is the purpose of the xamarin-android-api-compatibility repo?
To ensure that we don't accidentally break API, both for the latest
supported API level, and *between* API level binding assemblies.

How's that working out for us?

It could be better.

Commit e353872 was due to a discovery that `mono-api-html` behavior
had changed: it *used* to emit separate "Removed" and "Added"
declarations whenever a method was changed. Then it started emitting
"Modified" sections, but we weren't aware of this change. The result
was an [accidental API break][pr-771], and a change to start looking
for `data-is-breaking` instead of `>Removed`.

[pr-771]: dotnet/android#771 (comment)

*Then* we learned that `make check-inter-api-level` was broken, due to
bad `test` logic. This was fixed in 6dfba92.

The problem is that xamarin-android has not been able to use
[xamarin-android-api-compatibility/master][pr-1078] since 6dfba92 has
been merged, because it found [inter-API level breakage][inter-break]
that we haven't been able to work around, e.g.:

[pr-1078]: dotnet/android#1078
[inter-break]: dotnet/android#1078 (comment)

	<h3>Type Changed: Android.Preferences.CheckBoxPreference</h3>
	Modified base type: <span class='removed removed-inline removed-breaking-inline'>Android.Preferences.Preference</span> <span class='added '>Android.Preferences.TwoStatePreference</span>

The "obvious" solution would have been to use the existing
`inter-api-extra*` files/mechanism to ignore the changes which kept
`make check-inter-api-level` from succeeding, but `mono-api-html`
didn't provide a mechanism to ignore all of those changes.

Doh!

The fix? [Improve `mono-api-html` so it can ignore more][api-ignore]
API artifacts. (Additionally, improve `mono-api-html` so that we can
"scope" what changes we're ignoring, so that e.g. `mono-api-html -r`
can specify the *type* that the ignore should apply to, and not be
matched against *every member in the assembly*.)

[api-ignore]: mono/mono@de4729f

With that infrastructural change in place, update the
`inter-api-extra*` files so that the acceptable changes are ignored,
thus allowing `make check` to run w/o error on xamarin-android/master.

Additionally:

  * The updated `mono-api-html` supports response files. Update the
    `mono-api-html` invocation to provide the `inter-api-extra*` files as
    response files, instead of `cat`ing the `inter-api-extra*` files.
    Eventually, `mono-api-html` may support comments in response files, which
    would allow us to use them.

  * Update `reference/Mono.Android.xml` for `ChoiceMode`.
    Commit 6874e3f updated `Android.Widget.ListView` to use `ChoiceMode`
    instead of `int` for many of the constants. This was inadvertently
    "reverted"/overwrritten in bb8630a.

  * Add support for a new `$(HTML_OUTPUT_DIR)` make variable. If set,
    `mono-api-html`-generated HTML files will be written into the
    `$(HTML_OUTPUT_DIR)` directory.
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request Jan 6, 2021
@jonpryor
Bump to xamarin/Java.Interop/master@fdc200cc
a0ea50d 
Fixes: dotnet/java-interop#767

Changes: dotnet/java-interop@7574f16...fdc200c

  * dotnet/java-interop@fdc200cc: [Xamarin.Android.Tools.Bytecode] Relax _ApiXml check (dotnet#772)
  * dotnet/java-interop@f1b93653: [generator] Change generated code to not emit CA1305 warning. (dotnet#771)
  * dotnet/java-interop@2244407d: [generator] Ensure DIM from assembly refs are correctly marked (dotnet#770)
  * dotnet/java-interop@da73d6a5: [Java.Interop] Prevent premature collection w/ JniInstance* (dotnet#768)

Commit a7413a2 added support for invoking `java-source-utils.jar`
on `@(JavaSourceJar)` to extract Javadoc comments and translate them
into C# XML Documentation Comments.

What this can *also* do is provide correct parameter names.
As of commit a7413a2, the `BindingBuildTest.JavaSourceJar()`
integration test would emit the warning:

	obj/Debug/generated/src/Com.Xamarin.Android.Test.Msbuildtest.JavaSourceJarTest.cs(75,20):
	warning CS1572: XML comment has a param tag for 'name', but there is no parameter by that name

Commit dotnet/java-interop@fdc200cc allows `java-source-utils.jar`
output to be used with `class-parse`, allowing
`@(_JavaSourceJavadocXml)` files -- the output of
`java-source-utils.jar` -- to be included in
`@(_AndroidDocumentationPath)`.

This allows `@(JavaSourceJar)` files to provide parameter names
within bindings, removing the CS1572 warning, and making for
better overall bindings.
@jonpryor jonpryor mentioned this pull request Jan 6, 2021
Merged
jonpryor added a commit to jonpryor/xamarin-android that referenced this pull request Jan 6, 2021
@jonpryor
Bump to xamarin/Java.Interop/master@fdc200cc
01fcfed 
Fixes: dotnet/java-interop#767

Changes: dotnet/java-interop@7574f16...fdc200c

  * dotnet/java-interop@fdc200cc: [Xamarin.Android.Tools.Bytecode] Relax _ApiXml check (dotnet#772)
  * dotnet/java-interop@f1b93653: [generator] Change generated code to not emit CA1305 warning. (dotnet#771)
  * dotnet/java-interop@2244407d: [generator] Ensure DIM from assembly refs are correctly marked (dotnet#770)
  * dotnet/java-interop@da73d6a5: [Java.Interop] Prevent premature collection w/ JniInstance* (dotnet#768)

Commit a7413a2 added support for invoking `java-source-utils.jar`
on `@(JavaSourceJar)` to extract Javadoc comments and translate them
into C# XML Documentation Comments.

What this can *also* do is provide correct parameter names.
As of commit a7413a2, the `BindingBuildTest.JavaSourceJar()`
integration test would emit the warning:

	obj/Debug/generated/src/Com.Xamarin.Android.Test.Msbuildtest.JavaSourceJarTest.cs(75,20):
	warning CS1572: XML comment has a param tag for 'name', but there is no parameter by that name

Commit dotnet/java-interop@fdc200cc allows `java-source-utils.jar`
output to be used with `class-parse`, allowing
`@(_JavaSourceJavadocXml)` files -- the output of
`java-source-utils.jar` -- to be included in
`@(_AndroidDocumentationPath)`.

This allows `@(JavaSourceJar)` files to provide parameter names
within bindings, removing the CS1572 warning, and making for
better overall bindings.
jonpryor added a commit that referenced this pull request Jan 7, 2021
@jonpryor
Bump to xamarin/Java.Interop/master@fdc200cc (#5468)
0e95ec7 
Fixes: dotnet/java-interop#767

Changes: dotnet/java-interop@7574f16...fdc200c

  * dotnet/java-interop@fdc200cc: [Xamarin.Android.Tools.Bytecode] Relax _ApiXml check (#772)
  * dotnet/java-interop@f1b93653: [generator] Change generated code to not emit CA1305 warning. (#771)
  * dotnet/java-interop@2244407d: [generator] Ensure DIM from assembly refs are correctly marked (#770)
  * dotnet/java-interop@da73d6a5: [Java.Interop] Prevent premature collection w/ JniInstance* (#768)

Commit a7413a2 added support for invoking `java-source-utils.jar`
on `@(JavaSourceJar)` to extract Javadoc comments and translate them
into C# XML Documentation Comments.

What this can *also* do is provide correct parameter names.
As of commit a7413a2, the `BindingBuildTest.JavaSourceJar()`
integration test would emit the warning:

	obj/Debug/generated/src/Com.Xamarin.Android.Test.Msbuildtest.JavaSourceJarTest.cs(75,20):
	warning CS1572: XML comment has a param tag for 'name', but there is no parameter by that name

Commit dotnet/java-interop@fdc200cc allows `java-source-utils.jar`
output to be used with `class-parse`, allowing
`@(_JavaSourceJavadocXml)` files -- the output of
`java-source-utils.jar` -- to be included in
`@(_AndroidDocumentationPath)`.

This allows `@(JavaSourceJar)` files to provide parameter names
within bindings, removing the CS1572 warning, and making for
better overall bindings.

We can see the benefits of this change in
`tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/BindingTests.cs`,
which required changes because the parameter names in the Java
`DataListener.onDataReceived()` method could now be determined;
previously they couldn't, resulting in the `P0`/`P1`/etc. names.
With the provision of `@(JavaSourceJar)` -- a7413a2 updated
`Xamarin.Android.McwGen-Tests.csproj` to have `@(JavaSourceJar)` --
the parameter names can now be determined, improving the binding.
@github-actions github-actions bot locked and limited conversation to collaborators Feb 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@atsushieno @jonpryor @dnfclas