[NET Attribute Conversion] Rerun with fixes to catch old/unlisted availability and type information. Enable one test. #14287
Conversation
chamons
requested review from
rolfbjarne,
dalexsoto and
mandel-macaque
as code owners
chamons
changed the title
❌ [PR Build] Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View dotnet API diffView dotnet legacy API diffView dotnet iOS-MacCatalayst API diff
API Current PR diffℹ️ API Diff (from PR only) (please review changes) View dotnet API diffView dotnet legacy API diffView dotnet iOS-MacCatalayst API diff
GitHub pagesResults can be found in the following github pages (it might take some time to publish): Test results54 tests failed, 92 tests passed.Failed tests
Pipeline on Agent XAMBOT-1098.BigSur' |
| @@ -29,15 +29,6 @@ | |||
| !missing-release-attribute-on-return-value! ModelIO.IMDLMeshBufferZone ModelIO.MDLMeshBufferAllocatorWrapper::CreateZone(System.UIntPtr)'s selector's ('newZone:') Objective-C method family ('new') indicates that the native method returns a retained object, and as such a '[return: Release]' attribute is required. | |||
| !missing-release-attribute-on-return-value! ModelIO.IMDLMeshBufferZone ModelIO.MDLMeshBufferDataAllocator::CreateZone(Foundation.NSNumber[],Foundation.NSNumber[])'s selector's ('newZoneForBuffersWithSize:andType:') Objective-C method family ('new') indicates that the native method returns a retained object, and as such a '[return: Release]' attribute is required. | |||
| !missing-release-attribute-on-return-value! ModelIO.IMDLMeshBufferZone ModelIO.MDLMeshBufferDataAllocator::CreateZone(System.UIntPtr)'s selector's ('newZone:') Objective-C method family ('new') indicates that the native method returns a retained object, and as such a '[return: Release]' attribute is required. | |||
| !missing-release-attribute-on-return-value! ModelIO.MDLMesh ModelIO.MDLMesh::CreateCapsule(System.Single,System.Numerics.Vector2,System.UIntPtr,System.UIntPtr,System.UIntPtr,ModelIO.MDLGeometryType,System.Boolean,ModelIO.IMDLMeshBufferAllocator)'s selector's ('newCapsuleWithHeight:radii:radialSegments:verticalSegments:hemisphereSegments:geometryType:inwardNormals:allocator:') Objective-C method family ('new') indicates that the native method returns a retained object, and as such a '[return: Release]' attribute is required. | |||
There was a problem hiding this comment.
This looks fishy, I don't see why adding availability attributes would cause xtro to stop complaining like this (the other !extra-enum-value! removals in other files make sense, but not these)
src/WebKit/WebKit.cs
Outdated
|
|
||
| #if NET | ||
| [SupportedOSPlatform ("macos")] | ||
| [UnsupportedOSPlatform ("macos10.14")] | ||
| #if MONOMAC | ||
| [Obsolete ("Starting with macos10.14 no longer supported.", DiagnosticId = "BI1234", UrlFormat = "https://github.com/xamarin/xamarin-macios/wiki/Obsolete")] | ||
| #endif | ||
| #endif |
There was a problem hiding this comment.
Wouldn't the generated part of this partial class have these attributes?
There was a problem hiding this comment.
Is this a question about this specific instance or the PR in general?
Right now, the generated code only has:
[Register("WebFrame", true)]
[UnsupportedOSPlatform ("macos10.14")]
public unsafe partial class WebFrame : NSObject {
but it should match this when I finish the generator PR.
Part of my question on Teams (that I referenced in the description) was if we should "duplicate" attributes on partial class definitions.
There was a problem hiding this comment.
Is this a question about this specific instance or the PR in general?
In general
Part of my question on Teams (that I referenced in the description) was if we should "duplicate" attributes on partial class definitions.
I misunderstood then, having two sources of truth (the api definition and the manual code), sounds like it'll diverge at some point, and cause problems.
In any case, the point is somewhat moot, because for this particular case once you fix the generator, you'll generate an Obsolete attribute, which means there will be two Obsolete attributes on the type, which is not allowed:
error CS0579: Duplicate 'Obsolete' attribute
There was a problem hiding this comment.
Ok, that changes things. I'll come up with a plan after lunch, likely just reverting commits by hand this one time.
There was a problem hiding this comment.
I have a local patch that removes all changes on public partial classes building locally.
Thanks patch iron and past-me for writing it to make this an hour-ish job instead of many hours.
|
I had to download it because Safari freaked out heh, and won't let me even approve it but other than what Rolf mentioned above is looking good to me. |
❌ [PR Build] Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View dotnet API diffView dotnet legacy API diffView dotnet iOS-MacCatalayst API diff
API Current PR diffℹ️ API Diff (from PR only) (please review changes) View dotnet API diffView dotnet legacy API diffView dotnet iOS-MacCatalayst API diff
GitHub pagesResults can be found in the following github pages (it might take some time to publish): Test results5 tests failed, 141 tests passed.Failed tests
Pipeline on Agent XAMBOT-1100.BigSur' |
❌ [PR Build] Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable View dotnet API diffView dotnet legacy API diffView dotnet iOS-MacCatalayst API diff
API Current PR diffℹ️ API Diff (from PR only) (please review changes) View dotnet API diffView dotnet legacy API diffView dotnet iOS-MacCatalayst API diff
GitHub pagesResults can be found in the following github pages (it might take some time to publish): Test results5 tests failed, 141 tests passed.Failed tests
Pipeline on Agent XAMBOT-1094.BigSur' |
|
The second new test (test #4) is being punted to the generator PR. It's written, but fails to a number of things that I think are fixed there. |
Co-authored-by: Rolf Bjarne Kvinge <rolf@xamarin.com>
✅ [PR Build] Tests passed on Build. ✅Tests passed on Build. API diff✅ API Diff from stable View dotnet API diffView dotnet legacy API diffView dotnet iOS-MacCatalayst API diff
API Current PR diffℹ️ API Diff (from PR only) (please review changes) View dotnet API diffView dotnet legacy API diffView dotnet iOS-MacCatalayst API diff
GitHub pagesResults can be found in the following github pages (it might take some time to publish): 🎉 All 146 tests passed 🎉Pipeline on Agent XAMBOT-1108.Monterey' |
|
This work looks ready to go in, however we're going to wait for a merge against main and possible fixes from @mandel-macaque since the test changes yesterday likely destabilized all of the cecil tests. |
🔥 Tests failed catastrophically on VSTS: simulator tests iOS (no summary found). 🔥Result file D:\a\1\s\Reports\TestSummary-simulator\TestSummary.md not found. |
🔥 Tests failed catastrophically on VSTS: simulator tests iOS (no summary found). 🔥Result file D:\a\1\s\Reports\TestSummary-simulator\TestSummary.md not found. |
❌ [CI Build] Tests failed on VSTS: simulator tests iOS ❌Tests failed on VSTS: simulator tests iOS. Test results21 tests failed, 125 tests passed.Failed tests
Pipeline on Agent XAMBOT-1103.Monterey |
❌ [CI Build] Tests failed on VSTS: simulator tests iOS ❌Tests failed on VSTS: simulator tests iOS. Test results3 tests failed, 143 tests passed.Failed tests
Pipeline on Agent XAMBOT-1107.Monterey |
|
Still trying to get a clean build. The 3 test failures look unrelated. |
|
It's been a month since this started, and there were a bunch of conflicts now. I'm going to push a new 3rd try in a few minutes.... |
A few mel commits of note include:
add-default-introducedthat will detect when an API is available on a given platform without availability attributes and create a default "non-versioned" one.Technically many of these attributes on types are not strictly required, as the generator (after another change post this) will generate a copy. However, there are some cases where it was required, and overall the team in discusses liked this as it removed an very non-obvious footgun inherit in the NET6 attribute behavior:
Buckle up, this PR is large.
Things to look for (Mistakes I've made in other places here and caught):