NotificationsYou must be signed in to change notification settings
Fork548
Star2.8k

[SoundAnalysis] Add Xcode 11 Beta 1 & Beta 2 bindings#6351

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

dalexsoto merged 8 commits intodotnet:xcode11fromtj-devel709:TJ-xcode11-SoundAnalysis

Jun 21, 2019

Merged

[SoundAnalysis] Add Xcode 11 Beta 1 & Beta 2 bindings#6351

dalexsoto merged 8 commits intodotnet:xcode11fromtj-devel709:TJ-xcode11-SoundAnalysis

Jun 21, 2019

Conversation

Copy link

Member

tj-devel709 commentedJun 18, 2019

Intro errors:
[FAIL] Could not load SoundAnalysis.SNClassificationResult
[FAIL] Could not load SoundAnalysis.SNClassifySoundRequest

tj-devel709 added4 commits

June 17, 2019 17:09

adding Sound Analysis

46cbca6

set up error domain and added CMTimeRange for all platforms except watch

1077416

removed whitespaces

8399dfd

removed more whitespaces

a0be501

tj-devel709 requested review fromchamons,rolfbjarne andspouliot ascode owners

June 18, 2019 15:41

Copy link

dnfclas commentedJun 18, 2019•
edited
Loading

All CLA requirements met.

Copy link

Member

dalexsoto commentedJun 18, 2019

build

dalexsoto added this to thexcode11 milestone

Jun 18, 2019

VincentDondain changed the title~~[SoundAnalysis] Add Xcode 9 Beta 1 & Beta 2 bindings~~[SoundAnalysis] Add Xcode 11 Beta 1 & Beta 2 bindings

Jun 18, 2019

dalexsoto added the do-not-mergeDo not merge this pull request label

Jun 18, 2019

Copy link

Member

dalexsoto commentedJun 18, 2019

This can't be merged until#6341 lands!

dalexsoto requested changes

Jun 18, 2019

View reviewed changes

src/soundanalysis.cs OutdatedShow resolvedHide resolved

spouliot suggested changes

Jun 18, 2019

View reviewed changes

Copy link

Contributor

spouliot left a comment

Choose a reason for hiding this comment

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

🎉

src/soundanalysis.cs OutdatedShow resolvedHide resolved

rolfbjarne requested changes

Jun 18, 2019

View reviewed changes

Copy link

Member

rolfbjarne left a comment

Choose a reason for hiding this comment

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

This looks very good, just a few nitpicks from me.

src/Constants.iOS.cs.inShow resolvedHide resolved

src/Constants.tvos.cs.inShow resolvedHide resolved

src/Constants.watch.cs.inShow resolvedHide resolved

src/soundanalysis.cs OutdatedShow resolvedHide resolved

tools/common/Frameworks.csShow resolvedHide resolved

Copy link

Contributor

spouliot commentedJun 18, 2019

[FAIL] Could not load SoundAnalysis.SNClassificationResult[FAIL] Could not load SoundAnalysis.SNClassifySoundRequest

^ intro, but on which platform(s) ?

VincentDondain suggested changes

Jun 18, 2019

View reviewed changes

Copy link

Contributor

VincentDondain left a comment

Choose a reason for hiding this comment

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

Some minor comments, looking good otherwise 👍

src/soundanalysis.cs OutdatedShow resolvedHide resolved

Copy link

Contributor

stephen-hawley commentedJun 18, 2019

👍

Copy link

Contributor

spouliot commentedJun 18, 2019

Also xtrospection tests will fail since the API are still in the.todo files. You need to remove what you implemented (runmake intests/xtro-sharpie)

edited framework files and constants. Changed header names and added …

6d7ef20

…todo files

mandel-macaque suggested changes

Jun 18, 2019

View reviewed changes

src/soundanalysis.csShow resolvedHide resolved

Copy link

MemberAuthor

tj-devel709 commentedJun 18, 2019

@spouliot @dalexsoto @stephen-hawley @VincentDondain @rolfbjarne @mandel-macaque @chamons Think I hit those edits and repushed!

Copy link

Collaborator

monojenkins commentedJun 18, 2019

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

spouliot approved these changes

Jun 18, 2019

View reviewed changes

Copy link

Contributor

spouliot left a comment

Choose a reason for hiding this comment

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

👍

dalexsoto requested review fromVincentDondain,dalexsoto,mandel-macaque androlfbjarne

June 18, 2019 17:58

Apply feedback

974ba52

VincentDondain approved these changes

Jun 18, 2019

View reviewed changes

Copy link

Contributor

VincentDondain left a comment

Choose a reason for hiding this comment

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

Nice changes 👍

Copy link

Member

dalexsoto commentedJun 18, 2019

whitelist tj-devel709

Copy link

Member

dalexsoto commentedJun 18, 2019

build

whitneyschmidt self-requested a review

June 18, 2019 20:02

whitneyschmidt approved these changes

Jun 18, 2019

View reviewed changes

rolfbjarne approved these changes

Jun 18, 2019

View reviewed changes

Copy link

Member

rolfbjarne left a comment

Choose a reason for hiding this comment

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

Great work, 👍

mandel-macaque approved these changes

Jun 18, 2019

View reviewed changes

Copy link

Collaborator

monojenkins commentedJun 18, 2019

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

Test results

7 tests failed, 87 tests passed.

Failed tests

xammac tests/Mac Unified/Release (all optimizations): Failed (Test run crashed (exit code: 134).)
mono-native-compat/Mac Unified/Modern: Failed (Test run crashed (exit code: 134).)
mono-native-unified/Mac Unified/Modern: Failed (Test run crashed (exit code: 134).)
introspection/iOS Unified 64-bits - simulator/Debug: Failed
MSBuild tests/iOS: Failed (Execution failed with exit code 1)
mmptest/macOS/NonSystem: Failed (Execution failed with exit code 3)
MTouch tests/NUnit: Failed (Execution failed with exit code 48)

mandel-macaque removed the do-not-mergeDo not merge this pull request label

Jun 19, 2019

Copy link

Contributor

spouliot commentedJun 19, 2019

wrt

[FAIL] Could not load SoundAnalysis.SNClassificationResult[FAIL] Could not load SoundAnalysis.SNClassifySoundRequest

you should test conformance in a small Xcode/ObjC application. it's possible it's defined (in headers) but notyet conforming. In that case filing an issue (with Apple) and linking it from our "todo before release" issue would be the way to go :)

Copy link

Contributor

spouliot commentedJun 20, 2019

so the failure (no handle) occurs in Class.cs:103

returnobjc_getClass(name);

This is likely the same root cause as70844a8 which@rolfbjarne already filed with Apple

To workaround (and merge) this please exclude both types from introspection and add this PR/commit to the list of things we need to review before the final release ->#6212

spouliot suggested changes

Jun 20, 2019

View reviewed changes

Copy link

Contributor

spouliot left a comment

Choose a reason for hiding this comment

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

intro must ignore the failures for now :|

fixing head and skipping functions with class issues

5334575

spouliot reviewed

Jun 20, 2019

View reviewed changes

tests/introspection/iOS/iOSApiProtocolTest.cs Outdated

		case"SNClassificationResult":// Class is not being created properly
		returntrue;
		case"SNClassifySoundRequest":// Class is not being created properly
		returntrue;