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

Add Opus support to CMake builds #386

Merged
merged 1 commit into from
May 20, 2022
Merged

Conversation

ccawley2011
Copy link
Contributor

This should not be merged until the necessary CMake changes are backported to the SDL branches for opus and opusfile.

@slouken
Copy link
Collaborator

slouken commented May 9, 2022

Are there pull requests for opus and opusfile?

@ccawley2011
Copy link
Contributor Author

Are there pull requests for opus and opusfile?

I've opened libsdl-org/opus#1 and libsdl-org/opusfile#1 with the required changes.

@slouken
Copy link
Collaborator

slouken commented May 12, 2022

Are there pull requests for opus and opusfile?

I've opened libsdl-org/opus#1 and libsdl-org/opusfile#1 with the required changes.

These are in!

@ccawley2011 ccawley2011 marked this pull request as ready for review May 12, 2022 23:42
@madebr
Copy link
Contributor

madebr commented May 17, 2022

I rebased your pr on top of current master to get CI to test with the updated submodules.

@slouken slouken merged commit 3251942 into libsdl-org:main May 20, 2022
@ccawley2011 ccawley2011 deleted the opus-cmake branch May 20, 2022 20:09
@sezero
Copy link
Contributor

sezero commented May 21, 2022

After merging this, CI cmake runs are always failing: did we miss something?

@slouken
Copy link
Collaborator

slouken commented May 21, 2022

@madebr
Copy link
Contributor

madebr commented May 21, 2022

The error is in the vendored opusfile:

Due to my workstation currently out of order, I'm unable to push/test a fix.
But I think replacing this set with set(OPUSFILE_PROJECT_VERSION 0.0) or set(OPUSFILE_PROJECT_VERSION 0.1) fixes the error.
It's not perfect because the version is not detected correctly, but it's a quick fix nonetheless.

@sezero
Copy link
Contributor

sezero commented May 23, 2022

Or, we can just put a package_version file in there with PACKAGE_VERSION="1.3.1" ?
Possibly to opusfile too, don't know if it's really needed there?

@sezero
Copy link
Contributor

sezero commented May 23, 2022

Hm, tried adding the package_version files, still fails
https://github.com/libsdl-org/SDL_mixer/runs/6552834511?check_suite_focus=true

@sezero
Copy link
Contributor

sezero commented May 23, 2022

Cmake package_version thing is fixed by this:
libsdl-org/opusfile@9ba5cc3
(maybe good for mainstream submission??)

But build fails for other reasons now - @slouken, for you:
https://github.com/libsdl-org/SDL_mixer/runs/6553099365?check_suite_focus=true#step:7:396

 /usr/bin/ld: CMakeFiles/SDL2_mixer.dir/src/codecs/music_wav.c.o: in function `ParseID3':
music_wav.c:(.text+0x1d4e): undefined reference to `read_id3v2_from_mem'
collect2: error: ld returned 1 exit status

@sezero
Copy link
Contributor

sezero commented May 23, 2022

But build fails for other reasons now - @slouken, for you

Well the cmake script was missing mp3utils.c - adding all missing sources fixed it.

@sezero
Copy link
Contributor

sezero commented May 23, 2022

I opened a ticked at xiph opusfile: xiph/opusfile#33

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