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

[release/6.0] Expand System.Runtime.InteropServices.NFloat to support the APIs required by Xamarin #64556

Merged
merged 13 commits into from
Feb 11, 2022

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Jan 31, 2022

Backport of #64234 to release/6.0

/cc @tannergooding

Customer Impact

Xamarin is taking some breaking changes for their .NET 6 release to improve the long term experience and integration support with the language: xamarin/xamarin-macios#13087

It has been extensively discussed that this should also extend to their nfloat type (previously exposed as System.nfloat) by normalizing on the version provided "in-box" by the BCL (System.Runtime.InteropServices.NFloat). This would allow a smoother experience long term and avoid needing to continue exposing a separate but similar type specifically for iOS (which would otherwise be moved to be ObjCRuntime.nfloat).

Normalizing on the in-box version requires that it expose the operators and other APIs required for common usage be exposed. This works well for .NET 7 but represents a potential issue for .NET 6 where the base "in box" APIs and reference assemblies have already shipped as stable. The proposed solution is to backport the implementation and provide the NFloat APIs for Xamarin/MAUI by exposing a reference assembly that overrides the in box version and is included in their ref pack.

Code examples

Existing Xamarin code:

// create array of nfloat using constants
var range = new nfloat[] {0f, 1f, 0f, 1f};
// create a CGSize (struct with two NFloat fields), doing math with other nfloat values
nfloat width = 10;
nfloat margin = 2;
var size = new CGSize (width - margin, float.MaxValue);
// rect math (from https://github.com/migueldeicaza/MonoTouch.Dialog/blob/3b9f579868d4e53aefc12c30da0a3c0944ebbd69/MonoTouch.Dialog/Utilities/Graphics.cs#L25-L29)
nfloat midx = rect.Left + (rect.Width)/2;

without this PR significant changes will have to be made:

// create array of nfloat using constants - need to create the NFloats manually
var range = new NFloat[] {new NFloat (0f), new NFloat (1f), new NFloat (0f), new NFloat (1f)};
// create a CGSize (struct with two NFloat fields), doing math with other nfloat values
nfloat width = 10;
nfloat margin = 2;
var size = new CGSize (new NFloat (width.Value - margin.Value), new NFloat (float.MaxValue));
// rect math (from https://github.com/migueldeicaza/MonoTouch.Dialog/blob/3b9f579868d4e53aefc12c30da0a3c0944ebbd69/MonoTouch.Dialog/Utilities/Graphics.cs#L25-L29)
NFloat midx = new NFloat (rect.Left.Value + (rect.Width.Value)/2);

with this PR:

// create array of nfloat using constants
var range = new NFloat[] {0f, 1f, 0f, 1f};
// create a CGSize (struct with two NFloat fields), doing math with other nfloat values
NFloat width = 10;
NFloat margin = 2;
var size = new CGSize (width - margin, float.MaxValue);
// rect math (from https://github.com/migueldeicaza/MonoTouch.Dialog/blob/3b9f579868d4e53aefc12c30da0a3c0944ebbd69/MonoTouch.Dialog/Utilities/Graphics.cs#L25-L29)
NFloat midx = rect.Left + (rect.Width)/2;

In most cases there's not even a need to change the type's case (from nfloat to NFloat), since customers can do that with a global using (global using nfloat = System.Runtime.InteropServices.NFloat;), in which case the existing Xamarin code at the top would work without any changes.

Testing

The general premise of using a separate NuGet package to override the in-box reference assembly is already decently well tested/understood and was previously used for System.Runtime.Experimental to enable generic math to similarly "light up" on an opt-in basis.

Additional end to end validation would still need to be done to ensure that it correctly integrates with the build of the SDK that will include Xamarin/MAUI.

Risk

For a customer using a shared runtime from the .NET 6 release, but an SDK for the .NET 6 update that includes Maui/Xamarin and manually referencing the reference assembly they could end up in a scenario where they get a MissingMethodException at runtime. This will be mitigated by stating it is an unsupported scenario (that is the special nuget package will only be supported by Xamarin/MAUI in .NET 6; such a NuGet package won't exist/be needed for .NET 7).

For Maui/Xamarin this is believed to be a non-issue as it is only going to be used for their Apple platform targets such as iOS (where AOT is required and the correct implementation assembly will be available at build time) and MacOS where we can ensure the right assemblies are present.

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Jan 31, 2022

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #64234 to release/6.0

/cc @tannergooding

Customer Impact

Testing

Risk

Author: github-actions[bot]
Assignees: -
Labels:

area-System.Runtime.InteropServices, new-api-needs-documentation

Milestone: -

@tannergooding tannergooding added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jan 31, 2022
@tannergooding
Copy link
Member

Marked * NO MERGE * as this needs an additional change on top compared to .NET 7. Namely I need to push a commit that rather than modifying System.Runtime.InteropServices adds a separate ref assembly that works in the same manner as System.Runtime.Experimental

…rrides the built-in ref assembly for System.Runtime.InteropServices
@@ -0,0 +1,198 @@
Microsoft Visual Studio Solution File, Format Version 12.00
Copy link
Member

Choose a reason for hiding this comment

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

@safern, @ericstj could you give this commit (7ae9004) a look and make sure it looks correct?

I think the biggest difference between S.Runtime and S.R.InteropServices is that the former doesn't actually include a src project while the latter does and I'm not 100% if matching S.Runtime is "correct" here (based on what I know about NuGet, I'd guess no and I need to also include a "copy" of the source assembly with the same name/contents).


Noting that we still haven't finalized on the naming here. We are likely to change this away from System.Runtime.InteropServices.Experimental and possibly to something else as this isn't truly an "experimental" package; but rather something for Xamarin/MAUI to get access to operators on NFloat for the LTS.

Copy link
Member

Choose a reason for hiding this comment

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

Looks good to me.

@lambdageek
Copy link
Member

@tannergooding @steveisok If this goes in we need to also bring along a 6.0 backport of #64618 to fix #64570, right?

@tannergooding
Copy link
Member

tannergooding commented Feb 1, 2022

@lambdageek, if the tests go in as is yes.

Although, that fix should likely be backported anyways as its a functional bug in float/double->nint and float/double->nuint in the LTS

@tannergooding tannergooding self-assigned this Feb 4, 2022
@tannergooding
Copy link
Member

@lambdageek, is there already the relevant issues for backporting #64618?

Trying to determine if I should wait on that or if I should disable the impacted test since we're trying to get this in for Xamarin/MAUI to be unblocked.

@akoeplinger
Copy link
Member

@tannergooding #64618 was merged, please cherry-pick it into the PR :)

@tannergooding
Copy link
Member

please cherry-pick it into the PR :)

I believe it needs to goto tactics separately (via /backport to release/6.0 + filling out the template, etc). If that's not the case (CC. @jeffhandley), then happy to cherry-pick it back

@github-actions
Copy link
Contributor Author

github-actions bot commented Feb 7, 2022

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1807522739

@akoeplinger
Copy link
Member

OK, I opened #64914.

@jeffhandley jeffhandley added the Servicing-approved Approved for servicing release label Feb 7, 2022
@jeffhandley
Copy link
Member

This was discussed in Tactics last week and was approved for backporting.

@steveisok
Copy link
Member

@tannergooding does this still need to be marked as No Merge?

@tannergooding
Copy link
Member

@steveisok, this has test failures due to mono and is pending #64914 going to tactics.

We could disable the relevant tests and merge anyways (this is a general Mono issue with float/double conversions, its not specific to nfloat)

@steveisok
Copy link
Member

#64914 was approved and should go in before this PR is merged.

@tannergooding
Copy link
Member

There is notably also a CI failure that I've pinged @safern on and he's going to help look at it after lunch.

@safern
Copy link
Member

safern commented Feb 11, 2022

Infra changes to add System.Runtime.InteropServices.Experimental, LGTM

@steveisok
Copy link
Member

@tannergooding Is this ready to go now? Can we take the no merge label off?

@tannergooding
Copy link
Member

It should get merged today. I'm trying to finalize a discussion on the name of the package as there was pushback against System.Runtime.InteropServices.Experimental since this is not actually experimental.

@tannergooding tannergooding removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 11, 2022
@tannergooding
Copy link
Member

This should be mergeable now; just as soon as the last leg here finishes.

@tannergooding
Copy link
Member

Looks like I don't have permission to merge this.

The failures are unrelated and being handled by @safern in:
#65038
#64955

@safern safern merged commit 826c3d7 into release/6.0 Feb 11, 2022
@safern safern deleted the backport/pr-64234-to-release/6.0 branch February 11, 2022 23:31
@ghost ghost locked as resolved and limited conversation to collaborators Mar 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants