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

[Foundation] Add bindings for Xcode 13 rc 1. #13328

Closed

Conversation

mandel-macaque
Copy link
Member

No description provided.

@mandel-macaque mandel-macaque added the note-highlight Worth calling out specifically in release notes label Nov 9, 2021
src/Foundation/Enums.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@chamons chamons left a comment

Choose a reason for hiding this comment

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

Looks reasonable. There are a lot of tests, but I'm not sure enough given how much manual bindings/wraps there were.

I want @rolfbjarne 's blessing before this goes in.

Good work, this was messy.

src/foundation.cs Outdated Show resolved Hide resolved
@@ -1785,7 +1785,7 @@ public TrampolineInfo MakeTrampoline (Type t)
}

var rt = mi.ReturnType;
var rts = IsNativeEnum (rt) ? "var" : rt.ToString ();
Copy link
Contributor

Choose a reason for hiding this comment

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

@mandel-macaque - Can you explain this bit?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, without this fix you will get an issue with the trampolines that use generics because you get the class name like NSArra'1 rather than NSArray

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally this would be in a separate PR to make blaming it in the future more obvious.

Consider calling it out with explaination in the PR/commit description for possible future us.

Copy link
Member Author

Choose a reason for hiding this comment

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

This can be a diff fix. The issue where is that the rt.ToString() ois doing the wrong thing and is generating a non-compilable name. I'll add this as a separate branch with a decent explanation.

@@ -394,7 +394,7 @@
<Exec Command="xcrun -sdk $(_SmeltingSdk) metal -c @(CustomMetalSmeltingInput) -o $(IntermediateOutputPath)\fragmentShader.air $(_SmeltingMinOS)" EnvironmentVariables="DEVELOPER_DIR=$(_SdkDevPath)" />
<Exec Command="xcrun -sdk $(_SmeltingSdk) metallib $(IntermediateOutputPath)\fragmentShader.air -o $(AppBundleDir)\fragmentShader.metallib" EnvironmentVariables="DEVELOPER_DIR=$(_SdkDevPath)" />
</Target>
<Target Name="BeforeBuild" Inputs="@(GeneratedTestInput)" Outputs="@(GeneratedTestOutput)" DependsOnTargets="CustomMetalSmelting" >
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems unrelated to PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

IDE being stupid.

Copy link
Contributor

@tj-devel709 tj-devel709 left a comment

Choose a reason for hiding this comment

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

Just some small things I noticed!

src/Foundation/Enums.cs Outdated Show resolved Hide resolved
@@ -3569,7 +3668,10 @@ interface NSMutableArray {
[iOS (8,0), Mac(10,10)]
[Static, Export ("arrayWithContentsOfURL:")]
NSMutableArray FromUrl (NSUrl url);

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Extra line removed

Copy link
Member Author

Choose a reason for hiding this comment

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

It was not removed. Someone left a tab and is added in the diff.

src/foundation.cs Outdated Show resolved Hide resolved
tests/monotouch-test/Foundation/NSArray1Test.cs Outdated Show resolved Hide resolved
TestRuntime.AssertXcodeVersion (13,0);

var str = new NSString ("Test");
var change = NSOrderedCollectionChange<NSString>.ChangeWithObject(str, NSCollectionChangeType.Insert, 0, 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
var change = NSOrderedCollectionChange<NSString>.ChangeWithObject(str, NSCollectionChangeType.Insert, 0, 1);
var change = NSOrderedCollectionChange<NSString>.ChangeWithObject (str, NSCollectionChangeType.Insert, 0, 1);
```nit

Co-authored-by: TJ Lambert <50846373+tj-devel709@users.noreply.github.com>
Co-authored-by: Chris Hamons <chris.hamons@xamarin.com>
Copy link
Member

@dalexsoto dalexsoto left a comment

Choose a reason for hiding this comment

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

Nice! 👍++ for the generator fix and the tests!

@vs-mobiletools-engineering-service2
Copy link
Collaborator

❌ [PR Build] Tests failed on Build ❌

Tests failed on Build.

API diff

✅ API Diff from stable

View API diff
View dotnet API diff
View dotnet legacy API diff
View dotnet iOS-MacCatalayst API diff

API & Generator diff

ℹ️ API Diff (from PR only) (please review changes)
ℹ️ Generator Diff (please review changes)

GitHub pages

Results can be found in the following github pages (it might take some time to publish):

Test results

11 tests failed, 90 tests passed.

Failed tests

  • introspection/Mac Catalyst [dotnet]/Debug [dotnet]: Failed (Tests run: 44 Passed: 41 Inconclusive: 0 Failed: 1 Ignored: 2)
  • introspection/iOS Unified 64-bits - simulator/Debug: Failed
  • introspection/tvOS - simulator/Debug: Failed
  • introspection/watchOS 32-bits - simulator/Debug: Failed
  • monotouch-test/Mac [dotnet]/Debug [dotnet]: Failed (Test run failed.
    Tests run: 2569 Passed: 2415 Inconclusive: 10 Failed: 1 Ignored: 153)
  • monotouch-test/Mac Catalyst [dotnet]/Debug [dotnet]: Failed (Tests run: 2734 Passed: 2502 Inconclusive: 35 Failed: 1 Ignored: 231)
  • monotouch-test/iOS Unified 64-bits - simulator/Release (all optimizations): Failed
  • monotouch-test/tvOS - simulator/Release (all optimizations): Failed
  • xammac tests/Mac Modern/Debug: Failed (Test run failed.
    Tests run: 2575 Passed: 2425 Inconclusive: 10 Failed: 1 Ignored: 149)
  • xammac tests/Mac Modern/Release: Failed (Test run failed.
    Tests run: 2572 Passed: 2421 Inconclusive: 10 Failed: 1 Ignored: 150)
  • xammac tests/Mac Modern/Release (all optimizations): Failed (Test run failed.
    Tests run: 2572 Passed: 2420 Inconclusive: 10 Failed: 1 Ignored: 151)

Pipeline on Agent XAMBOT-1104.BigSur'
Merge ea91c10 into f0ff01f

return first.ToString ().Equals (second.ToString ());
});
}, "Not throws");
Assert.NotNull (diff, "Not null");
Copy link
Member

Choose a reason for hiding this comment

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

Maybe assert that we get a specific diff result as well?

var changes = new NSOrderedCollectionChange<NSString> [] { change };
using var diff = new NSOrderedCollectionDifference<NSString> (changes);
Assert.DoesNotThrow (() => {
var i = diff.Insertions;
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for not asserting on the returned value?

Assert.AreEqual (1, diff.Insertions.Length, "insertions");

(or whatever it's supposed to return)

var changes = new NSOrderedCollectionChange [] { change };
using var diff = new NSOrderedCollectionDifference (changes);
Assert.DoesNotThrow (() => {
var i = diff.Insertions;
Copy link
Member

Choose a reason for hiding this comment

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

Same: assert on the returned value

[SupportedOSPlatform ("ios13.0"), SupportedOSPlatform ("tvos13.0"), SupportedOSPlatform ("macos10.15")]
#endif
public void ApplyDifference (NSOrderedCollectionDifference<TKey> difference)
=> _ApplyDifference (difference.Handle);
Copy link
Member

Choose a reason for hiding this comment

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

NRE if difference is null

src/Foundation/NSOrderedCollectionChange.cs Outdated Show resolved Hide resolved
src/Foundation/NSOrderedSet_1.cs Outdated Show resolved Hide resolved
[Watch (6,0), TV (13,0), Mac (10,15), iOS (13,0)]
[Static]
[Export ("stringFromMeasurement:countStyle:")]
string StringFromMeasurement (NSUnitInformationStorage measurement, NSByteCountFormatterCountStyle countStyle);
Copy link
Member

Choose a reason for hiding this comment

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

This needs a verb:

Suggested change
string StringFromMeasurement (NSUnitInformationStorage measurement, NSByteCountFormatterCountStyle countStyle);
string Create (NSUnitInformationStorage measurement, NSByteCountFormatterCountStyle countStyle);

src/foundation.cs Outdated Show resolved Hide resolved
@@ -7278,6 +7393,10 @@ partial interface NSUrlSessionTask : NSCopying, NSProgressReporting
[Export ("countOfBytesClientExpectsToReceive")]
long CountOfBytesClientExpectsToReceive { get; set; }

[Watch (8, 0), TV (15, 0), Mac (12, 0), iOS (15, 0), MacCatalyst (15,0)]
[NullAllowed, Export ("delegate", ArgumentSemantic.Retain)]
NSObject WeakDelegate { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

Is there a strongly typed Delegate property?

src/foundation.cs Outdated Show resolved Hide resolved
Co-authored-by: Rolf Bjarne Kvinge <rolf@xamarin.com>
@mandel-macaque
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Pull request contains merge conflicts.

@dalexsoto
Copy link
Member

@mandel-macaque is this still a thing? we may want to land it before @chamons converts attributes on Foundation

chamons pushed a commit to chamons/xamarin-macios that referenced this pull request Jul 22, 2022
chamons added a commit to chamons/xamarin-macios that referenced this pull request Jul 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
note-highlight Worth calling out specifically in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants