Skip to content

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

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 3 commits into from
Feb 23, 2024

Conversation

jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Feb 22, 2024

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:

  • Only set $(OutputPath) to $(ToolOutputFullPath) for netstandard2.0

This solves the build error above.

Other cleanup:

  • No need to import XAConfig.props, as it no longer exists

  • No need to set $(AppendTargetFrameworkToOutputPath), as it is already in Directory.Build.props

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, let's only build
`netstandard2.0` when `$(XABuild)` is `true`.

We should also avoid using `$(AppendTargetFrameworkToOutputPath)=false`
when multi-targeting: stop using it inside XA's build.
jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this pull request Feb 22, 2024
@jonpryor
Copy link
Contributor

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
Copy link
Contributor

jpobst commented Feb 22, 2024

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
Copy link
Member Author

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
Copy link
Contributor

@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
Copy link
Member Author

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
Copy link
Member Author

XAConfig.props does not appear to exist either.

@jpobst
Copy link
Contributor

jpobst commented Feb 22, 2024

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.'

@jonathanpeppers jonathanpeppers changed the title [Java.Interop.Tools.JavaCallableWrappers] don't build net8.0 in XA [Java.Interop.Tools.JavaCallableWrappers] fix net8.0 targeting in XA Feb 23, 2024
@jonathanpeppers jonathanpeppers marked this pull request as ready for review February 23, 2024 03:08
Comment on lines -22 to 19
<PropertyGroup>
<!-- Only use $(ToolOutputFullPath) for netstandard2.0 -->
<PropertyGroup Condition=" '$(TargetFramework)' == 'netstandard2.0' ">
<OutputPath>$(ToolOutputFullPath)</OutputPath>
</PropertyGroup>
Copy link
Member Author

@jonathanpeppers jonathanpeppers Feb 23, 2024

Choose a reason for hiding this comment

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

I think what I ended up with is simple enough. This prevents us from putting the net8.0 library in $(ToolOutputFullPath).

The build error is gone in dotnet/android#8751, but we still have some runtime test failures.

@jonpryor
Copy link
Contributor

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

@jonpryor jonpryor merged commit 67c079c into main Feb 23, 2024
@jonpryor jonpryor deleted the Java.Interop.Tools.JavaCallableWrappers-Build branch February 23, 2024 19:36
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2024
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