[generator] Do not zero-extend implied catalyst attributes #15648
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
- In the [Xcode 14 Photo PR](xamarin#15608) a test is failing with this: ``` ILLINK : error MT2362: The linker step 'Registrar' failed during processing: One or more errors occurred. (The type 'Photos.PHPersistentObjectChangeDetails' (used as a return type in Photos.PHPersistentChange.ChangeDetails) is not available in MacCatalyst 16.0 (it was introduced in MacCatalyst 16.0.0). Please build with a newer MacCatalyst SDK (usually done by using the most recent version of Xcode). [/Users/donblas/Programming/xamarin-macios/tests/dotnet/MySimpleApp/MacCatalyst/MySimpleApp.csproj] ) (The type 'Photos.PHObjectType' (used as a parameter in Photos.PHPersistentChange.ChangeDetails) is not available in MacCatalyst 16.0 (it was introduced in MacCatalyst 16.0.0). Please build with a newer MacCatalyst SDK (usually done by using the most recent version of Xcode). ) (The type 'Photos.PHPersistentChangeFetchResult' (used as a return type in Photos.PHPhotoLibrary.FetchPersistentChanges) is not available in MacCatalyst 16.0 (it was introduced in MacCatalyst 16.0.0). Please build with a newer MacCatalyst SDK (usually done by using the most recent version of Xcode). ) (The type 'Photos.PHPersistentChangeToken' (used as a parameter in Photos.PHPhotoLibrary.FetchPersistentChanges) is not available in MacCatalyst 16.0 (it was introduced in MacCatalyst 16.0.0). Please build with a newer MacCatalyst SDK (usually done by using the most recent version of Xcode). ``` The details of how we fail are written up in [this issue](xamarin#15643) but since sharpie never outputs versions in the form of x.y.z where .z is zero we only hit this with generated attributes. Because of this fact, we can work around it with a generator change. This commit changes how we "imply" attributes from iOS to Catalyst. As a brief reminder, because of historical bindings we assume anything that has iOS and not a Catalyst really means "treat iOS as if it was also Catalyst". This work is done in `AddImpliedCatalyst` and uses `CloneFromOtherPlatform` to make a copy of an attribute, because there is no easy way to say "I want a copy of this, but with this other platform". `CloneFromOtherPlatform` used to always call the 3 version (Major, Minor, Revision) constructor, even when the attribute being cloned only used Major.Minor. However, this caused us to "zero extend" the version with another zero, which triggers this bug, so stop doing that. Uglier code in the generator, but it works better.
|
This needs to be cherry-picked to xcode14 after landing to unblock #15608. |
dalexsoto
approved these changes
Aug 5, 2022
mandel-macaque
approved these changes
Aug 5, 2022
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Oooops, I think I clicked the wrong PR. |
🔥 [PR Build] Build failed 🔥Build failed for the job 'Detect API changes' Pipeline on Agent |
|
🔥 Unable to find the contents for the comment: D:\a\1\s\change-detection\results\gh-comment.md does not exist :fire Pipeline on Agent |
|
mac_binding_project passes locally for me. |
|
/sudo backport xcode14 |
|
Backport Job to branch xcode14 Created! The magic is happening here |
|
Hooray! Backport succeeded! Please see https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=6527798 for more details. |
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.
The details of how we fail are written up in this issue but since sharpie never outputs versions in the form of x.y.z where .z is zero we only hit this with generated attributes.
Because of this fact, we can work around it with a generator change.
This commit changes how we "imply" attributes from iOS to Catalyst. As a brief reminder, because of historical bindings we assume anything that has iOS and not a Catalyst really means "treat iOS as if it was also Catalyst".
This work is done in
AddImpliedCatalystand usesCloneFromOtherPlatformto make a copy of an attribute, because there is no easy way to say "I want a copy of this, but with this other platform".CloneFromOtherPlatformused to always call the 3 version (Major, Minor, Revision) constructor, even when the attribute being cloned only used Major.Minor.However, this caused us to "zero extend" the version with another zero, which triggers this bug, so stop doing that. Uglier code in the generator, but it works better.