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

io.grpc:grpc-api:1.64.2 crashes dotnet build #988

Open
moljac opened this issue Sep 30, 2024 · 3 comments
Open

io.grpc:grpc-api:1.64.2 crashes dotnet build #988

moljac opened this issue Sep 30, 2024 · 3 comments

Comments

@moljac
Copy link
Contributor

moljac commented Sep 30, 2024

Android application type

Android for .NET (net6.0-android, etc.)

Affected platform version

nuget packages AndroidX and GooglePlayServices-Firebase-MLKit

Description

Bindings for io.grpc:grpc-api:1.64.2 crashes dotnet build

/usr/local/share/dotnet/packs/Microsoft.Android.Sdk.Darwin/34.0.113/tools/Xamarin.Android.Bindings.Core.targets(99,5): 
error MSB6006: 
    "dotnet" exited with code 134. 
[./generated/io.grpc.grpc-api/io.grpc.grpc-api.csproj::TargetFramework=net8.0-android]
    17336 Warning(s)
    1 Error(s)

Steps to Reproduce

  1. bump grpc to 1.64.2
  2. run binderator

Did you find any workaround?

No. undergoing investigation.

Relevant log output

Binlog hangs MsBuildLogViewer (Avalonia App) on Mac.

/usr/local/share/dotnet/packs/Microsoft.Android.Sdk.Darwin/34.0.113/tools/Xamarin.Android.Bindings.Core.targets(99,5): 
error MSB6006: 
    "dotnet" exited with code 134. 
[./generated/io.grpc.grpc-api/io.grpc.grpc-api.csproj::TargetFramework=net8.0-android]
    17336 Warning(s)
    1 Error(s)

msbuild.binlog.zip

Online version at https://live.msbuildlog.com/

System.NotSupportedException: Unsupported log file format. Latest supported version is 16, the log file has version 21. at Microsoft.Build.Logging.StructuredLogger.BinLogReader.EnsureFileFormatVersionKnown(Int32 fileFormatVersion) at Microsoft.Build.Logging.StructuredLogger.BinLogReader.Replay(Stream stream, Func`3 progressFunc) at Microsoft.Build.Logging.StructuredLogger.BinaryLog.ReadBuild(Stream stream, Byte[] projectImportsArchive, Func`3 progressFunc) at StructuredLogViewerWASM.Shared.NavMenu.LoadBinlog(MemoryStream memoryStream) at StructuredLogViewerWASM.Shared.NavMenu.ReadFile(IFileListEntry[] files) 
@jpobst
Copy link
Contributor

jpobst commented Oct 1, 2024

This appears to be a stack overflow:

Repeat 13729 times:
--------------------------------
   at generator.SourceWriters.BoundMethod.GetDeclaringTypeOfExplicitInterfaceMethod(MonoDroid.Generation.Method)
--------------------------------
   at generator.SourceWriters.BoundMethod..ctor(MonoDroid.Generation.GenBase, MonoDroid.Generation.Method, MonoDroid.Generation.CodeGenerationOptions, Boolean)
   at generator.SourceWriters.BoundInterface.AddMethods(MonoDroid.Generation.InterfaceGen, MonoDroid.Generation.CodeGenerationOptions)
   at generator.SourceWriters.BoundInterface..ctor(MonoDroid.Generation.InterfaceGen, MonoDroid.Generation.CodeGenerationOptions, MonoDroid.Generation.CodeGeneratorContext, MonoDroid.Generation.GenerationInfo)
   at MonoDroid.Generation.JavaInteropCodeGenerator.WriteType(MonoDroid.Generation.GenBase, System.String, MonoDroid.Generation.GenerationInfo)
   at MonoDroid.Generation.InterfaceGen.Generate(MonoDroid.Generation.CodeGenerationOptions, MonoDroid.Generation.GenerationInfo)
   at Xamarin.Android.Binder.CodeGenerator.Run(Xamarin.Android.Binder.CodeGeneratorOptions, Java.Interop.Tools.Cecil.DirectoryAssemblyResolver)
   at Xamarin.Android.Binder.CodeGenerator.Run(Xamarin.Android.Binder.CodeGeneratorOptions)
   at Xamarin.Android.Binder.CodeGenerator.Main(System.String[])

Working on a fix here: dotnet/java-interop#1261

It may be able to be worked around by using <remove-node> on io.grpc.InternalConfigurator.

@moljac
Copy link
Contributor Author

moljac commented Oct 1, 2024

Wow. Thanks. I didn't want to bother you yet. I tried to get binlog, but that failed with incorrect format (on my Mac), so I went to fix easier fixes 1st.

This appears to be a stack overflow:

Repeat 13729 times:
--------------------------------
   at generator.SourceWriters.BoundMethod.GetDeclaringTypeOfExplicitInterfaceMethod(MonoDroid.Generation.Method)
--------------------------------
   at generator.SourceWriters.BoundMethod..ctor(MonoDroid.Generation.GenBase, MonoDroid.Generation.Method, MonoDroid.Generation.CodeGenerationOptions, Boolean)
   at generator.SourceWriters.BoundInterface.AddMethods(MonoDroid.Generation.InterfaceGen, MonoDroid.Generation.CodeGenerationOptions)
   at generator.SourceWriters.BoundInterface..ctor(MonoDroid.Generation.InterfaceGen, MonoDroid.Generation.CodeGenerationOptions, MonoDroid.Generation.CodeGeneratorContext, MonoDroid.Generation.GenerationInfo)
   at MonoDroid.Generation.JavaInteropCodeGenerator.WriteType(MonoDroid.Generation.GenBase, System.String, MonoDroid.Generation.GenerationInfo)
   at MonoDroid.Generation.InterfaceGen.Generate(MonoDroid.Generation.CodeGenerationOptions, MonoDroid.Generation.GenerationInfo)
   at Xamarin.Android.Binder.CodeGenerator.Run(Xamarin.Android.Binder.CodeGeneratorOptions, Java.Interop.Tools.Cecil.DirectoryAssemblyResolver)
   at Xamarin.Android.Binder.CodeGenerator.Run(Xamarin.Android.Binder.CodeGeneratorOptions)
   at Xamarin.Android.Binder.CodeGenerator.Main(System.String[])

Working on a fix here: dotnet/java-interop#1261

Nice.

It may be able to be worked around by using <remove-node> on io.grpc.InternalConfigurator.

I will try it ASAP, but will bump it with next update.

@moljac
Copy link
Contributor Author

moljac commented Oct 1, 2024

It may be able to be worked around by using on io.grpc.InternalConfigurator.

Nope. Did not help. Moving on to other issues.

jonpryor pushed a commit to dotnet/java-interop that referenced this issue Oct 21, 2024
…#1261

Context: dotnet/android-libraries#988
Context: 9e0a469
Context: 1adb796
Context: #1183
Context: #1269

Imagine you bind the following Java interfaces:

	// Java
	package io.grpc;

	interface Configurator {
	  default void configureChannelBuilder(ManagedChannelBuilder<?> channelBuilder) {}
	}

	public interface InternalConfigurator extends Configurator {
	}

Because the `Configurator` interface is package-protected, its method
is copied into the `public` `InternalConfigurator` interface via
`generator`'s `FixupAccessModifiers()` code.

Later, we look for the "base" method that the copied
`InternalConfigurator.configureChannelBuilder()` overrides, which is
`Configurator.configureChannelBuilder`.  However, because we simply
added the same method instance to two types, this method reference is
actually pointing to itself.  

Eventually we hit a StackOverflowException when trying to retrieve the
[overridden method's declaring type][0].

Fix this by _cloning_ the method when we "copy" it, rather than having
two types reference the same `Method` instance.

…***then*** we get to *testing* this behavior: during code review,
@jonpryor noticed this change:

	diff --git a/tests/generator-Tests/expected.ji/AccessModifiers/Xamarin.Test.IExtendedInterface.cs b/tests/generator-Tests/expected.ji/AccessModifiers/Xamarin.Test.IExtendedInterface.cs
	index 3ea50e6bd..83d01f24a 100644
	--- a/tests/generator-Tests/expected.ji/AccessModifiers/Xamarin.Test.IExtendedInterface.cs
	+++ b/tests/generator-Tests/expected.ji/AccessModifiers/Xamarin.Test.IExtendedInterface.cs
	@@ -46,7 +46,7 @@ public unsafe void BaseMethod ()
	 		{
	 			const string __id = "baseMethod.()V";
	 			try {
	-				_members_xamarin_test_BaseInterface.InstanceMethods.InvokeAbstractVoidMethod (__id, this, null);
	+				_members_xamarin_test_ExtendedInterface.InstanceMethods.InvokeAbstractVoidMethod (__id, this, null);
	 			} finally {
	 			}
	 		}

and went "hmm".

Recall and consider 1adb796: if we invoke the interface method on
the "wrong" declaring type, things break.  This is why 1adb796
updated interface invokers to only invoke methods upon their declared
interfaces.

The concern comes around *non-`public`* interface method invocation:

	/* package */ interface PackagePrivateInterface {
	    void m();
	}
	public interface PublicInterface extends PackagePrivateInterface {
	}

With this commit, attempts to invoke `m()` are made upon
`PublicInterface`, not `PackagePrivateInterface`.

Is this a problem?

Begin partially implementing #1183, adding a new
`build-tools/Java.Interop.Sdk` (not quite a) "project", which has an
`Sdk.props` and `Sdk.targets` file, which combined add support for:

  * A `@(JavaCompile)` build action, for Java source.
  * A `@(JavaReference)` build action, for Java libraries.
  * As a pre-Compile step, files with `%(JavaCompile.Bind)`=True or
    `%(JavaReference.Bind)`=True will be automatically bound, via
    `class-parse` + `generator`.
  * As a post-Compile step, the assembly will be processed with
    `jcw-gen` to generate Java Callable Wrappers, which will be
    compiled into `$(AssemblyName).jar`.

Then, update `tests/Java.Base-Tests` to use this new functionality,
adding a test case to hit the "invoke method declared in a private
interface upon the public interface" scenario.

Result: it works!

Of partial interest is `IInterfaceMethodInheritanceInvoker`:

	internal partial class IInterfaceMethodInheritanceInvoker : global::Java.Lang.Object, IInterfaceMethodInheritance {
	    static readonly JniPeerMembers _members_net_dot_jni_test_BaseInterface = new JniPeerMembers ("net/dot/jni/test/BaseInterface", typeof (IInterfaceMethodInheritanceInvoker));
	    static readonly JniPeerMembers _members_net_dot_jni_test_InterfaceMethodInheritance = new JniPeerMembers ("net/dot/jni/test/InterfaceMethodInheritance", typeof (IInterfaceMethodInheritanceInvoker));
	    static readonly JniPeerMembers _members_net_dot_jni_test_InternalInterface = new JniPeerMembers ("net/dot/jni/test/InternalInterface", typeof (IInterfaceMethodInheritanceInvoker));
	    static readonly JniPeerMembers _members_net_dot_jni_test_PublicInterface = new JniPeerMembers ("net/dot/jni/test/PublicInterface", typeof (IInterfaceMethodInheritanceInvoker));
	}

Of these four fields, two are for internal types:
`_members_net_dot_jni_test_BaseInterface` and
`_members_net_dot_jni_test_InternalInterface`.

Fortunately those types aren't otherwise used.

Of concern, though, is that the constructor invocation *does* result
in a `JNIEnv::FindClass()` invocation, meaning these bindings would
look up (ostensibly) "private" types, which could change!

This presents a compatibility concern: if (when?) those type names
change, then the generated bindings will break.

TODO:

  * #1269: Update `generator` output to *not* emit the
    `static readonly JniPeerMembers` fields for internal types.

Finally, update `jnimarshalmethod-gen` to support resolving types
from the active assembly

For reasons I'm not going to investigate, if an interface type is
defined in the assembly that `jnimarshalmethod-gen` is processing,
`Assembly.Location` will be the empty string, which causes an error:

	% dotnet bin/Release-net8.0/jnimarshalmethod-gen.dll bin/TestRelease-net8.0/Java.Base-Tests.dll -v -v --keeptemp
	…
	error JM4006: jnimarshalmethod-gen: Unable to process assembly 'bin/TestRelease-net8.0/Java.Base-Tests.dll'
	Name can not be empty
	System.ArgumentException: Name can not be empty
	   at Mono.Cecil.AssemblyNameReference.Parse(String fullName)
	   at Java.Interop.Tools.Cecil.DirectoryAssemblyResolver.Resolve(String fullName, ReaderParameters parameters) in /Users/runner/work/1/s/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/DirectoryAssemblyResolver.cs:line 261
	   at Java.Interop.Tools.Cecil.DirectoryAssemblyResolver.Resolve(String fullName) in /Users/runner/work/1/s/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/DirectoryAssemblyResolver.cs:line 256
	   at Java.Interop.Tools.Cecil.DirectoryAssemblyResolver.GetAssembly(String fileName) in /Users/runner/work/1/s/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/DirectoryAssemblyResolver.cs:line 251
	   at Xamarin.Android.Tools.JniMarshalMethodGenerator.Extensions.NeedsMarshalMethod(MethodDefinition md, DirectoryAssemblyResolver resolver, TypeDefinitionCache cache, MethodInfo method, String& name, String& methodName, String& signature) in /Users/runner/work/1/s/tools/jnimarshalmethod-gen/App.cs:line 790
	   at Xamarin.Android.Tools.JniMarshalMethodGenerator.App.CreateMarshalMethodAssembly(String path) in /Users/runner/work/1/s/tools/jnimarshalmethod-gen/App.cs:line 538
	   at Xamarin.Android.Tools.JniMarshalMethodGenerator.App.ProcessAssemblies(List`1 assemblies) in /Users/runner/work/1/s/tools/jnimarshalmethod-gen/App.cs:line 285

Update `App.NeedsMarshalMethod()` so that when the assembly Location
is not present, `DirectoryAssemblyResolver.Resolve(assemblyName)`
is instead used.  This prevents the `ArgumentException`.

[0]: https://github.com/dotnet/java-interop/blob/9d997232f0ce3ca6a5f788b1cdb0fb9b2f978c84/tools/generator/SourceWriters/BoundMethod.cs#L95-L100
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants