dotnet / java-interop

Java.Interop provides open-source bindings of Java's Java Native Interface (JNI) for use with .NET managed languages such as C#
Other
189 stars 48 forks source link

[Java.Interop.Tools.JavaCallableWrappers] fix net8.0 targeting in XA #1197

Closed jonathanpeppers closed 4 months ago

jonathanpeppers commented 4 months ago

xamarin/xamarin-android's build began failing with:

(CoreCompile target) ->
Xamarin.Android.Build.Tasks\Utilities\MamJsonParser.cs(92,43): error CS0122: 'NotNullWhenAttribute' is inaccessible due to its protection level [Xamarin.Android.Build.Tasks\Xamarin.Android.Build.Tasks.csproj]
Xamarin.Android.Build.Tasks\Utilities\MamJsonParser.cs(92,81): error CS0122: 'NotNullWhenAttribute' is inaccessible due to its protection level [Xamarin.Android.Build.Tasks\Xamarin.Android.Build.Tasks.csproj]
Xamarin.Android.Build.Tasks\Utilities\MavenExtensions.cs(26,32): error CS0122: 'NotNullWhenAttribute' is inaccessible due to its protection level [Xamarin.Android.Build.Tasks\Xamarin.Android.Build.Tasks.csproj]

This was happening because the net8.0 build of Java.Interop.Tools.JavaCallableWrappers.dll was being used and should not be: we want the netstandard2.0 version to be used for MSBuild task assemblies.

To fix this:

This solves the build error above.

Other cleanup:

jonpryor commented 4 months ago

I'm slightly confused by this.

Xamarin.Android.Build.Tasks.csproj is a netstandard2.0 project and references Java.Interop.Tools.JavaCallableWrappers.csproj:

https://github.com/xamarin/xamarin-android/blob/7559d12bdfb717d756795bb59d928d160e5eb6fb/src/Xamarin.Android.Build.Tasks/Xamarin.Android.Build.Tasks.csproj#L226

Because Xamarin.Android.Build.Tasks.csproj is a netstandard2.0 project, shouldn't it thus build and reference the netstandard2.0 build of Java.Interop.Tools.JavaCallableWrappers.csproj?

If not, couldn't we use AdditionalProperties="TargetFramework=netstandard2.0" in the xamarin-android side to force things?

The existence of this PR suggests I don't understand something within MSBuild-land. :-)

jpobst commented 4 months ago

I wonder if it would be cleaner to create a Java.Interop.Tools.TypeNameMappings.csproj targeting $(DotNetTargetFramework) that we could build to test trimmer things. We wouldn't use the .dll anywhere, but it would prevent having to bring multitargeting and XA dependent logic into Java.Interop.Tools.JavaCallableWrappers.

jonathanpeppers commented 4 months ago

Because Xamarin.Android.Build.Tasks.csproj is a netstandard2.0 project, shouldn't it thus build and reference the netstandard2.0 build of Java.Interop.Tools.JavaCallableWrappers.csproj?

$(AppendTargetFrameworkToOutputPath) is causing the problem. It's just copying net8.0 on top.

jonpryor commented 4 months ago

@jonathanpeppers wrote:

$(AppendTargetFrameworkToOutputPath) is causing the problem. It's just copying net8.0 on top.

Ah. What if we flipped the order, so that it was instead:

<TargetFrameworks>$(DotNetTargetFramework);netstandard2.0</TargetFrameworks>

Would that alter anything?

jonathanpeppers commented 4 months ago

This would maybe work:

<TargetFrameworks>$(DotNetTargetFramework);netstandard2.0</TargetFrameworks>

But incremental builds would run every time, because the two target frameworks copy on top of each other.

I used $(XABuild) because I found it in another file. Let me look for a cleaner way.

jonathanpeppers commented 4 months ago

XAConfig.props does not appear to exist either.

jpobst commented 4 months ago

Ah. What if we flipped the order, so that it was instead: ...

Builds are done in parallel, so it is likely a race condition.

It can also lead to this error:

Error CS2012: Cannot open 'D:\a\_work\1\s\src\Java.Interop.Tools.JavaCallableWrappers\obj\Release-net8.0\Java.Interop.Tools.JavaCallableWrappers.dll' for writing -- 'The process cannot access the file 'D:\a\_work\1\s\src\Java.Interop.Tools.JavaCallableWrappers\obj\Release-net8.0\Java.Interop.Tools.JavaCallableWrappers.dll' because it is being used by another process.'
jonpryor commented 4 months ago
Context: https://github.com/xamarin/xamarin-android/pull/8748
Context: 56b7eeb771aceb7d47b31a4337f7b0e73ba74447

xamarin/xamarin-android#8748 attempts to bump xamarin-android to use
commit 56b7eeb7, and fails to build:

    (CoreCompile target) ->
    Xamarin.Android.Build.Tasks\Utilities\MamJsonParser.cs(92,43): error CS0122: 'NotNullWhenAttribute' is inaccessible due to its protection level [Xamarin.Android.Build.Tasks\Xamarin.Android.Build.Tasks.csproj]
    Xamarin.Android.Build.Tasks\Utilities\MamJsonParser.cs(92,81): error CS0122: 'NotNullWhenAttribute' is inaccessible due to its protection level [Xamarin.Android.Build.Tasks\Xamarin.Android.Build.Tasks.csproj]
    Xamarin.Android.Build.Tasks\Utilities\MavenExtensions.cs(26,32): error CS0122: 'NotNullWhenAttribute' is inaccessible due to its protection level [Xamarin.Android.Build.Tasks\Xamarin.Android.Build.Tasks.csproj]

The cause of the failure is that 56955d9a updated
`Java.Interop.Tools.JavaCallableWrappers.csproj` to multitarget both
netstandard2.0 and net8.0.  This is nominally "fine", except that
`Java.Interop.Tools.JavaCallableWrappers.csproj` *also* sets
[`$(AppendTargetFrameworkToOutputPath)`][0]=false, which means that
both builds use the *same* output directory.  This means that the
netstandard2.0 and net8.0 build outputs clobber each other -- in
parallel! -- which means that the `Xamarin.Android.Build.Tasks.csproj`
build doesn't reliably use the netstandard2.0 output assembly.

Fix this scenario by no longer overriding the
`$(AppendTargetFrameworkToOutputPath)` MSBuild property, and only
setting `$(OutputPath)` to `$(ToolOutputFullPath)` for netstandard2.0
builds.

Finally, remove use of `XAConfig.props`.  This appears to be vestigial,
as we can't find any current code that would produce this file.

[0]: https://learn.microsoft.com/dotnet/core/project-sdk/msbuild-props#appendtargetframeworktooutputpath