[Xamarin.Android.Build.Tasks] Generate JNI marshal methods #2153
Conversation
|
build |
1 similar comment
|
build |
| public class JdkInfo : Task | ||
| { | ||
| [Output] | ||
| public string JdkJvmPath { get; set; } |
There was a problem hiding this comment.
The addition of this task seems odd; I think it would make more sense to export this property in the <ResolveSdks/> task, a'la:
// Existing; line 84
JavaSdkPath = MonoAndroidHelper.AndroidSdk.JavaSdkPath;
// New
try {
var info = new JdkInfo (JavaSdkPath);
JdkJvmPath = info.JdkJvmPath;
}
catch (Exception e) {
Log.LogCodedError (...);
}This way we ensure that we're consistent. By using JdkInfo.GetKnownSystemJdkInfos(), it's possible that <ResolveSdks/> will provide one JDK path, but JdkJvmPath could come from a different path. (Likely? I hope not, but possible, which is scary.)
There was a problem hiding this comment.
Makes sense. That or use JavaSdkPath as an input for the task. I will try to extend the ResolveSdks first to see if it doesn't screw up the dependencies.
| } | ||
| } | ||
|
|
||
| static Action<TraceLevel, string> CreateTaskLogger (Task task) |
There was a problem hiding this comment.
I already advocated for the removal of this task, but we already have this method, in Utilities/MSBuildExtensions.cs, as a .CreateTaskLogger() extension method.
| DependsOnTargets="_ResolveAssemblies;_CreatePackageWorkspace;_GenerateJniMarshalMethods;_LinkAssembliesNoShrink;_LinkAssembliesShrink" /> | ||
|
|
||
| <Target Name="_GenerateJniMarshalMethods" | ||
| Condition="'$(AndroidGenerateJniMarshalMethods)' == 'True' And '$(AndroidLinkMode)' != 'None'" |
There was a problem hiding this comment.
Code convention: attributes are indented an extra "tab stop" vs. child elements, thus:
<Target Name="_GenerateJniMarshalMethods"
Condition="'$(AndroidGenerateJniMarshalMethods)' == 'True' And '$(AndroidLinkMode)' != 'None'"
...>
<JdkInfo>
...
| </JdkInfo> | ||
|
|
||
| <Exec | ||
| Command="MONO_PATH=$(_XATargetFrameworkDirectories) $(MonoAndroidBinDirectory)\mono $(MonoAndroidBinDirectory)\..\jnimarshalmethod-gen.exe --jvm=$(_AndroidJvmPath) @(ResolvedUserAssemblies->'$(MonoAndroidLinkerInputDir)%(Filename)%(Extension)', ' ')" |
There was a problem hiding this comment.
I suspect that this should be quoted: MONO_PATH="$(_XATargetFrameworkDirectories)" ....
This should use $(MonoAndroidToolsDirectory) instead of $(MonoAndroidBinDirectory)\...
All parameters must be properly quoted, especially the @(ResolvedUserAssemblies->...) output, as those files are within the project directory, which could contain spaces.
| Condition="'$(AndroidGenerateJniMarshalMethods)' == 'True' And '$(AndroidLinkMode)' != 'None'" | ||
| DependsOnTargets="_GetReferenceAssemblyPaths" | ||
| Inputs="@(ResolvedUserAssemblies->'$(MonoAndroidLinkerInputDir)%(Filename)%(Extension)')" | ||
| Outputs="$(_AndroidJniMarshalMethodsFlag)"> |
There was a problem hiding this comment.
Why do we need a flag file and not the actual output assemblies, a'la _CreateIntermediateAssembliesDir?
There was a problem hiding this comment.
We update the assemblies in place, so we need a flag.
|
The macOS PR Build is failing to build: Er...what? That implies that
Which all seems rather odd; why are we trying to build Why did the dependency fail to build? In short, I'm finding this log file confusing. :-( Worse, it gets more confusing: I guess This PR has nothing to do with |
radekdoulik
force-pushed
the
pr-generate-jni-marshal-methods
branch
2 times, most recently
from
54a59f0 to
3f3e073
Compare
|
build |
radekdoulik
force-pushed
the
pr-generate-jni-marshal-methods
branch
from
ca30a37 to
47eb647
Compare
|
Could the |
|
I hope it is not related. Retrying the build. |
|
Retried build failed in weird way: I guess it might be regression caused by 95ca102 |
radekdoulik
force-pushed
the
pr-generate-jni-marshal-methods
branch
from
ad619d9 to
778d9e4
Compare
| <_AssembliesToProcess Include="@(ResolvedAssemblies)" Condition=" '%(Filename)' != '' And '@(_JniFrameworkAssembly)' != '' " /> | ||
| </ItemGroup> | ||
| <Exec | ||
| Command="MONO_PATH="$(_XATargetFrameworkDirectories)" "$(MonoAndroidBinDirectory)\mono" "$(MonoAndroidToolsDirectory)\jnimarshalmethod-gen.exe" --jvm="$(JdkJvmPath)" @(_AssembliesToProcess->'"$(MonoAndroidLinkerInputDir)%(Filename)%(Extension)"', ' ')" |
There was a problem hiding this comment.
@jonpryor what is our preference there? using Exec or writing a Task to do this job?
If we add a test... we can add a unit test.....
There was a problem hiding this comment.
@dellis1972, I am not sure what do you propose here. To have a task to run mono, similar to Adb task, or a task to generate the marshal methods?
There was a problem hiding this comment.
@radekdoulik that is a good point. How will this work on windows? mono is not installed as part of windows.
There was a problem hiding this comment.
It might be a good idea to use mono --debug here, to get filename & line number information from exception messages.
There was a problem hiding this comment.
@dellis1972: We'll need to update the Xamarin.Android install so that we do install mono on Windows (and macOS, for that matter).
@radekdoulik: Either this PR or another will need to update src/mono-runtimes so that mono.exe for Windows is included into the bundle. A Darwin mono is already being included.
@radekdoulik: This may in turn require more "flexibility": iirc Mono on Windows has both a mono.exe and a monow.exe, which differ only on the Windows Subsystems that they target. Executing mono.exe may result in a "console window" "flashing" on the user's screen, which is really annoying. monow.exe won't cause such flashing, iirc.
Or we might want to use a dedicated Task, as @dellis1972 suggests, either so that we can use monow on Windows, or so that we can use a specific System.Diagnostics.Process which sets ProcessStartInfo.CreateNoWindow=true and ProcessStartInfo.WindowStyle=ProcessWindowStyle.Hidden, as is done elsewhere.
radekdoulik
force-pushed
the
pr-generate-jni-marshal-methods
branch
from
a43f3cc to
596222a
Compare
|
The macOS PR Build failed after 36 minutes, so something horrific broke: |
| <_AssembliesToProcess Include="@(ResolvedAssemblies)" Condition=" '%(Filename)' != '' And '@(_JniFrameworkAssembly)' != '' " /> | ||
| </ItemGroup> | ||
| <Exec | ||
| Command="MONO_PATH="$(_XATargetFrameworkDirectories)" "$(MonoAndroidBinDirectory)\mono" "$(MonoAndroidToolsDirectory)\jnimarshalmethod-gen.exe" --jvm="$(JdkJvmPath)" @(_AssembliesToProcess->'"$(MonoAndroidLinkerInputDir)%(Filename)%(Extension)"', ' ')" |
There was a problem hiding this comment.
It might be a good idea to use mono --debug here, to get filename & line number information from exception messages.
| <_AssembliesToProcess Include="@(ResolvedAssemblies)" Condition=" '%(Filename)' != '' And '@(_JniFrameworkAssembly)' != '' " /> | ||
| </ItemGroup> | ||
| <Exec | ||
| Command="MONO_PATH="$(_XATargetFrameworkDirectories)" "$(MonoAndroidBinDirectory)\mono" "$(MonoAndroidToolsDirectory)\jnimarshalmethod-gen.exe" --jvm="$(JdkJvmPath)" @(_AssembliesToProcess->'"$(MonoAndroidLinkerInputDir)%(Filename)%(Extension)"', ' ')" |
There was a problem hiding this comment.
@dellis1972: We'll need to update the Xamarin.Android install so that we do install mono on Windows (and macOS, for that matter).
@radekdoulik: Either this PR or another will need to update src/mono-runtimes so that mono.exe for Windows is included into the bundle. A Darwin mono is already being included.
@radekdoulik: This may in turn require more "flexibility": iirc Mono on Windows has both a mono.exe and a monow.exe, which differ only on the Windows Subsystems that they target. Executing mono.exe may result in a "console window" "flashing" on the user's screen, which is really annoying. monow.exe won't cause such flashing, iirc.
Or we might want to use a dedicated Task, as @dellis1972 suggests, either so that we can use monow on Windows, or so that we can use a specific System.Diagnostics.Process which sets ProcessStartInfo.CreateNoWindow=true and ProcessStartInfo.WindowStyle=ProcessWindowStyle.Hidden, as is done elsewhere.
Yup, that looks like another System.Reflection behavior change after mono-2018-06 bump. I will need to handle |
|
I am unable to repro even with clean build. So restarted the build to see whether it is a real issue. |
…perty" This reverts commit 47eb647.
…ndroid" This reverts commit 66d34a1.
The new marshaling code uses `JniValueManager`, so we don't want to trow here. Just keep it empty to make things work, until we have proper implementation of it.
Bump Java.Interop to get all the fixes for jnimarshalmethod-gen.exe and value manager marshaling. That fixes all the remaining issues in the XF test with JNI marshal methods enabled. Finally enable the marshaling in the XF test.
As new marshaling is an experimental feature, it is convenient to get more information when something breaks.
Until we have the mono[w].exe in the bundle and are able to run jnimarshalmethod-gen.exe on Windows
That might hopefully fix the type loading problems
JI tests overwrite Java.Runtime.Environment.dll.config. The original updated config file is needed for apk tests as some of them now use jnimarshalmethod-gen.exe tool.
To allow passing of additional arguments to the `jnimarshalmethod-gen.exe` tool. This comes handy when debugging, so that one can pass options like `-v`, `-d` or `--keeptemp` easily.
Fixes dotnet/java-interop#387 The custom value marshaler takes care about marshaling the `Color` as `int` and thus avoiding proxy objects and their marshaling.
Once dotnet/java-interop#389 is merged, it should fix dotnet/java-interop#388 Example of updated marshalers: using Android.Runtime; using Java.Interop; using System; public static void n_setAdapter_Landroid_widget_ListAdapter_ (IntPtr __jnienv, IntPtr __this, IntPtr value) { JniTransition jniTransition = new JniTransition (__jnienv); JniRuntime runtime = default(JniRuntime); try { runtime = JniEnvironment.Runtime; JniRuntime.JniValueManager valueManager = runtime.ValueManager; valueManager.WaitForGCBridgeProcessing (); AbsListView value2 = valueManager.GetValue<AbsListView> (__this); IListAdapter listAdapter2 = value2.Adapter = Java.Interop.JavaConvert.FromJniHandle<IListAdapter> (value, JniHandleOwnership.DoNotTransfer); } catch (Exception ex) when (runtime.ExceptionShouldTransitionToJni (ex)) { jniTransition.SetPendingException (ex); } finally { jniTransition.Dispose (); } } and using Android.Runtime; using Java.Interop; using System; public static IntPtr n_getAdapter (IntPtr __jnienv, IntPtr __this) { JniTransition jniTransition = new JniTransition (__jnienv); JniRuntime runtime = default(JniRuntime); try { runtime = JniEnvironment.Runtime; JniRuntime.JniValueManager valueManager = runtime.ValueManager; valueManager.WaitForGCBridgeProcessing (); AbsListView value = valueManager.GetValue<AbsListView> (__this); IListAdapter adapter = value.Adapter; return JNIEnv.ToLocalJniHandle (adapter); } catch (Exception ex) when (runtime.ExceptionShouldTransitionToJni (ex)) { jniTransition.SetPendingException (ex); return default(IntPtr); } finally { jniTransition.Dispose (); } IntPtr intPtr = default(IntPtr); return intPtr; }
... when building the tests, which use jnimarshalmethod-gen.exe
radekdoulik
force-pushed
the
pr-generate-jni-marshal-methods
branch
from
9fe8958 to
f5ff71c
Compare
| Log.LogCodedError ("XA5300", $"Unable to find {nameof (JdkJvmPath)}{Environment.NewLine}{e}"); | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
@radekdoulik quick question about this task. There is a Visual Studio team very focused on the performance of "solution create", which is basically the initial design-time build from our targets.
Since looking up JdkJvmPath takes extra time, could it:
- Not run on
DesignTimeBuild=True? - Not run unless
AndroidGenerateJniMarshalMethods=True? - It also might be worth splitting this out into another MSBuild task?
There was a problem hiding this comment.
It is now only used by jnimarshalmethod-gen, so we can look it up only when AndroidGenerateJniMarshalMethods is true
There was a problem hiding this comment.
Ok, cool. I might look at making those changes ^^ I'll have you review.
Might not happen this week, though. 😃
4 participants
Implementation of #2138
Also fixes dotnet/java-interop#387,
dotnet/java-interop#388
Conditionally generate JNI marshal methods for user and relevant SDK
assemblies. It is enabled by setting
AndroidGenerateJniMarshalMethodsto True.This is done by
_GenerateJniMarshalMethodstarget, which runsjnimarshalmethod-gen.exe with XA's mono. It is possible to pass
additional arguments to the tool by setting
AndroidGenerateJniMarshalMethodsAdditionalArgumentsproperty.The target also passes the path to the JVM shared library to the tool
using
--jvmoption. The path comes from the updatedResolveSdkstask. The task will be optimized to improve performance by caching the
path.
The commit also removes the
NotImplementedExceptionthrows from theAndroid.Runtime.AndroidValueManager. The reason for that is thatthis is now used from the generated marshal methods, which handle
primitive arrays. Namely
AddPeer (IJavaPeerable value)is used. Thatis something to further examine.
The
RunJavaInteropTestswas moved from_RunParallelTestTargetitemgroup to
_RunTestTargetgroup, because it overwrites theJava.Runtime.Environment.dll.config, which is required byjnimarshalmethod-gen.exeto run. And so we need to run these tests afterapk tests. That should be fixed in future, hopefully by adding and/or
fixing the Inputs/Outputs of the relevant targets.
Enable JNI marshal methods in Mono.Android runtime test and
Xamarin.Forms test in Release and Release/AOT configurations.
Add custom
JniCustomValueMarshaler's forAndroid.Graphics.Colorand for
IJavaObjectbased objects, which don't implementIJavaPeerable.The generated
Colorclass marshaler example:The generated
IJavaObjectmarshaler examples:and
The Java.Interop submodule is bumped to have all the relevant fixes.
Profiling results of Xamarn.Forms test running on Pixel 2 XL phone:
Old marshaling
New marshaling
The native members registration is about 2 times faster and also the
JNIEnvis about 20ms faster.