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

Don't need to find vorbisfile or vorbisenc on iOS #1347

Closed
wants to merge 1 commit into from
Closed

Don't need to find vorbisfile or vorbisenc on iOS #1347

wants to merge 1 commit into from

Conversation

JonnyPtn
Copy link
Contributor

As mentioned in #1209 the vorbis lib on iOS is now all inclusive, however SFML cmake configuration still looks for vorbisfile and vorbisenc, so the build fails if they aren't present

@eXpl0it3r eXpl0it3r added this to the 2.5 milestone Jan 24, 2018
@eXpl0it3r eXpl0it3r added this to Review & Testing in SFML 2.5.0 Jan 24, 2018
@LaurentGomila
Copy link
Member

When did that change? What about still supporting older versions of iOS? Did they also change that for other OSes?

@MarioLiebisch
Copy link
Member

My big question would possibly be whether this change is compile/link time only or run-time as well? Considering how Apple regularly deprecates older versions and forces upgrades I'm not sure how important keeping the old code would be.

@JonnyPtn
Copy link
Contributor Author

When did that change?

When #1209 was merged

What about still supporting older versions of iOS?

This PR has nothing to do with iOS versions, it just fixes the project to build. I don't know what effect #1209 would have on iOS version compatibility, although I still suspect not much, if any

Did they also change that for other OSes?

Again, If you're talking about #1209, then no, as far as I'm aware other OS's still have separate vorbis files which is why this PR only affects iOS builds

My big question would possibly be whether this change is compile/link time only or run-time as well?

It's CMake configure time.

Considering how Apple regularly deprecates older versions and forces upgrades I'm not sure how important keeping the old code would be.

Apple have nothing to do with this. If I understand correctly the reason #1209 changed this is because it's using a more recent version of vorbis than other OS extlibs.

@LaurentGomila
Copy link
Member

When #1209 was merged

I mean, when did that change in the library, not "when did we apply the change in SFML".

This PR has nothing to do with iOS versions

My bad, I thought the library was provided by the OS, but apparently we build it ourselves.

as far as I'm aware other OS's still have separate vorbis files which is why this PR only affects iOS builds

Sounds weird that they did such a change only for iOS libraries. Would be interesting to know why they did it.

@JonnyPtn
Copy link
Contributor Author

No idea when vorbis changed, struggling to find any information on it... @sol-prog or @eXpl0it3r may be able to shed some light

I agree it's weird, and ideally the libs should be the same version/format as on other platforms.

@JonnyPtn
Copy link
Contributor Author

Any more thoughts on this? If you'd prefer me to rebuild the vorbis libraries I can do that?

This is a real blocker for any other iOS testing

@eXpl0it3r
Copy link
Member

Somehow I thought they moved on to merging them in general, but looking at the VS solution in their repo, it doesn't look to be the case.

Personally, I don't mind if the library list is different for iOS compared to other OS, as long as it works. I mean if you have the time, you could see if you build them for iOS whether they get built or if @sol-prog did some magic to include them into one library.

If you don't want to investigate this, I'll merge this PR.

@JonnyPtn
Copy link
Contributor Author

I definitely think it should match other platforms, but right now it doesn't seem clear which version is correct/best. Personally I'd say merge this for the time being as it only affects iOS and is required to get it building, then we can open a separate issue to discuss and fix up the vorbis extlibs inconsistencies?

@eXpl0it3r
Copy link
Member

Sounds fine to me, it's already this way except for the CMake-thingy being broken, so lets first fix the broken thingy and then see if/how the rest needs to be fixed. 🙂

@eXpl0it3r eXpl0it3r moved this from Review & Testing to Ready in SFML 2.5.0 Jan 27, 2018
@eXpl0it3r
Copy link
Member

Merged in d6c6345

@eXpl0it3r eXpl0it3r closed this Jan 29, 2018
SFML 2.5.0 automation moved this from Ready to Merged / Superseded Jan 29, 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

4 participants