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

[master] [generator] Do not generate PlatformNotSupportedException in chaining .ctor #7092

Merged
merged 2 commits into from
Sep 25, 2019

Conversation

monojenkins
Copy link
Collaborator

Types that are new in 64bits only OS are generated differently on 32bits
bindings. They mainly throw a PlatformNotSupportedException so it's
easier to diagnose (than a crash) what's happening at runtime.

This works well in all cases except one. When a new type, let's say
UIMenuElement is added and serves as a new base type for existing
types.

UIKeyCommand (iOS 7) -> UICommand (iOS 13)-> UIMenuElement (iOS 13)

This is correct as new base types can be added (in ObjC and C#).
However the generated code for the constructors of UICommand and
UIMenuElement would be throwing a PlatformNotSupportedException
which breaks the UIKeyCommand on 32 bits devices.

We fixed this in a few places by tweaking the availability attribute
but that requires spotting the new base type while doing bindings and
that is error prone [1][2].

This PR simply does let the protected constructor, using when chaining,
be generated normally. It's simpler and will cover all the cases (without
requiring hacks in the availability of those types)

[1] #7083
[2] #7084

Backport of #7085.

/cc @spouliot

Sebastien Pouliot added 2 commits September 24, 2019 20:39
… .ctor

Types that are new in 64bits only OS are generated differently on 32bits
bindings. They mainly throw a `PlatformNotSupportedException` so it's
easier to diagnose (than a crash) what's happening at runtime.

This works well in all cases except one. When a new type, let's say
`UIMenuElement` is added **and** serves as a new base type for existing
types.

`UIKeyCommand` (iOS 7) -> `UICommand` (iOS 13)-> `UIMenuElement` (iOS 13)

This is _correct_ as new base types can be added (in ObjC and C#).
However the generated code for the constructors of `UICommand` and
`UIMenuElement` would be throwing a `PlatformNotSupportedException`
which breaks the `UIKeyCommand` on 32 bits devices.

We fixed this in a few places by tweaking the availability attribute
but that requires spotting the new base type while doing bindings and
that is error prone [1][2].

This PR simply does let the `protected` constructor, using when chaining,
be generated normally. It's simpler and will cover all the cases (without
requiring hacks in the availability of those types)

[1] xamarin#7083
[2] xamarin#7084
@monojenkins
Copy link
Collaborator Author

Build success
Build succeeded
API Diff (from stable)
API Diff (from PR only) (no change)
ℹ️ Generator Diff (please review changes)
Test run succeeded

@spouliot spouliot merged commit abfa32c into xamarin:master Sep 25, 2019
@LeoJHarris
Copy link

Hi there, any possible eta when this will be released and available?
Regarding: #7084

@AntRemo
Copy link

AntRemo commented Oct 3, 2019

Hi @spouliot

Any new updates on when this may be available in Stable. This is affecting our production users.

Thanks 🙏

@VincentDondain
Copy link
Contributor

Hi @AntRemo this is going out very very soon. I will make sure to ping you here when it's out, I'm writing the release notes for it right now (:

@AntRemo
Copy link

AntRemo commented Oct 9, 2019

Thanks @VincentDondain !

I really appreciate it!! 😄🙏

@LeoJHarris
Copy link

@VincentDondain Thats great, until then please dont update the preview Xamarin.iOS version as thats all we have to make new uploads and builds for older iOS devices! Also a little bit more update would be great particularly for breaking issues like these :) dont wanna sound like a whinge but it would help us out just so we are not left in the dark :)

@AntRemo
Copy link

AntRemo commented Oct 9, 2019

@LeoJHarris That's funny because Dark Mode is the reason why I updated 😂

@chamons
Copy link
Contributor

chamons commented Oct 9, 2019

@LeoJHarris I understand the frustration when your tools break.

Part of the challenge with iOS development is that Apple constantly pushes the tooling forward. They regularly updates the minimum Xcode required to submit applications, and customers expect applications to use the latest features (such as dark mode support).

As an SDK that exposes those APIs into C#, we have to "keep up" with the latest changes from Apple or we quickly hold back many customer's use cases.

In this case, a change required to support new iOS 13 APIs impacted 32-bit device support due to a quirk in how our tooling generated that code. Things of this nature are unfortunate, if rare. We should have caught it at a number of levels of testing, but it is a subtle bug.

In hindsight, we likely should have updated the iOS 13.2 release notes to mention this issue, bug given the relatively rapid turn around that was overlooked.

Despite this, we generally funnel users to open issues when they run into regressions, as what's a duplicate known issue compared to a new issue is not always easily apparent.

I hope that helps you understand the situation.

We always welcome feedback on how we can improve.

@AntRemo
Copy link

AntRemo commented Oct 9, 2019

For what it's worth, even though I very much want this bug fixed ASAP, I fault myself for not properly testing my app before releasing.

I honestly can't remember the last time I had an issue due to a Xamarin.iOS upgrade. The one time I don't test on all devices and boom.

Looking forward to one day being able to use Test Cloud, or whatever the marketing folks are calling it these days. 😊

@VincentDondain
Copy link
Contributor

@LeoJHarris @AntRemo we just released Xamarin.iOS 13.4 to stable. You can see the release notes here: https://docs.microsoft.com/en-us/xamarin/ios/release-notes/13/13.4. It mentions the fix for this regression specifically.

@AntRemo
Copy link

AntRemo commented Oct 10, 2019

@VincentDondain that fixed it for me!

Many thanks for the fix! 😄🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants