[jnimarshalmethod-gen] fixes for interfaces #472
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When bumping xamarin-android to java.interop/master, we are hitting a
failure:
Instance property 'PeerReference' is not defined for type 'Android.Widget.IListAdapter'
Parameter name: propertyName
System.ArgumentException: Instance property 'PeerReference' is not defined for type 'Android.Widget.IListAdapter'
Parameter name: propertyName
at System.Linq.Expressions.Expression.Property (System.Linq.Expressions.Expression expression, System.String propertyName) [0x00051] in <9536922a6deb4f2dbdd3b7dedc620be3>:0
at Java.Interop.JavaPeerableValueMarshaler.CreateIntermediaryExpressionFromManagedExpression (Java.Interop.Expressions.JniValueMarshalerContext context, System.Linq.Expressions.ParameterExpression sourceValue) [0x0002e] in external/Java.Interop/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs:620
at Java.Interop.JavaPeerableValueMarshaler.CreateReturnValueFromManagedExpression (Java.Interop.Expressions.JniValueMarshalerContext context, System.Linq.Expressions.ParameterExpression sourceValue) [0x00001] in external/Java.Interop/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs:631
at Java.Interop.MarshalMemberBuilder.CreateMarshalToManagedExpression (System.Reflection.MethodInfo method, Java.Interop.JavaCallableAttribute callable, System.Type type) [0x0042d] in external/Java.Interop/src/Java.Interop.Export/Java.Interop/MarshalMemberBuilder.cs:207
at Java.Interop.MarshalMemberBuilder.CreateMarshalToManagedExpression (System.Reflection.MethodInfo method) [0x00017] in external/Java.Interop/src/Java.Interop.Export/Java.Interop/MarshalMemberBuilder.cs:32
at Xamarin.Android.Tools.JniMarshalMethodGenerator.App.CreateMarshalMethodAssembly (System.String path) [0x003fc] in external/Java.Interop/tools/jnimarshalmethod-gen/App.cs:393
at Xamarin.Android.Tools.JniMarshalMethodGenerator.App.ProcessAssemblies (System.Collections.Generic.List`1[T] assemblies) [0x00150] in external/Java.Interop/tools/jnimarshalmethod-gen/App.cs:186
Since ef092a8 introduced some changes to interfaces, there are a
couple spots in `jnimarshalmethod-gen.exe` that need a fix.
First, there was an additional cast needed where an interface might
extend `IJavaPeerable`:
interface IListAdapter : IJavaPeerable { }
After this, we got a new error:
System.ArgumentException: 'this' type cannot be an interface itself
at System.RuntimeType.GetInterfaceMap (System.Type ifaceType) [0x00069] in <2d5573434b724f28896144eecd6f4489>:0
at Xamarin.Android.Tools.JniMarshalMethodGenerator.Extensions.NeedsMarshalMethod (Mono.Cecil.MethodDefinition md, Java.Interop.Tools.Cecil.DirectoryAssemblyResolver resolver, System.Reflection.MethodInfo method, System.String& name, System.String& methodName, System.String& signature) [0x0006b] in external/Java.Interop/tools/jnimarshalmethod-gen/App.cs:642
at Xamarin.Android.Tools.JniMarshalMethodGenerator.App.CreateMarshalMethodAssembly (System.String path) [0x00370] in external/Java.Interop/tools/jnimarshalmethod-gen/App.cs:374
at Xamarin.Android.Tools.JniMarshalMethodGenerator.App.ProcessAssemblies (System.Collections.Generic.List`1[T] assemblies) [0x00150] in external/Java.Interop/tools/jnimarshalmethod-gen/App.cs:186
For this one, we can actually skip processing of interfaces -- we
don't need to generate marshal methods for interfaces at all.
I also updated `ExportTest.cs` so it included a problematic interface.
Before these fixes, I could repro the same problems with `make
run-test-jnimarshal`.
radekdoulik
reviewed
Aug 16, 2019
Member
radekdoulik
left a comment
radekdoulik
left a comment
There was a problem hiding this comment.
The src/Java.Interop.Export/Tests/Java.Interop/MarshalMemberBuilderTest.cs test needs to be updated with the new cast:
- Failed : Java.InteropTests.MarshalMemberBuilderTest.CreateMarshalFromJniMethodExpression_FuncIJavaObject
Expected string length 834 but was 849. Strings differ at index 554.
Expected: "...se\n\t\t{\n\t\t\treturn __mret_ref = __mret.PeerReference;\n\t\t}\n\t\t..."
But was: "...se\n\t\t{\n\t\t\treturn __mret_ref = (IJavaPeerable)__mret.PeerRe..."
---------------------------------------------------^
at Java.InteropTests.MarshalMemberBuilderTest.CheckExpression (System.Linq.Expressions.LambdaExpression expression, System.String memberName, System.Type expressionType, System.String expectedBody) [0x00000] in :0
🤦♂ there is no space
Member
Author
|
When we merge this, we'll need it on |
Member
|
@monojenkins backport mono-2019-08 |
radekdoulik
pushed a commit
that referenced
this pull request
Aug 16, 2019
When bumping xamarin-android to java.interop/master, we are hitting a failure: Instance property 'PeerReference' is not defined for type 'Android.Widget.IListAdapter' Parameter name: propertyName System.ArgumentException: Instance property 'PeerReference' is not defined for type 'Android.Widget.IListAdapter' Parameter name: propertyName at System.Linq.Expressions.Expression.Property (System.Linq.Expressions.Expression expression, System.String propertyName) at Java.Interop.JavaPeerableValueMarshaler.CreateIntermediaryExpressionFromManagedExpression (Java.Interop.Expressions.JniValueMarshalerContext context, System.Linq.Expressions.ParameterExpression sourceValue) at Java.Interop.JavaPeerableValueMarshaler.CreateReturnValueFromManagedExpression (Java.Interop.Expressions.JniValueMarshalerContext context, System.Linq.Expressions.ParameterExpression sourceValue) at Java.Interop.MarshalMemberBuilder.CreateMarshalToManagedExpression (System.Reflection.MethodInfo method, Java.Interop.JavaCallableAttribute callable, System.Type type) at Java.Interop.MarshalMemberBuilder.CreateMarshalToManagedExpression (System.Reflection.MethodInfo method) at Xamarin.Android.Tools.JniMarshalMethodGenerator.App.CreateMarshalMethodAssembly (System.String path) at Xamarin.Android.Tools.JniMarshalMethodGenerator.App.ProcessAssemblies (System.Collections.Generic.List`1[T] assemblies) Since ef092a8 introduced some changes to interfaces, there are a couple spots in `jnimarshalmethod-gen.exe` that need a fix. First, there was an additional cast needed where an interface might extend `IJavaPeerable`: interface IListAdapter : IJavaPeerable { } Update `JavaPeerableValueMarshaler.CreateIntermediaryExpressionFromManagedExpression()` to insert an intermediate cast to `IJavaPeerable` to fix the `ArgumentException`. After inserting the cast, we got a new error: System.ArgumentException: 'this' type cannot be an interface itself at System.RuntimeType.GetInterfaceMap (System.Type ifaceType) [0x00069] in <2d5573434b724f28896144eecd6f4489>:0 at Xamarin.Android.Tools.JniMarshalMethodGenerator.Extensions.NeedsMarshalMethod (Mono.Cecil.MethodDefinition md, Java.Interop.Tools.Cecil.DirectoryAssemblyResolver resolver, System.Reflection.MethodInfo method, System.String& name, System.String& methodName, System.String& signature) [0x0006b] in external/Java.Interop/tools/jnimarshalmethod-gen/App.cs:642 at Xamarin.Android.Tools.JniMarshalMethodGenerator.App.CreateMarshalMethodAssembly (System.String path) [0x00370] in external/Java.Interop/tools/jnimarshalmethod-gen/App.cs:374 at Xamarin.Android.Tools.JniMarshalMethodGenerator.App.ProcessAssemblies (System.Collections.Generic.List`1[T] assemblies) [0x00150] in external/Java.Interop/tools/jnimarshalmethod-gen/App.cs:186 For this one, we can actually skip processing of interfaces -- we don't need to generate marshal methods for interfaces at all. I also updated `ExportTest.cs` so it included a problematic interface, which triggers both of the above error conditions.
jonpryor
added a commit
to jonpryor/java.interop
that referenced
this pull request
Aug 16, 2019
Commit ffbb2dc introduced a unit test failure, as it overlooked updating an "expected" string within the test `Java.InteropTests.JniValueMarshaler_IJavaPeerable_ContractTests.CreateReturnValueFromManagedExpression()`. The more important question is this: why was that test failure missed? Turns Out™ that our unit test infrastructure is "dodgy": it was possible for tests to fail while the overall `make run-all-tests` target reported success! For example, as of commit ffbb2dc we *know* that `Java.Interop-Tests.dll` will report a test failure. Yet, if we run `make run-tests` such that a different (successful) unit test assembly follows it, no error is reported: $ make run-tests TESTS="bin/TestDebug/Java.Interop-Tests.dll bin/TestDebug/Java.Interop.Export-Tests.dll" ... …/Java.Interop/build-tools/scripts/RunNUnitTests.targets(35,5): error MSB3073: The command "mono --debug packages/NUnit.ConsoleRunner.3.9.0/tools/nunit3-console.exe bin/TestDebug/Java.Interop-Tests.dll --result="TestResult-Java.Interop-Tests.xml;format=nunit2" --output="bin/TestDebug/TestOutput-Java.Interop-Tests.txt"" exited with code 1. 0 Warning(s) 1 Error(s) ... $ echo $? 0 *This* is why the [DevOps build for PR dotnet#472][0] was green and the PR was merged. It's also why one of the [subsequent master builds][1] was green, even though it [reported unit test failures][2]. Our tests are unreliable. What will we do about it? 1. Update `make run-all-tests` to explicitly invoke targets. 2. Update the other `run-*` targets to exit with a non-zero value if *any* command exits with a non-zero value. `make run-all-tests` cannot use prerequisites, because once a prerequisites fails, all following prerequisites are skipped, and we want *all* of the unit tests to run, so that we collect as many errors as possible from a test run. As such, this rule: run-all-tests: run-tests run-ptests must be converted into: run-all-tests: r=0; \ $(MAKE) run-tests || r=1; \ $(MAKE) run-ptests || r=1; \ exit $$r The other `run-*` targets must be fixed in a similar way, to ensure that if *any* command fails, the entire rule *also* fails, while also executing as many tests as possible. With these changes in place, `make run-all-tests` will now report an error if any of the executed tests fail: $ make run-tests TESTS="bin/TestDebug/Java.Interop-Tests.dll bin/TestDebug/Java.Interop.Export-Tests.dll" ... $ echo $? 2 Finally, fix the error reported from `Java.InteropTests.JniValueMarshaler_IJavaPeerable_ContractTests`. [0]: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=2950259&view=results [1]: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=2950430&view=results [2]: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=2950430&view=ms.vss-test-web.build-test-results-tab
jonpryor
added a commit
that referenced
this pull request
Aug 16, 2019
Commit ffbb2dc introduced a unit test failure, as it overlooked updating an "expected" string within the test `Java.InteropTests.JniValueMarshaler_IJavaPeerable_ContractTests.CreateReturnValueFromManagedExpression()`. The more important question is this: why was that test failure missed? Turns Out™ that our unit test infrastructure is "dodgy": it was possible for tests to fail while the overall `make run-all-tests` target reported success! For example, as of commit ffbb2dc we *know* that `Java.Interop-Tests.dll` will report a test failure. Yet, if we run `make run-tests` such that a different (successful) unit test assembly follows it, no error is reported: $ make run-tests TESTS="bin/TestDebug/Java.Interop-Tests.dll bin/TestDebug/Java.Interop.Export-Tests.dll" ... …/Java.Interop/build-tools/scripts/RunNUnitTests.targets(35,5): error MSB3073: The command "mono --debug packages/NUnit.ConsoleRunner.3.9.0/tools/nunit3-console.exe bin/TestDebug/Java.Interop-Tests.dll --result="TestResult-Java.Interop-Tests.xml;format=nunit2" --output="bin/TestDebug/TestOutput-Java.Interop-Tests.txt"" exited with code 1. 0 Warning(s) 1 Error(s) ... $ echo $? 0 *This* is why the [DevOps build for PR #472][0] was green and the PR was merged. It's also why one of the [subsequent master builds][1] was green, even though it [reported unit test failures][2]. Our tests are unreliable. What will we do about it? 1. Update `make run-all-tests` to explicitly invoke targets. 2. Update the other `run-*` targets to exit with a non-zero value if *any* command exits with a non-zero value. `make run-all-tests` cannot use prerequisites, because once a prerequisites fails, all following prerequisites are skipped, and we want *all* of the unit tests to run, so that we collect as many errors as possible from a test run. As such, this rule: run-all-tests: run-tests run-ptests must be converted into: run-all-tests: r=0; \ $(MAKE) run-tests || r=1; \ $(MAKE) run-ptests || r=1; \ exit $$r The other `run-*` targets must be fixed in a similar way, to ensure that if *any* command fails, the entire rule *also* fails, while also executing as many tests as possible. With these changes in place, `make run-all-tests` will now report an error if any of the executed tests fail: $ make run-tests TESTS="bin/TestDebug/Java.Interop-Tests.dll bin/TestDebug/Java.Interop.Export-Tests.dll" ... $ echo $? 2 Finally, fix the error reported from `Java.InteropTests.JniValueMarshaler_IJavaPeerable_ContractTests`. [0]: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=2950259&view=results [1]: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=2950430&view=results [2]: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=2950430&view=ms.vss-test-web.build-test-results-tab
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When bumping xamarin-android to java.interop/master, we are hitting a
failure:
Since ef092a8 introduced some changes to interfaces, there are a
couple spots in
jnimarshalmethod-gen.exethat need a fix.First, there was an additional cast needed where an interface might
extend
IJavaPeerable:After this, we got a new error:
For this one, we can actually skip processing of interfaces -- we
don't need to generate marshal methods for interfaces at all.
I also updated
ExportTest.csso it included a problematic interface.Before these fixes, I could repro the same problems with
make run-test-jnimarshal.