search

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
201 stars 53 forks source link

Do not generate `SupportedOSPlatformAttribute` type on net6? #777

Closed radekdoulik closed 3 years ago

radekdoulik commented 3 years ago

Commit https://github.com/xamarin/java.interop/commit/da12df4535422f2aaf3a7570326336b30cd50b55 added support for using SupportedOSPlatformAttribute.

In XA it breaks CI builds as we are getting this warning in DotNetBuild test and the test fails when we get any warning:

/Users/runner/work/1/s/bin/TestRelease/temp/DotNetBuildandroid.21-armandroid.21-arm64android.21-x86android.21-x64False/obj/Debug/net6.0-android/UnnamedProject.AssemblyInfo.cs(21,38): warning CS0436: The type 'SupportedOSPlatformAttribute' in '/Users/runner/work/1/s/bin/TestRelease/temp/DotNetBuildandroid.21-armandroid.21-arm64android.21-x86android.21-x64False/obj/Debug/net6.0-android/generated/src/__NamespaceMapping__.cs' conflicts with the imported type 'SupportedOSPlatformAttribute' in 'System.Runtime, Version=5.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'. Using the type defined in '/Users/runner/work/1/s/bin/TestRelease/temp/DotNetBuildandroid.21-armandroid.21-arm64android.21-x86android.21-x64False/obj/Debug/net6.0-android/generated/src/__NamespaceMapping__.cs'. [/Users/runner/work/1/s/bin/TestRelease/temp/DotNetBuildandroid.21-armandroid.21-arm64android.21-x86android.21-x64False/UnnamedProject.csproj]

Complete build.log from https://github.com/xamarin/xamarin-android/pull/5444 CI build

Looks like we should not be generating own SupportedOSPlatformAttribute type on net6?

radekdoulik commented 3 years ago

It is probably caused by this line? https://github.com/xamarin/java.interop/blob/da12df4535422f2aaf3a7570326336b30cd50b55/tools/generator/Java.Interop.Tools.Generator.ObjectModel/NamespaceMapping.cs#L38

We should probably use #if !NETCOREAPP instead? As we still build our libraries and tools with netcoreapp3.1.

jpobst commented 3 years ago

We definitely need to have it for netcoreapp3.1 as the type does not exist until net5.0.

I am trying to understand this test, is it building with netcoreapp3.1 but referencing net5.0 BCL? That would be a problem.

https://github.com/xamarin/xamarin-android/blob/master/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/XASdkTests.cs#L338

cc: @jonathanpeppers

jonathanpeppers commented 3 years ago

@jpobst MSBuild in these tests run out of process. This test build the actual project with ~\android-toolchain\dotnet\dotnet.exe build, but the C# code in this test is netcoreapp3.1.

We could change this now if we need to.

radekdoulik commented 3 years ago

The test already uses net6.0-android TFM. So maybe there is an issue with NET5_0_OR_GREATER define missing.

From the log I see these defines when compiling the test:

/define:TRACE;DEBUG;NET;NET6_0;NETCOREAPP;__XAMARIN_ANDROID_v1_0__;__MOBILE__;__ANDROID__;__ANDROID_1__;__ANDROID_2__;__ANDROID_3__;__ANDROID_4__;__ANDROID_5__;__ANDROID_6__;__ANDROID_7__;__ANDROID_8__;__ANDROID_9__;__ANDROID_10__;__ANDROID_11__;__ANDROID_12__;__ANDROID_13__;__ANDROID_14__;__ANDROID_15__;__ANDROID_16__;__ANDROID_17__;__ANDROID_18__;__ANDROID_19__;__ANDROID_20__;__ANDROID_21__;__ANDROID_22__;__ANDROID_23__;__ANDROID_24__;__ANDROID_25__;__ANDROID_26__;__ANDROID_27__;__ANDROID_28__;__ANDROID_29__;__ANDROID_30__;ANDROID
radekdoulik commented 3 years ago

It looks like the define might still be only in design phase? https://github.com/dotnet/designs/blob/main/accepted/2020/or-greater-defines/or-greater-defines.md

I think we can use NET instead and change this and other places which use NETCOREAPP once that the define is available.

radekdoulik commented 3 years ago

Ah, I missed the last line of the design:

Due to a lot of confusion and concern we ended up not shipping this design.

So that's why the define is missing.