[generator][dotnet] Add support for [Uns|S]upportedOSPlatformAttribute
#10580
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This moves our current/legacy attributes to the ones added in dotnet 5 [1].
Short Forms (only in bindings)
| Old | New |
|---------------------------------------|-------------------------------------|
| [iOS (7,0)] | [SupportedOSPlatform ("ios7.0")] |
| [NoIOS] | [UnsupportedOSPlatform ("ios")] |
Long Forms
| Old | New |
|---------------------------------------|-------------------------------------|
| [Introduced (PlatformName.iOS, 7,0)] | [SupportedOSPlatform ("ios7.0")] |
| [Obsoleted (PlatformName.iOS, 12,1)] | [Obsolete (...)] |
| [Deprecated (PlatformName.iOS, 14,3)] | [UnsupportedOSPlatform ("ios14.3")] |
| [Unavailable (PlatformName.iOS)] | [UnsupportedOSPlatform ("ios")] |
Other changes
* `[SupportedOSPlatform]` and `[UnsupportedOSPlatform]` are not allowed on `interface` [2] which means they cannot be used for protocols. This is currently handled by inlining the existing attributes on all members.
* `[ObsoletedInOSPlatform]` was removed in net5 RC. This PR is now mapping the existing attributes to `[Obsolote]`, however multiple ones cannot be added so they need to be platform specific.
Still Missing
* [ ] Generator deduplication of type/members attributes for interfaces (due to [2])
* [ ] Manual bindings
* [ ] Introspection tests updates
References
* [1] xamarin#10170
* [2] dotnet/runtime#47599
* [3] dotnet/runtime#47601
|
Note: right now the CI does not produce the generator diff that makes it hard to review the actual changes. That should be coming soon... |
|
@spouliot bumping the generator diff up in my todo then |
|
@mandel-macaque no rush, there's quite a few more commits before it can land |
rolfbjarne
reviewed
Feb 4, 2021
❌ Tests failed on Build ❌Tests failed on Build. Test results6 tests failed, 172 tests passed.Failed tests
Pipeline on Agent XAMBOT-1095' |
It's a kind of break'ish change as the current behavior is needed to run the test (even if much less likely to affect production code)
It's defined two times - but only one had the [Deprecated] attribute
Fix existing tests. Add new `LegacyAttributes` test to spot where and how many of the old attributes are still in use (manual bindings)
❌ Tests failed on Build ❌Tests failed on Build. Test results7 tests failed, 174 tests passed.Failed tests
Pipeline on Agent XAMBOT-1104' |
❌ Tests failed on Build ❌Tests failed on Build. Test results5 tests failed, 176 tests passed.Failed tests
Pipeline on Agent XAMBOT-1094' |
❌ Tests failed on Build ❌Tests failed on Build. Test results5 tests failed, 176 tests passed.Failed tests
Pipeline on Agent XAMBOT-1106' |
❌ Tests failed on Build ❌Tests failed on Build. Test results5 tests failed, 176 tests passed.Failed tests
Pipeline on Agent XAMBOT-1095' |
* Add test to detect duplicates between members and type-level attributes to reduce the number of manual entries required to be updated * Add test to check the string argument of [SupportedOSPlatform] and [UnsupportedOSPlatform] in preparation for manual bindings updates * Fix Legacy test - check the members, not twice the type, custom attributes
❌ Tests failed on Build ❌Tests failed on Build. Test results11 tests failed, 170 tests passed.Failed tests
Pipeline on Agent XAMBOT-1100' |
❌ Tests failed on Build ❌Tests failed on Build. Test results11 tests failed, 170 tests passed.Failed tests
Pipeline on Agent XAMBOT-1095' |
spouliot
added a commit
to spouliot/xamarin-macios
that referenced
this pull request
Feb 22, 2021
- this requires small (but tricky) generator changes - duplicate availability attributes detection is tested by introspection - it also requires removing dupes in manual bindings (which was the current, if not the _named_, goal for this PR) Long story: It's always been hard to detect and enforce removal of extraneous availability attributes since the generator inlined them _freely_ when processing ObjC protocols. That meant introspection could not report dupes and led to many extra attributes. This was **not** a problem in binding files since we could ignore them. IOW their presence (in the input) does not mean they are _all_ dupes in generated code (output). However dupes in manual bindings were also (forced to be) _ignored_ and those were part of the assemblies we ship. Again it was not a big deal, nor a source of much extra metadata/size, so it was ignored. Forward to today, manual bindings needs to be updated for net6 [1] so any extra we have requires more (manual) work. Cleaning this problem up in the generator code reduce the (manual) work we need to do. It also means introspection can detect dupes (in generated and manual code) so we don't end up adding more (which would also require more manual work to support both set of attributes). [1] xamarin#10580
❌ Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View API diffTest results2 tests failed, 180 tests passed.Failed tests
Pipeline on Agent XAMBOT-1095' |
Mostly for dotnet, few for watchOS 2.0. Only API newer than the minimum supported OS needs to be decorated.
❌ Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View API diffTest results6 tests failed, 176 tests passed.Failed tests
Pipeline on Agent XAMBOT-1094' |
Tracking with xamarin#11029
❌ Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View API diffTest results3 tests failed, 179 tests passed.Failed tests
Pipeline on Agent XAMBOT-1099' |
added 2 commits
March 30, 2021 21:00
…until their complete removal
❌ Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View API diffTest results3 tests failed, 179 tests passed.Failed tests
Pipeline on Agent XAMBOT-1101' |
added 2 commits
March 31, 2021 09:57
and, without it, introspection (on dotnet [1]) will try to run tests on it Quote from `NSMenuView.h` ```objc ``` [1] the new attribute do not have anythings similar to the legacy `PlatformArchitecture` enum
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
dalexsoto
approved these changes
Mar 31, 2021
❌ Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View API diffTest results2 tests failed, 96 tests passed.Failed tests
Pipeline on Agent XAMBOT-1106' |
❌ Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View API diffTest results1 tests failed, 97 tests passed.Failed tests
Pipeline on Agent XAMBOT-1104.BigSur |
|
Known issue unrelated to PR https://github.com/xamarin/maccore/issues/2411 |
This was referenced Aug 5, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This moves our current/legacy attributes to the ones added in dotnet 5 [1].
Short Forms (only in bindings)
Long Forms
Other changes
[SupportedOSPlatform]and[UnsupportedOSPlatform]are not allowed oninterface[2] which means they cannot be used for protocols. This is currently handled by inlining the existing attributes on all members.[ObsoletedInOSPlatform]was removed in net5 RC. This PR is now mapping the existing attributes to[Obsolete], however multiple ones cannot be added so they need to be platform specific.Tasks
Manual bindings-> [introspection][dotnet] Enable LegacyAttributes test #11055Introspection tests updatesReferences
SupportedOSPlatformAttributeon interfaces dotnet/runtime#47599ObsoletedInOSPlatformAttributefor net6 dotnet/runtime#47601