Skip to content
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

[tests] Add net6.0 TargetFramework. #888

Merged
merged 4 commits into from
Oct 12, 2021
Merged

[tests] Add net6.0 TargetFramework. #888

merged 4 commits into from
Oct 12, 2021

Conversation

jpobst
Copy link
Contributor

@jpobst jpobst commented Oct 7, 2021

Today, even when building and testing on .NET 6, most of our test suites are still building and running with net472. Fix this by adding the net6.0 <TargetFramework> and updating CI to run the net6.0 version instead of the net472 version.

Fixes were required to account for differences between net472 and net6.0 behavior.

@jpobst jpobst force-pushed the net6-tests branch 2 times, most recently from fcf1ae6 to 994c890 Compare October 7, 2021 21:17
@jpobst jpobst marked this pull request as ready for review October 8, 2021 14:18
@@ -12,6 +12,7 @@

namespace Java.InteropTests
{
#if !NET
Copy link
Member

Choose a reason for hiding this comment

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

What error is being observed that requires disabling this fixture?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Compile errors:

error CS0117: 'AssemblyBuilderAccess' does not contain a definition for 'Save'
error CS1061: 'AppDomain' does not contain a definition for 'DefineDynamicAssembly' and no accessible extension method 'DefineDynamicAssembly' accepting a first argument of type 'AppDomain' could be found (are you missing a using directive or an assembly reference?)
error CS1061: 'LambdaExpression' does not contain a definition for 'CompileToMethod' and no accessible extension method 'CompileToMethod' accepting a first argument of type 'LambdaExpression' could be found (are you missing a using directive or an assembly reference?)

@@ -114,25 +114,41 @@ public void ClassFile_WithJavaType_class ()
Name = "STATIC_FINAL_SINGLE_MIN",
Descriptor = "F",
AccessFlags = FieldAccessFlags.Public | FieldAccessFlags.Static | FieldAccessFlags.Final,
#if NET
ConstantValue = "Float(1E-45)",
Copy link
Member

Choose a reason for hiding this comment

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

I think the .NET rounding algorithm changed?

I think this might be better addressed by "forcing" the rounding mode within the ConstantPoolItem subclasses, e.g.

diff --git a/src/Xamarin.Android.Tools.Bytecode/ConstantPool.cs b/src/Xamarin.Android.Tools.Bytecode/ConstantPool.cs
index a030f37f..59eda31f 100644
--- a/src/Xamarin.Android.Tools.Bytecode/ConstantPool.cs
+++ b/src/Xamarin.Android.Tools.Bytecode/ConstantPool.cs
@@ -260,7 +260,7 @@ namespace Xamarin.Android.Tools.Bytecode {
 
 		public override string ToString ()
 		{
-			return string.Format (CultureInfo.InvariantCulture, "Float({0})", Value);
+			return string.Format (CultureInfo.InvariantCulture, "Float({0:E6})", Value);
 		}
 	}
 
@@ -329,7 +329,7 @@ namespace Xamarin.Android.Tools.Bytecode {
 
 		public override string ToString ()
 		{
-			return string.Format (CultureInfo.InvariantCulture, "Double({0})", Value);
+			return string.Format (CultureInfo.InvariantCulture, "Double({0:E14})", Value);
 		}
 	}
 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The actual output we are writing to XML files is using the "R" format specifier, which is producing different values based on target framework. The documentation suggests using "G9" and "G17" instead for roundtripping.

I have updated our code to use this and tests pass. This is technically a functional change, but it seems like it should not cause any issues.

@@ -0,0 +1,614 @@
<api
Copy link
Member

Choose a reason for hiding this comment

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

I'm not fond that this file exists, as it (again) suggests differing output based on whether net6 or mono is used.

Without running diff -u JavaType.xml JavaType-net.xml, I'm going to assume that the diffs are due to float/double representation differences. If this is the case, the solution is to as done above, and force a consistent rounding length, so that Mono & net6 produce identical output.

MetadataReference.CreateFromFile (Path.Combine (facDir, "netstandard.dll"))
MetadataReference.CreateFromFile (Path.Combine (facDir, "netstandard.dll")),
#if NET
MetadataReference.CreateFromFile (Path.Combine (facDir, "System.Runtime.dll")),
Copy link
Member

Choose a reason for hiding this comment

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

Why is this reference required? It doesn't appear to have anything:

 % monodis --typedef /usr/local/share/dotnet/shared/Microsoft.NETCore.App/6.0.0-preview.7.21377.19/System.Runtime.dll       
Typedef Table
1: (null) (flist=1, mlist=1, flags=0x0, extends=0x0)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CS0012: The type 'Enum' is defined in an assembly that is not referenced. You must add a reference to assembly 'System.Runtime, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'. (SourceFile([871..891)))
CS0012: The type 'ValueType' is defined in an assembly that is not referenced. You must add a reference to assembly 'System.Runtime, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'. (SourceFile([944..962)))
CS0012: The type 'IDisposable' is defined in an assembly that is not referenced. You must add a reference to assembly 'System.Runtime, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'. (SourceFile([312..321)))

@jonpryor
Copy link
Member

Today, even when building and testing on .NET 6, most of our test
suites are still building and running with `net472`.

Fix this by adding `net6.0` to `$(TargetFrameworks)` and updating CI
to run the `net6.0` version instead of the `net472` version.

Fixes were required to account for differences between `net472` and
`net6.0` behavior, in particular:

  * The semantics for the [Round-trip format specifier `R`][0]
    changed between .NET Framework and .NET 6.
    Update `ConstantPoolFloatItem.ToString()` and
    `ConstantPoolDoubleItem.ToString()` to use the G9 and G17
    format specifiers, respectively, so that behavior is consistent.

  * Types such as `System.String` moved from `mscorlib.dll` to
    `System.Private.CoreLib.dll`, which impacts the output of
    `JavaNativeTypeManager.GetPackageName()`, as the assembly name
    is taken consideration.  Assertions around
    `JavaNativeTypeManager.GetPackageName()` need to be based on the
    target framework.

  * When using `CSharpCompilation.Create()`, `System.Runtime.dll`
    must now be referenced when building on net6+, otherwise certain
    types won't be resolved:

        error CS0012: The type 'Enum' is defined in an assembly that is not referenced. You must add a reference to assembly 'System.Runtime, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'. (SourceFile([871..891)))
        error CS0012: The type 'ValueType' is defined in an assembly that is not referenced. You must add a reference to assembly 'System.Runtime, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'. (SourceFile([944..962)))
        error CS0012: The type 'IDisposable' is defined in an assembly that is not referenced. You must add a reference to assembly 'System.Runtime, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'. (SourceFile([312..321)))

  * `MarshalMemberBuilderTest.cs` must be disabled when building for
    .NET 6+, as `AssemblyBuilderAccess.Save` doesn't exist, nor does
    `AppDomain.DefineDynamicAssembly()`, nor
    `LambdaExpression.CompileToMethod()`.
    Related: https://github.com/xamarin/java.interop/issues/616

        error CS0117: 'AssemblyBuilderAccess' does not contain a definition for 'Save'
        error CS1061: 'AppDomain' does not contain a definition for 'DefineDynamicAssembly' and no accessible extension method 'DefineDynamicAssembly' accepting a first argument of type 'AppDomain' could be found (are you missing a using directive or an assembly reference?)
        error CS1061: 'LambdaExpression' does not contain a definition for 'CompileToMethod' and no accessible extension method 'CompileToMethod' accepting a first argument of type 'LambdaExpression' could be found (are you missing a using directive or an assembly reference?)

[0]: https://docs.microsoft.com/en-us/dotnet/standard/base-types/standard-numeric-format-strings#round-trip-format-specifier-r

@jonpryor jonpryor merged commit 41ba348 into main Oct 12, 2021
@jonpryor jonpryor deleted the net6-tests branch October 12, 2021 00:30
@github-actions github-actions bot locked and limited conversation to collaborators Apr 13, 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.

2 participants