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.

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

Bump to Java.Interop/master/1bcfcc5 #1027

Merged
merged 1 commit into from
Nov 15, 2017

Conversation

jonathanpeppers
Copy link
Member

Context: dotnet/java-interop#211

Looking to fix ANDROID_SDK_PATH not set in tests on Windows.

@dnfclas
Copy link

dnfclas commented Nov 14, 2017

@jonathanpeppers,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

jonpryor added a commit to jonpryor/java.interop that referenced this pull request Nov 15, 2017
Context: dotnet/android#1027
Context: https://jenkins.mono-project.com/job/xamarin-android-pr-builder/2048/

Commit 7d51163 changed the visibility of
`JniType.RegisterNativeMethods()` from `public` to `internal`, in an
effort to reduce the likelihood of "user error" from calling
`JNIEnv::RegisterNatives()` multiple times, as
`JNIEnv::RegisterNatives()` invocations are *replacements*, not
"additive"; calling it more than once is *generally* Wrong™.

*Unfortunately*, `JniType.RegisterNativeMethods()` is part of the
public and shipping API, so we can't remove it, as seen in the
[xamarin-android PR build][pr2048] output:

[pr2048]: https://jenkins.mono-project.com/job/xamarin-android-pr-builder/2048/consoleText

	ABI BREAK IN: Java.Interop.dll
	<!-- start namespace Java.Interop --> <div>
	<h2>Namespace Java.Interop</h2>
	<!-- start type JniType --> <div>
	<h3>Type Changed: Java.Interop.JniType</h3>
	<p>Removed method:</p>

	<pre>
	        <span class='removed removed-method breaking' data-is-breaking>public void RegisterNativeMethods (JniNativeMethodRegistration[]);</span>
	</pre>

Revert the visibility of `JniType.RegisterNativeMethods()` to
`public`, so that we don't break API compatibility.
jonpryor added a commit to dotnet/java-interop that referenced this pull request Nov 15, 2017
Context: dotnet/android#1027
Context: https://jenkins.mono-project.com/job/xamarin-android-pr-builder/2048/

Commit 7d51163 changed the visibility of
`JniType.RegisterNativeMethods()` from `public` to `internal`, in an
effort to reduce the likelihood of "user error" from calling
`JNIEnv::RegisterNatives()` multiple times, as
`JNIEnv::RegisterNatives()` invocations are *replacements*, not
"additive"; calling it more than once is *generally* Wrong™.

*Unfortunately*, `JniType.RegisterNativeMethods()` is part of the
public and shipping API, so we can't remove it, as seen in the
[xamarin-android PR build][pr2048] output:

[pr2048]: https://jenkins.mono-project.com/job/xamarin-android-pr-builder/2048/consoleText

	ABI BREAK IN: Java.Interop.dll
	<!-- start namespace Java.Interop --> <div>
	<h2>Namespace Java.Interop</h2>
	<!-- start type JniType --> <div>
	<h3>Type Changed: Java.Interop.JniType</h3>
	<p>Removed method:</p>

	<pre>
	        <span class='removed removed-method breaking' data-is-breaking>public void RegisterNativeMethods (JniNativeMethodRegistration[]);</span>
	</pre>

Revert the visibility of `JniType.RegisterNativeMethods()` to
`public`, so that we don't break API compatibility.
Context: dotnet/java-interop#211

Looking to fix `ANDROID_SDK_PATH` not set in tests on Windows.
@jonathanpeppers jonathanpeppers changed the title Bump to Java.Interop/master/f90f006 Bump to Java.Interop/master/1bcfcc5 Nov 15, 2017
@jonpryor jonpryor merged commit 3750fbf into dotnet:master Nov 15, 2017
@jonathanpeppers jonathanpeppers deleted the bump-java-interop branch November 15, 2017 19:35
@jonpryor jonpryor mentioned this pull request Nov 15, 2017
@github-actions github-actions bot locked and limited conversation to collaborators Feb 2, 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.

3 participants