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

Find OpenAL correctly on iOS #1376

Closed
wants to merge 1 commit into from
Closed

Find OpenAL correctly on iOS #1376

wants to merge 1 commit into from

Conversation

JonnyPtn
Copy link
Contributor

Following on from #1263

This removes the conditional checks which were stopping openAL being found properly on iOS, and updates the relevant includes

@eXpl0it3r eXpl0it3r added this to Discussion in SFML 2.5.0 via automation Feb 24, 2018
@eXpl0it3r eXpl0it3r added this to the 2.5 milestone Feb 24, 2018
@eXpl0it3r eXpl0it3r moved this from Discussion to Ready in SFML 2.5.0 Feb 24, 2018
@Ceylo
Copy link
Contributor

Ceylo commented Feb 25, 2018

Does this find the OpenAL framework in macOS SDK, or iOS SDK, or the headers provided by SFML? In my opinion the best would be to use the framework from iOS SDK but I don’t think current code with find_host_package() does this. And actually there is most likely no search to do, just using flag «-framework OpenAL » should use the correct headers (from iOS SDK).

@JonnyPtn
Copy link
Contributor Author

JonnyPtn commented Feb 26, 2018

For me, it finds the openAL framework in the iOS sdk: /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator11.2.sdk/System/Library/Frameworks/OpenAL.framework

if I just manually link the framework it doesn't seem to find any of the headers

@Ceylo
Copy link
Contributor

Ceylo commented Feb 26, 2018

Ok.. then I really don’t understand the purpose of find_host_package() which is made (from what I understand) specifically to NOT find dependencies for the target system but for the host...

Can check what I get with your branch on my PC in ~2 days if you want to wait.

@JonnyPtn
Copy link
Contributor Author

Sure, we can hold off for a bit. In the original PR for the toolchain it was mentioned that find_host_package was just taken from the android toolchain, so it may well be redundant...

@Ceylo
Copy link
Contributor

Ceylo commented Feb 28, 2018

Here is what I get with your branch
cmake cache

So for OpenAL it looks good indeed, even if I'm still puzzled by find_host_package() and its expected behavior…

@JonnyPtn
Copy link
Contributor Author

JonnyPtn commented Mar 1, 2018

I think the intention is that find_package should only search in the relevant sdk, and find_host_package uses all the standard search paths?

If that's correct, then it should probably be find_package for openAL, instead of find_host_package?

@Ceylo
Copy link
Contributor

Ceylo commented Mar 4, 2018

This is also my understanding… but hey, good luck getting it to work with just find_package() :)
Can be fixed later I think, this is a global issue with find_host_package() on iOS, not specific to OpenAL.

@JonnyPtn
Copy link
Contributor Author

JonnyPtn commented Mar 5, 2018

I just tried using find_package() for openAL, and it seems to still work as expected. It's the other 3rd party libs which don't like find_package(), because they aren't installed in the sdk, as expected. Are you able to do a quick test using find_package for openAL and confirm it still works for you too?

@Ceylo
Copy link
Contributor

Ceylo commented Mar 6, 2018

find_package() indeed works fine for OpenAL, which makes sense indeed… so it could be fixed (at least for this dependency).

As a sidenote, it is however possible to wait for #1335 to be merged before this PR? #1335 assumes that all find_package() on iOS actually are find_host_package() so they were all factorized in sfml_find_package(). If we introduce an exception for OpenAL, it'll require significant changes in the already long-waiting PR :(

@JonnyPtn
Copy link
Contributor Author

JonnyPtn commented Mar 6, 2018

I have no problem with waiting. I haven't changed this PR, so it still uses find_host_package, so it can either be merged in it's current state which (if I understand correctly) won't conflict with #1335, or If #1335 is merged first I will make the relevant changes to this PR

@Ceylo
Copy link
Contributor

Ceylo commented Mar 6, 2018

See https://github.com/SFML/SFML/pull/1335/files#diff-6e3aba620b5d5027b0b256ccb9ad1063
A lot has changed actually so even if this PR was merged as-is it would conflict. I've been too greedy in changing everything, sorry 😄

@JonnyPtn
Copy link
Contributor Author

JonnyPtn commented Mar 6, 2018

No worries, this is a super simple fix, so let's just pop it on hold, and I'll update once #1335 is merged.

@eXpl0it3r
Copy link
Member

Merged in f963faa

@eXpl0it3r eXpl0it3r closed this Mar 12, 2018
SFML 2.5.0 automation moved this from Ready to Merged / Superseded Mar 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
SFML 2.5.0
  
Merged / Superseded
Development

Successfully merging this pull request may close these issues.

None yet

3 participants