Skip to content

Conversation

@radekdoulik
Copy link
Member

…blies

Use jnimarshalmethod-gen on resolved user assemblies. As a preview
feature it is conditional and only enabled when JniMarshalMethods is
True.

This feature implements main part of
https://github.com/xamarin/xamarin-android/projects/1

…blies

Use `jnimarshalmethod-gen` on resolved user assemblies. As a preview
feature it is conditional and only enabled when `JniMarshalMethods` is
True.

This feature implements main part of
https://github.com/xamarin/xamarin-android/projects/1
@radekdoulik radekdoulik added the do-not-merge PR should not be merged. label May 30, 2018
@radekdoulik radekdoulik requested a review from jonpryor May 30, 2018 10:39
@radekdoulik radekdoulik requested a review from dellis1972 as a code owner May 30, 2018 10:39
@radekdoulik
Copy link
Member Author

It requires these PRs to be merged first: #1739 and dotnet/java-interop#321

</Target>

<Target Name="_GenerateJniMarshalMethods"
Condition="'$(JniMarshalMethods)' == 'True' And '$(Configuration)' == 'Release' And '$(AndroidLinkMode)' != 'None'"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, $(JniMarshalMethods) does appear to control generation as well, wrt #1745 (comment).

Thus, $(AndroidGenerateJniMarshalMethods) seems appropriate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Convention (also not followed consistently, but let's not make it worse):

For XML, attributes are indented one "tab stop" more than child elements. Additionally, prefer spaces over tabs, and use two-space "tab stops". Thus:

<Target Name="_GenerateJniMarshalMethods"
    Condition=" '$(AndroidGenerateJniMarshalMethods)' == 'True' And '$(Configuration)' == 'Release' And '$(AndroidLinkMode)' != 'None' "
    DependsOnTargets="_GetReferenceAssemblyPaths"
    Inputs="@(ResolvedUserAssemblies->'$(MonoAndroidLinkerInputDir)%(Filename)%(Extension)')"
    Outputs="$(_AndroidJniMarshalMethodsFlag)">
  <Exec
      Command="MONO_PATH=$(_XATargetFrameworkDirectories) $(MonoAndroidBinDirectory)\mono $(MonoAndroidBinDirectory)\..\jnimarshalmethod-gen.exe @(ResolvedUserAssemblies->'$(MonoAndroidLinkerInputDir)%(Filename)%(Extension)')"
  />
</Target>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not use '$(Configuration)' == 'Release'. $(Configuration) values are convention, and developers are able to add arbitrary names. (For example, Java.Interop had a XAIntegrationRelease configuration for quite some time.)

Any check against $(Configuration) is a "code smell." (I think we still have one anyway -- though I can't currently find it -- but, again, we shouldn't make things worse.)

Outputs="$(_AndroidJniMarshalMethodsFlag)">

<Exec
Command="MONO_PATH=$(_XATargetFrameworkDirectories) $(MonoAndroidBinDirectory)\mono $(MonoAndroidBinDirectory)\..\jnimarshalmethod-gen.exe @(ResolvedUserAssemblies->'$(MonoAndroidLinkerInputDir)%(Filename)%(Extension)')"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we also need to update the GetMonoBundleItems target within src/mono-runtimes/mono-runtimes.targets so that mono is included in the bundle*.zip file?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$(_XATargetFrameworkDirectories) should be quoted, in case it contains spaces:

Command="MONO_PATH=&quot;$(_XATargetFrameworkDirectories)&quot; ...

Additionally, I believe that $(_XATargetFrameworkDirectories) will be incomplete: it should contain only the directories (directory) for MonoAndroid,Version=v1.0, which will not be a directory which contains Mono.Android.dll.

For example (from a random full xamarin-android build log I have lying around):

Task "ResolveSdks" (TaskId:1387)
...
  Task Parameter:ReferenceAssemblyPaths=/Users/builder/jenkins/workspace/xamarin-android-pr-builder/xamarin-android/bin/Debug/lib/xamarin.android/xbuild-frameworks/MonoAndroid/v1.0/ (TaskId:1387)
...
  Output Property: _XATargetFrameworkDirectories=/Users/builder/jenkins/workspace/xamarin-android-pr-builder/xamarin-android/bin/Debug/lib/xamarin.android/xbuild-frameworks/MonoAndroid/v1.0/ (TaskId:1387)

If e.g. .../xbuild-frameworks/MonoAndroid/v8.1 isn't included, how will Mono.Android.dll be found?

Alternatively, perhaps MONO_PATH should be set to $(MonoAndroidLinkerInputDir) and called after that directory has been setup, e.g. after _CopyIntermediateAssemblies?

@jonpryor
Copy link
Contributor

This PR should also pick a project to have jnimarshalmethod-gen.exe use enabled on, e.g. tests/Xamarin.Forms-Performance-Integration, so that we can the entire process is actually used ("tested") and so we can see the impact of the change on our startup graphs.

@radekdoulik
Copy link
Member Author

Superseded by #2153

@radekdoulik radekdoulik closed this Nov 6, 2018
@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

do-not-merge PR should not be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants