Skip to content

[jcw-gen] Use + for nested types, not / #1304

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

Merged
merged 4 commits into from
Feb 6, 2025

Conversation

jonpryor
Copy link
Contributor

@jonpryor jonpryor commented Feb 6, 2025

TODO: full explanation.

#1302 and dotnet/android#9750 has turned into a bit of a boondoggle. Maybe the better approach is to just update jcw-gen to use + instead of / for nested types, instead of both jcw-gen and generator, because the generator changes just require changes everywhere.

Will this smaller fix work?

TODO: full explanation.

#1302 and dotnet/android#9750 has turned into a
bit of a boondoggle.  Maybe the better approach is to just update
`jcw-gen` to use `+` instead of `/` for nested types, instead of
*both* `jcw-gen` *and* `generator`, because the `generator` changes
just require changes *everywhere*.

Will this smaller fix work?
jonpryor added a commit to dotnet/android that referenced this pull request Feb 6, 2025
Does It Build™?
@jonpryor
Copy link
Contributor Author

jonpryor commented Feb 6, 2025

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jonpryor
Copy link
Contributor Author

jonpryor commented Feb 6, 2025

Draft commit message:

[jcw-gen] Use `+` for nested types, not `/`

Context: https://github.com/dotnet/android/pull/9747
Context: https://discord.com/channels/732297728826277939/732297837953679412/1336353039031734352
Context: https://discord.com/channels/732297728826277939/732297837953679412/1336358257769316372
Context: https://github.com/dotnet/java-interop/pull/1302
Context: https://github.com/dotnet/android/pull/9750

The `[Register]` attribute provides "connector method" names, and for
interface methods this will also include the name of the type which
declares the method, which itself may be in a nested type:

	namespace Android.App {
	  public partial class Application {
	    public partial interface IActivityLifecycleCallbacks : IJavaObject, IJavaPeerable {
	      [Register (
	          name: "onActivityCreated",
	          signature: "(Landroid/app/Activity;Landroid/os/Bundle;)V",
	          connector: "GetOnActivityCreated_Landroid_app_Activity_Landroid_os_Bundle_Handler:Android.App.Application/IActivityLifecycleCallbacksInvoker, Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null")]
	      void OnActivityCreated (Android.App.Activity activity, Android.OS.Bundle? savedInstanceState);

	      // …
	    }
	  }
	}

The `connector` value is copied as-is into Java Callable Wrappers,
as part of the `__md_methods` value and `Runtime.register()` call.
Given the C# type:

	partial class MauiApplication : Application {
	  partial class ActivityLifecycleCallbacks : Java.Lang.Object, Application.IActivityLifecycleCallbacks {
	    public void OnActivityCreated (Activity activity, Bundle? savedInstanceState) => …
	  }
	}

then `Java.Interop.Tools.JavaCallableWrappers` will produce:

	// Java Callable Wrapper
	/* partial */ class MauiApplication_ActivityLifecycleCallbacks
	{
	  public static final String __md_methods;
	  static {
	    __md_methods =
	      // …
	      "n_onActivityCreated:(Landroid/app/Activity;Landroid/os/Bundle;)V:GetOnActivityCreated_Landroid_app_Activity_Landroid_os_Bundle_Handler:Android.App.Application/IActivityLifecycleCallbacksInvoker, Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null\n" +
	      // …
	      "";
	    mono.android.Runtime.register ("Microsoft.Maui.MauiApplication+ActivityLifecycleCallbacks, Microsoft.Maui", MauiApplication_ActivityLifecycleCallbacks.class, __md_methods);
	  }
	}

The `signature` and `connector` values from the `[Register(…)]` on
the method declaration are copied as-is into `__md_methods`.
As the `connector` value contains a `/`, `__md_methods` does as well.

This has worked fine for nearly 15+ years…on Mono/MonoVM.

This *fails* on NativeAOT and CoreCLR, as:

	Type.GetType ("Android.App.Application/IActivityLifecycleCallbacksInvoker, Mono.Android, …", throwOnError:true)

fails with:

	TypeLoadException: Could not resolve type 'Android.App.Application/IActivityLifecycleCallbacksInvoker' in assembly 'Mono.Android, …'.

The reason for the failure is that when using Reflection APIs such as
`Type.GetType()`, the [`Type.AssemblyQualifiedName` grammar][0] must
be followed, and that grammar uses `+` to "Precede a nested class",
*not* `/`.  (`/` isn't even in the Reflection grammar!)

(Aside: where does `/` come from then?  It's the *IL* separator for
nested types!)

For eventual CoreCLR and NativeAOT support, then, we need to replace
`/` with `+` *somewhere* before it hits `Type.GetType()`.

There are (at least?) three places to do so:

 1. Within `JniRuntime.JniTypeManager.RegisterNativeMembers()` or
    equivalent override.

 2. Within `generator`, updating the contents of `[Register]`.

 3. Within `Java.Interop.Tools.JavaCallableWrappers`.

(1) is rejected out of hand as it would be additional work done on-
device at runtime.  Why do that if we don't need to?

(2) was attempted in dotnet/java-interop#1302 & dotnet/android#9750.
It turned into a bit of a boondoggle, because there are lots of
linker steps which interpret the `connector` value on `[Register]`,
and it "just worked" that the `connector` value contained IL names,
as the linker steps deal in IL!  Trying to update `connector` to
instead contain Reflection syntax required finding all the places in
the linker that used `connector` values, which was tedious & brittle.

"Just" (2) is also inadequate, as it would require new binding
assemblies to take advantage of, so (2) *also* needed (3).

Which brings us to (3), the current approach: *don't* alter the
semantics of the `connect` value within `[Register]`, and instead
require that `Java.Interop.Tools.JavaCallableWrappers` replace all
instances of `/` with `+` within `__md_methods`.  This is needed
*anyway*, for compatibility with existing bindings, and also avoids
lots of pain that (2) encountered.

With this approach, `generator` output is unchanged, and
`jcw-gen` output for `MauiApplication.ActivityLifecycleCallbacks`
becomes:

	// Java Callable Wrapper
	/* partial */ class MauiApplication_ActivityLifecycleCallbacks
	{
	  public static final String __md_methods;
	  static {
	    __md_methods =
	      // …
	      "n_onActivityCreated:(Landroid/app/Activity;Landroid/os/Bundle;)V:GetOnActivityCreated_Landroid_app_Activity_Landroid_os_Bundle_Handler:Android.App.Application+IActivityLifecycleCallbacksInvoker, Mono.Android, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null\n" +
	      // …
	      "";
	    mono.android.Runtime.register ("Microsoft.Maui.MauiApplication+ActivityLifecycleCallbacks, Microsoft.Maui", MauiApplication_ActivityLifecycleCallbacks.class, __md_methods);
	  }
	}

[0]: https://learn.microsoft.com/en-us/dotnet/api/system.type.assemblyqualifiedname?view=net-9.0#remarks

@jonpryor
Copy link
Contributor Author

jonpryor commented Feb 6, 2025

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Context: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=10949922&view=logs&j=fc903661-9840-55b3-f380-f59f595bd021&t=96c38280-19e2-5308-b6a5-3c71c0cffc81
Context: #1302

PR #1302 is failing on Windows for a non-obvious reason:

    Java.Interop.Export-Tests -> D:\a\1\s\bin\TestRelease-net8.0\Java.Interop.Export-Tests.dll
      Possible reasons for this include:
        * You misspelled a built-in dotnet command.
        * You intended to execute a .NET program, but dotnet-D:\a\1\s\bin\Release-net8.0\/jcw-gen.dll does not exist. Could not execute because the specified command or file was not found. * You intended to run a global tool, but a dotnet-prefixed executable with this name could not be found on the PATH. ##[error]tests\Java.Interop.Export-Tests\Java.Interop.Export-Tests.targets(20,5): Error MSB3073: The command "dotnet "D:\a\1\s\bin\Release-net8.0\/jcw-gen.dll" "D:\a\1\s\bin\TestRelease-net8.0\Java.Interop.Export-Tests.dll" --codegen-target JavaInterop1 -o "obj\\Release-net8.0\/java" -L "D:\a\1\s\src\Java.Base\bin\Release\ref\." -L "D:\a\1\s\bin\Release-net8.0\ref\." -L "D:\a\1\s\bin\TestRelease-net8.0\ref\." -L "C:\hostedtoolcache\windows\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.7\ref\net8.0\." -L "C:\Users\VssAdministrator\.nuget\packages\microsoft.testplatform.testhost\17.5.0-preview-20221003-04\lib\netcoreapp3.1\." -L "C:\Users\VssAdministrator\.nuget\packages\microsoft.codecoverage\17.5.0-preview-20221003-04\lib\netcoreapp3.1\." -L "C:\Users\VssAdministrator\.nuget\packages\mono.linq.expressions\2.0.0\lib\netstandard2.0\." -L "C:\Users\VssAdministrator\.nuget\packages\mono.options\6.12.0.148\lib\netstandard2.0\." -L "C:\Users\VssAdministrator\.nuget\packages\newtonsoft.json\13.0.1\lib\netstandard2.0\." -L "C:\Users\VssAdministrator\.nuget\packages\nuget.frameworks\5.11.0\lib\netstandard2.0\." -L "C:\Users\VssAdministrator\.nuget\packages\nunit\3.13.2\lib\netstandard2.0\." -L "D:\a\1\s\external\xamarin-android-tools\bin\Release\net6.0\ref\."" exited with code 1. D:\a\1\s\tests\Java.Interop.Export-Tests\Java.Interop.Export-Tests.targets(20,5): error MSB3073: The command "dotnet "D:\a\1\s\bin\Release-net8.0\/jcw-gen.dll" "D:\a\1\s\bin\TestRelease-net8.0\Java.Interop.Export-Tests.dll" --codegen-target JavaInterop1 -o "obj\\Release-net8.0\/java" -L "D:\a\1\s\src\Java.Base\bin\Release\ref\." -L "D:\a\1\s\bin\Release-net8.0\ref\." -L "D:\a\1\s\bin\TestRelease-net8.0\ref\." -L "C:\hostedtoolcache\windows\dotnet\packs\Microsoft.NETCore.App.Ref\8.0.7\ref\net8.0\." -L "C:\Users\VssAdministrator\.nuget\packages\microsoft.testplatform.testhost\17.5.0-preview-20221003-04\lib\netcoreapp3.1\." -L "C:\Users\VssAdministrator\.nuget\packages\microsoft.codecoverage\17.5.0-preview-20221003-04\lib\netcoreapp3.1\." -L "C:\Users\VssAdministrator\.nuget\packages\mono.linq.expressions\2.0.0\lib\netstandard2.0\." -L "C:\Users\VssAdministrator\.nuget\packages\mono.options\6.12.0.148\lib\netstandard2.0\." -L "C:\Users\VssAdministrator\.nuget\packages\newtonsoft.json\13.0.1\lib\netstandard2.0\." -L "C:\Users\VssAdministrator\.nuget\packages\nuget.frameworks\5.11.0\lib\netstandard2.0\." -L "C:\Users\VssAdministrator\.nuget\packages\nunit\3.13.2\lib\netstandard2.0\." -L "D:\a\1\s\external\xamarin-android-tools\bin\Release\net6.0\ref\."" exited with code 1. [D:\a\1\s\tests\Java.Interop.Export-Tests\Java.Interop.Export-Tests.csproj]

Given that *later* in the build, we see:

      jcw-gen -> D:\a\1\s\bin\Release-net8.0\jcw-gen.dll

it looks like we have a build ordering issue, and
`tests/Java.Interop.Export-Tests` is being run *before* `jcw-gen`?

Add a `@(PackagReference)` to `Java.Interop.Export-Tests.csproj`
to ensure `jcw-gen.csproj` is built first.
@jonpryor
Copy link
Contributor Author

jonpryor commented Feb 6, 2025

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jonpryor jonpryor requested review from jpobst, Copilot and jonathanpeppers and removed request for jpobst February 6, 2025 19:22
@jonpryor jonpryor changed the title [jcw-gen] Use + for ntested types, not / [jcw-gen] Use + for nested types, not / Feb 6, 2025
@jonpryor jonpryor requested a review from jpobst February 6, 2025 19:22
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 7 changed files in this pull request and generated no comments.

Files not reviewed (2)
  • tests/Java.Interop.Export-Tests/Java.Interop.Export-Tests.csproj: Language not supported
  • src/Java.Interop.Tools.JavaCallableWrappers/Java.Interop.Tools.JavaCallableWrappers.Adapters/CecilImporter.cs: Evaluated as low risk

@jonpryor jonpryor merged commit 6bc87e8 into main Feb 6, 2025
4 checks passed
@jonpryor jonpryor deleted the dev/jonp/jonp-dotnet-requires-plus-not-slash-take2 branch February 6, 2025 20:47
jonpryor added a commit to dotnet/android that referenced this pull request Feb 7, 2025
Context: #9747

Changes: dotnet/java-interop@dd3c1d0...6bc87e8

  * dotnet/java-interop@6bc87e8b: [jcw-gen] Use `+` for nested types, not `/` (dotnet/java-interop#1304)

dotnet/java-interop@6bc87e8b is needed to unblock parts of #9747.

Additionally, two "quality of life" changes:

Firstly, update the `src/Mono.Android` build so that if there are
API breaks reported, the breakage is collated into 
`src/Mono.Android/ApiCompatLinesToAdd.txt` in a format that can be
directly copied into 
`tests/api-compatibility/acceptable-breakages-vReference-*.txt` if
deemed useful.

Previously, *all* changes were printed to the build log, which was
annoying to deal with because (1) there may be duplicates, and
(2) the lines would contain `TaskId`/etc. "noise" that would need to
be removed in order to be used.

Secondly, update `build-tools/xaprepare` to improve reliability when
running `Step_InstallDotNetPreview`.  `Step_InstallDotNetPreview`
will cache e.g. `dotnet-install.sh` into `$HOME/android-archives`,
but there was no logic to verify that it was still *valid*.

We've been seeing some recurring build failures in **macOS > Build**
such as:

	Downloading dotnet-install script...
	Warning: Using cached installation script found in '/Users/builder/android-archives/dotnet-install.sh'
	Discovering download URLs for dotnet SDK '10.0.100-preview.2.25102.3'...
	Downloading dotnet archive...
	dotnet archive URL https://dotnetcli.azureedge.net/dotnet/Sdk/10.0.100-preview.2.25102.3/dotnet-sdk-10.0.100-preview.2.25102.3-osx-arm64.tar.gz not found
	Downloading dotnet archive...
	dotnet archive URL https://dotnetcli.azureedge.net/dotnet/Sdk/10.0.100-preview.2.25102.3/dotnet-dev-osx-arm64.10.0.100-preview.2.25102.3.tar.gz not found
	Downloading dotnet archive...
	Warning: Failed to obtain dotnet archive size. HTTP status code: InternalServerError (500)
	Downloading dotnet archive...
	Warning: Failed to obtain dotnet archive size. HTTP status code: InternalServerError (500)
	  Error: Installation of dotnet SDK '10.0.100-preview.2.25102.3' failed.

	Step Xamarin.Android.Prepare.Step_InstallDotNetPreview failed
	System.InvalidOperationException: Step Xamarin.Android.Prepare.Step_InstallDotNetPreview failed
	   at Xamarin.Android.Prepare.Scenario.Run(Context context, Log log) in /Users/builder/azdo/_work/8/s/xamarin-android/build-tools/xaprepare/xaprepare/Application/Scenario.cs:line 50
	   at Xamarin.Android.Prepare.Context.Execute() in /Users/builder/azdo/_work/8/s/xamarin-android/build-tools/xaprepare/xaprepare/Application/Context.cs:line 488
	   at Xamarin.Android.Prepare.App.Run(String[] args) in /Users/builder/azdo/_work/8/s/xamarin-android/build-tools/xaprepare/xaprepare/Main.cs:line 155

Indeed, [`dotnet-sdk-10.0.100-preview.2.25102.3-osx-arm64.tar.gz`][0]
no longer exists on <https://dotnetcli.azureedge.net>.

The problem, though, is that .NET changed the CDN that is used in the
past month, and that's not the correct URL.  A newer `dotnet-install.sh`
reports:

	% bash "…/dotnet-install.sh" "--version" "10.0.100-preview.2.25102.3" "--install-dir" "/Volumes/Xamarin-Work/src/dotnet/android/bin/Release/dotnet" "--verbose" "--dry-run
	…
	dotnet-install: Link 0: primary, 10.0.100-preview.2.25102.3, https://builds.dotnet.microsoft.com/dotnet/Sdk/10.0.100-preview.2.25102.3/dotnet-sdk-10.0.100-preview.2.25102.3-osx-x64.tar.gz
	dotnet-install: Link 1: legacy, 10.0.100-preview.2.25102.3, https://builds.dotnet.microsoft.com/dotnet/Sdk/10.0.100-preview.2.25102.3/dotnet-dev-osx-x64.10.0.100-preview.2.25102.3.tar.gz
	dotnet-install: Link 2: primary, 10.0.100-preview.2.25102.3, https://ci.dot.net/public/Sdk/10.0.100-preview.2.25102.3/dotnet-sdk-10.0.100-preview.2.25102.3-osx-x64.tar.gz
	dotnet-install: Link 3: legacy, 10.0.100-preview.2.25102.3, https://ci.dot.net/public/Sdk/10.0.100-preview.2.25102.3/dotnet-dev-osx-x64.10.0.100-preview.2.25102.3.tar.gz

Note the different domain, <https://builds.dotnet.microsoft.com>!

Update `Step_InstallDotNetPreview` to try to install .NET potentially
*twice*: the first time using the cached `dotnet-install.sh`, and
*if that fails*, it tries again after downloading a *new*
`dotnet-install.sh`.

Hopefully this will fix the build failure on this machine!

[0]: https://dotnetcli.azureedge.net/dotnet/Sdk/10.0.100-preview.2.25102.3/dotnet-sdk-10.0.100-preview.2.25102.3-osx-arm64.tar.gz
@github-actions github-actions bot locked and limited conversation to collaborators Mar 9, 2025
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