[AudioToolbox] Update API to xcode 12 beta 6. #9603

mandel-macaque · 2020-09-08T23:25:42Z

The most important changes in the API are ignored until we fix issue #9602

The most important changes in the API are ignored until we fix issue xamarin#9602

monojenkins · 2020-09-08T23:35:16Z

Build failure
❌ Build failed or was aborted

monojenkins · 2020-09-09T00:54:34Z

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

spouliot · 2020-09-09T01:15:43Z

src/AudioUnit/AudioComponent.cs

@@ -342,6 +351,13 @@ public UIKit.UIImage GetIcon (float desiredPointSize)
 				return AudioComponentGetLastActiveTime (handle);
 			}
 		}
+
+		[Watch (7,0), TV (14,0), iOS (14,0)]
+		public UIImage CopyIcon () {


minor

style, { on the next line

maybe GetIcon since we don't have to follow the ObjC copy rule/convention

We already have a GetIcon, either we ignore the method of we use Copy

spouliot · 2020-09-09T01:17:10Z

src/AudioUnit/AudioComponent.cs

+		public AppKit.NSImage CopyIcon () { 
+			var ptr = AudioComponentCopyIcon (handle);
+			return ptr == IntPtr.Zero ? null : new AppKit.NSImage (ptr);
+		}


you can merge both method if you add using NSImage = UIKit.UIImage; in the #if MONOMAC on top of the file

There is some extra ugliness, but sure, can be done.

spouliot · 2020-09-09T01:21:45Z

src/AudioUnit/AudioComponent.cs

@@ -342,6 +351,13 @@ public UIKit.UIImage GetIcon (float desiredPointSize)
 				return AudioComponentGetLastActiveTime (handle);
 			}
 		}
+
+		[Watch (7,0), TV (14,0), iOS (14,0)]


API diff for watchOS is empty so this code is not included in the platform assembly
Either it's available and should be added or the [Watch (7,0)] attribute should be removed (here and elsewhere).
Considering that there was no xtro file updated for the watchOS profile I suspect it's the former.

rolfbjarne · 2020-09-09T07:02:01Z

src/AudioUnit/AudioComponent.cs

+		[Watch (7,0), TV (14,0), iOS (14,0)]
+		public UIImage CopyIcon () {
+			var ptr = AudioComponentCopyIcon (handle);
+			return ptr == IntPtr.Zero ? null : new UIImage (ptr);


AudioComponentCopyIcon returns a retained handle, and it can also be an existing UIImage (for which we have a managed instance), so:

Suggested change

return ptr == IntPtr.Zero ? null : new UIImage (ptr);

return (UIImage) Runtime.GetNSObject (ptr, true);

rolfbjarne · 2020-09-09T07:03:12Z

And most importantly: manual API need manual tests!

dalexsoto

After feedback is addressed 👍

monojenkins · 2020-09-09T22:08:29Z

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

Test results

3 tests failed, 73 tests passed.

Failed tests

xammac tests/Mac Modern/Debug: Failed (Test run failed.)
xammac tests/Mac Modern/Release: Failed (Test run failed.)
xammac tests/Mac Modern/Release: Failed (Test run failed.)

mandel-macaque · 2020-09-09T22:33:40Z

Failures are unrelated, but lets kick it again (that msbuild issue is starting to happy more often).

mandel-macaque · 2020-09-09T22:33:44Z

build

monojenkins · 2020-09-10T00:20:44Z

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

Test results

3 tests failed, 73 tests passed.

Failed tests

xammac tests/Mac Modern/Debug: Failed (Test run failed.)
xammac tests/Mac Modern/Release: Failed (Test run failed.)
xammac tests/Mac Modern/Release: Failed (Test run failed.)

rolfbjarne · 2020-09-10T06:41:11Z

The failures are very much related:

1) CopyIconTest (MonoTouchFixtures.AudioUnit.AudioUnitTest.CopyIconTest)
     Expected: No Exception to be thrown
  But was:   (AudioComponentCopyIcon assembly:<unknown assembly> type:<unknown type> member:(null))
  at (wrapper managed-to-native) AudioUnit.AudioComponent.AudioComponentCopyIcon(intptr)
  at AudioUnit.AudioComponent.CopyIcon () [0x00001] in /Library/Frameworks/Xamarin.Mac.framework/Versions/6.99.0.255/src/Xamarin.Mac/AudioUnit/AudioComponent.cs:339 
  at MonoTouchFixtures.AudioUnit.AudioUnitTest+<>c__DisplayClass2_0.<CopyIconTest>b__0 () [0x00001] in /Users/builder/jenkins/workspace/xamarin-macios-pr-builder/tests/monotouch-test/AudioUnit/AudioUnitTest.cs:67 
  at NUnit.Framework.Constraints.VoidInvocationDescriptor.Invoke () [0x00001] in /Users/builder/jenkins/workspace/xamarin-macios-pr-builder/external/guiunit/src/framework/Constraints/ThrowsConstraint.cs:208 
  at NUnit.Framework.Constraints.ExceptionInterceptor.Intercept (System.Object invocation) [0x00050] in /Users/builder/jenkins/workspace/xamarin-macios-pr-builder/external/guiunit/src/framework/Constraints/ThrowsConstraint.cs:163 
  at MonoTouchFixtures.AudioUnit.AudioUnitTest.CopyIconTest () [0x00042] in /Users/builder/jenkins/workspace/xamarin-macios-pr-builder/tests/monotouch-test/AudioUnit/AudioUnitTest.cs:66 
  at (wrapper managed-to-native) System.Reflection.RuntimeMethodInfo.InternalInvoke(System.Reflection.RuntimeMethodInfo,object,object[],System.Exception&)
  at System.Reflection.RuntimeMethodInfo.Invoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x0006a] in /Library/Frameworks/Xamarin.Mac.framework/Versions/Current/src/Xamarin.Mac/mcs/class/corlib/System.Reflection/RuntimeMethodInfo.cs:395

rolfbjarne · 2020-09-10T06:42:17Z

tests/monotouch-test/AudioUnit/AudioUnitTest.cs

+		[Test]
+		public void CopyIconTest ()
+		{ 
+			AudioComponentDescription cd = new AudioComponentDescription () {


You need a version check here to only run on Xcode 12+ OSes.

rolfbjarne · 2020-09-10T06:42:43Z

tests/monotouch-test/AudioUnit/AudioUnitTest.cs

+			Assert.DoesNotThrow ( () => {
+				var icon = component.CopyIcon (); // ensuring that the manual binding does not throw, we do not care about the result
+			});


You could do something like this:

Assert.IsNotNull (component.CopyIcon (), "CopyIcon");

It can be null, there is no guarantee, I much prefer to make sure it does not crash.

rolfbjarne · 2020-09-10T06:42:51Z

src/AudioUnit/AudioComponent.cs

@@ -342,6 +361,7 @@ public UIKit.UIImage GetIcon (float desiredPointSize)
 				return AudioComponentGetLastActiveTime (handle);
 			}
 		}
+


Whitespace!

rolfbjarne · 2020-09-10T06:43:01Z

src/AudioUnit/AudioComponent.cs

@@ -353,6 +373,7 @@ public AppKit.NSImage GetIcon ()
 		{
 			return new AppKit.NSImage (AudioComponentGetIcon (handle));
 		}
+


And more whitespace 😄

monojenkins · 2020-09-10T19:06:43Z

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

[AudioToolbox] Update API to xcode 12 beta 6.

86fd09b

The most important changes in the API are ignored until we fix issue xamarin#9602

mandel-macaque requested review from dalexsoto and spouliot as code owners September 8, 2020 23:25

Add missing comma.

1d0cb82

spouliot suggested changes Sep 9, 2020

View reviewed changes

rolfbjarne requested changes Sep 9, 2020

View reviewed changes

dalexsoto approved these changes Sep 9, 2020

View reviewed changes

whitneyschmidt approved these changes Sep 9, 2020

View reviewed changes

Address reviews. Add missing test.

4761cf6

mandel-macaque added the note-highlight Worth calling out specifically in release notes label Sep 9, 2020

rolfbjarne requested changes Sep 10, 2020

View reviewed changes

spouliot approved these changes Sep 10, 2020

View reviewed changes

Address reviews.

6220c08

rolfbjarne approved these changes Sep 11, 2020

View reviewed changes

mandel-macaque merged commit 5a9139b into xamarin:xcode12 Sep 11, 2020

mandel-macaque deleted the audiotools-xcode12-beta-4 branch September 11, 2020 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AudioToolbox] Update API to xcode 12 beta 6. #9603

[AudioToolbox] Update API to xcode 12 beta 6. #9603

mandel-macaque commented Sep 8, 2020

monojenkins commented Sep 8, 2020

monojenkins commented Sep 9, 2020

spouliot Sep 9, 2020

mandel-macaque Sep 9, 2020

spouliot Sep 9, 2020

mandel-macaque Sep 9, 2020

spouliot Sep 9, 2020

rolfbjarne Sep 9, 2020

rolfbjarne commented Sep 9, 2020

dalexsoto left a comment

monojenkins commented Sep 9, 2020

Failed tests

mandel-macaque commented Sep 9, 2020

mandel-macaque commented Sep 9, 2020

monojenkins commented Sep 10, 2020

Failed tests

rolfbjarne commented Sep 10, 2020

rolfbjarne Sep 10, 2020

rolfbjarne Sep 10, 2020

mandel-macaque Sep 10, 2020

rolfbjarne Sep 10, 2020

rolfbjarne Sep 10, 2020

monojenkins commented Sep 10, 2020

	return ptr == IntPtr.Zero ? null : new UIImage (ptr);
	return (UIImage) Runtime.GetNSObject (ptr, true);

[AudioToolbox] Update API to xcode 12 beta 6. #9603

[AudioToolbox] Update API to xcode 12 beta 6. #9603

Conversation

mandel-macaque commented Sep 8, 2020

monojenkins commented Sep 8, 2020

monojenkins commented Sep 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rolfbjarne commented Sep 9, 2020

dalexsoto left a comment

Choose a reason for hiding this comment

monojenkins commented Sep 9, 2020

Test results

Failed tests

mandel-macaque commented Sep 9, 2020

mandel-macaque commented Sep 9, 2020

monojenkins commented Sep 10, 2020

Test results

Failed tests

rolfbjarne commented Sep 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monojenkins commented Sep 10, 2020