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

Revert AudioUnit/AudioToolbox linking change but put AudioUnit first #49

Closed
wants to merge 1 commit into from

Conversation

MichaelHills
Copy link
Contributor

In response to #48 but need to do some testing

Copy link
Member

@simlay simlay left a comment

Choose a reason for hiding this comment

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

If this builds for iOS and macOS, this is fine with me. It's unclear to me how order would change the linking error @clangdo mentioned in their ci. I could see how order would matter for headers in bindgen but this seems to be after the bindgen phase.

Would you be up to adding #38 to part of this PR? I can follow up otherwise.

@clangdo
Copy link

clangdo commented Feb 22, 2021

Wow you folks are really quick with these things, are you sure this won't backfire on people who just want the audio-unit feature on recent versions of the SDK? Are they going to be in trouble when they link to the (now apparently defunct) AudioUnit framework? (Also I can see I wasn't the only one getting FFI warnings when I did my testing.) I know this is a little more work, but I might be inclined to put in another config option to link the old library rather than the new one? Otherwise just keep linking AudioToolbox? I agree that Apple's official documentation suggests linking AudioToolbox for those functions is the way to go.

@MichaelHills
Copy link
Contributor Author

There's a reason this is a draft PR. :) I was going to re-test everything first but had trouble connecting XCode to my iPhone the other night.

FYI @simlay the reason for the swap was linked in the issue, this link https://forum.juce.com/t/plug-in-built-with-xcode-11-fails-to-load-on-yosemite-10-10/35912/44 I stumbled across. I figured swapping the order can't hurt? At the same time, we can leave this all alone, if it's not broken don't fix it.

Anyway I now see the edit in the linked issue, so it did work at some point. @simlay if you have time feel free to push ahead with adding the cross-compilation tests. I can help with some testing in general. I only have a small amount of time on weekends to give this any attention.

@MichaelHills
Copy link
Contributor Author

I was also consdering linking AudioUnit and AudioToolbox for the AudioUnit feature flag (not just reverting the change) if it was necessary after a round of testing. But I was able to build for macOS, iOS simulator and iOS device with just the revert... so I was still trying to figure out the original need for the change.

@madsmtm
Copy link
Contributor

madsmtm commented Dec 1, 2021

coreaudio-sys does not work on older macOS versions than 10.11 (don't know anything about iOS), because a lot of functions that were previously in the AudioUnit framework moved to the AudioToolbox framework (so just linking to AudioToolbox won't work).

Try it with MACOSX_DEPLOYMENT_TARGET=10.10 cargo build --example feedback in the coreaudio-rs repo.

With this PR, the issue is fixed.

This is especially relevant now that the current nightly sets the default MACOSX_DEPLOYMENT_TARGET to 10.7, see rust-lang/rust#91372, which means that this library doesn't work for users without specifying a higher MACOSX_DEPLOYMENT_TARGET.

(now apparently defunct) AudioUnit framework?

Can you elaborate on this? Is there a problem with linking to AudioUnit on newer versions? (Except that it doesn't do anything)?

I was also consdering linking AudioUnit and AudioToolbox for the AudioUnit feature flag

I think that would work.

@madsmtm
Copy link
Contributor

madsmtm commented Dec 6, 2021

Pinging @MichaelHills @mitchmindtree @simlay, please look into this when you get the time.

There's a bit of time pressure here; again, this library doesn't link for macOS users using Rust nightly, and the same goes for the not-so-far-away Rust 1.58.
And it affects all downstream dependencies (coreaudio-rs, cpal, rodio, hound, amethyst, bevy, and many others).

I can make an updated PR with the changes described above (linking both AudioUnit and AudioToolbox for the audio_unit feature flag) if that's helpful?

@simlay
Copy link
Member

simlay commented Dec 6, 2021

I can make an updated PR with the changes described above (linking both AudioUnit and AudioToolbox for the audio_unit feature flag) if that's helpful?

Yes. You definitely know the space the best it seems.

PS: Love your work with objc2 and such @madsmtm.

@madsmtm madsmtm mentioned this pull request Dec 9, 2021
@madsmtm
Copy link
Contributor

madsmtm commented Dec 9, 2021

linking both AudioUnit and AudioToolbox for the audio_unit feature flag

So, I looked into it some more, and found that on macOS, AudioUnit exports the desired symbols in all versions (on newer versions they're just re-exports from AudioToolbox). On iOS, there is no AudioUnit to link to, so I've opened #51 (which builds upon this PR) but adds the required changes for iOS.

Wow you folks are really quick with these things, are you sure this won't backfire on people who just want the audio-unit feature on recent versions of the SDK? Are they going to be in trouble when they link to the (now apparently defunct) AudioUnit framework?

See my PR for more info on the linking; in short, yes, I'm at least like 90% sure, applications linking to AudioUnit still works like they always have, Apple has kept backwards compatibility there.

It's unclear to me how order would change the linking error

I honestly think it's just some arcane wisdom that probably doesn't apply any more, and maybe never did? See this SA answer for a bit of background on linking dynamic libraries (frameworks are just a glorified version of that), it says directly "dynamic libraries sort out their dependencies themselves".
But I digress, it doesn't hurt to change the order.
See below.

@madsmtm
Copy link
Contributor

madsmtm commented Dec 9, 2021

Actually, forget what I wrote about the order, I looked at the linked thread and that does seem like a real problem (even though I can't reproduce it because I don't have an old enough macOS version).

PS: Love your work with objc2 and such @madsmtm.

Thanks!

@MichaelHills
Copy link
Contributor Author

Looks like this has been superceded by #51 which has landed, closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants