[sdl2-mixer] Fix features dependency link.

[LilyWangL]

Fix features dependency link.
Related PR: #7720.
@Vermeille @lethal-guitar Could you test this?
Thanks.

[Vermeille]

Thanks for fixing that so fast!

Unfortunately doesn't work. See the inline comments.

Also it doesn't look like the libraries are considered a dependency by CMake, so then won't be installed by default when installing something using SDL2-mixer. I'd gladly do it myself but I have no idea how to achieve that

[JackBoosY]

@Vermeille If we want them to be the default feature, I will add them.

[Vermeille]

@JackBoosY I'm not sure I explained myself clearly.

I don't think SDL2_mixer should be built with any optional dependency by default.

However, if we select mpg123 as a vcpkg feature, then obviously SDL2_mixer should be built with the mpg123 functionality (done with activating the feature macro at build time), and considered by cmake as a dependency, so that when installing a project (like here ), sdl2_mixer is seen as a dependency of the project (and have sdl libs installed) and mpg123 is seen as a dep of SDL2_mixer (thus installing mpg123 libs as well).

[JackBoosY]

@Vermeille We can use two way to solve this issue:

1. Add them as dependencies and statically link them to sdl2-mixer using target_link_library. So these libraries must exist while sdl2-mixer starts, otherwise sdl2-mixer will not be available. This way will breaks the feature of sdl2-mixer, see the discussion, documentation.

As of SDL_mixer 1.2.7, FLAC, MikMod, Ogg Vorbis and MP3 loading libraries are dynamically loaded, so if you don't need to load those formats, you don't need to include those shared libraries.

2. Add them as dependencies and compile ONLY their header files and wait for them to be dynamically loaded at runtime. This way you don't need to have these libraries when sdl2-mixer starts. For some reason, vcpkg does not copy dependencies when the build is complete.

@lethal-guitar What do you think？

[Vermeille]

@JackBoosY

Yes. I understand that. But there are two dependecies at play:

1. Dependencies of the library at link time. I do agree with the previous PR. sdl2 mixer allows optional deps, they should be kept optional, and the previous PR removing target_link_library was right.
2. Dependencies of the cmake target, that will be shipped in the installation process.

While I don't care about the dependencies of the library since I will not change what sdl2 mixer addons will be used after installation, I do care much about what CMake's install will ship.

I've been trying to suggest, maybe with poor wording, that removing the addons as a link dependency of the library is a fine and probably best, but they should still be considered deps by cmake to get installed along sdl2 mixer

So it definitely looks like your solution 2 is what I'm after, if we can have cmake copy dependencies correctly with sdl2 mixer.

[JackBoosY]

@Vermeille Even though sdl2-mixer does not handle the installation process, vcpkg does this because vcpkg builds these dependencies before building sdl2-mixer and installs them in the same directory with sdl2-mixer. And when you need to export sdl2-mixer, vcpkg will also export them, so you don't need to care about them. Unless you use other tools to run CMakeLists.txt instead of vcpkg.

[Vermeille]

@JackBoosY I use CPack. Is this wrong?

[JackBoosY]

@Vermeille Well, that's the difference between us. If we add these file(INSTALL) to CMakeLists.txt, it will conflict with the vcpkg installation process. So I suggest you use vcpkg export to export them : )

[Vermeille]

@JackBoosY Thank you for this clarification. I will investigate how to use vcpkg export in conjuction of CPack for my project then. Thanks!

[lethal-guitar]

@LilyWangL sorry for the late response. I'm happy to do some testing, but won't get around to it until next week.

@JackBoosY solution 2 sounds like the right one to me as well.

[ghost]

Whatever was done to SDL2-Mixer has broken it on win64. Tons of unresolved symbols now.
install-x64-windows-dbg-out.log
This was on both vs2017 and vs2019 - just did a vcpkg upgrade --no-dry-run on either version, x64.

[Vultraz]

Can confirm broken on x64

   Creating library SDL2_mixer.lib and object SDL2_mixer.exp
music_ogg.c.obj : error LNK2019: unresolved external symbol ov_clear referenced in function OGG_Load
music_ogg.c.obj : error LNK2019: unresolved external symbol ov_open_callbacks referenced in function OGG_Load
music_ogg.c.obj : error LNK2019: unresolved external symbol ov_pcm_total referenced in function OGG_Load
music_ogg.c.obj : error LNK2019: unresolved external symbol ov_pcm_seek referenced in function OGG_Load
music_ogg.c.obj : error LNK2019: unresolved external symbol ov_time_seek referenced in function OGG_Load
music_ogg.c.obj : error LNK2019: unresolved external symbol ov_pcm_tell referenced in function OGG_Load
music_ogg.c.obj : error LNK2019: unresolved external symbol ov_info referenced in function OGG_Load
music_ogg.c.obj : error LNK2019: unresolved external symbol ov_comment referenced in function OGG_Load
music_ogg.c.obj : error LNK2019: unresolved external symbol ov_read referenced in function OGG_Load
SDL2_mixer.dll : fatal error LNK1120: 9 unresolved externals

[Vultraz]

@timautry Found linking against vorbisfile fixed the problem for me. Perhaps a similar fix would work for the other options?

 ports/sdl2-mixer/CMakeLists.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ports/sdl2-mixer/CMakeLists.txt b/ports/sdl2-mixer/CMakeLists.txt
index 96529d096..ced7b4061 100644
--- a/ports/sdl2-mixer/CMakeLists.txt
+++ b/ports/sdl2-mixer/CMakeLists.txt
@@ -34,8 +34,10 @@ endif()
 # Ogg-Vorbis support
 if(SDL_MIXER_ENABLE_OGGVORBIS)
     find_path(VORBIS_INCLUDE_DIR vorbis/codec.h)
+    find_library(VORBIS_LIB vorbisfile)
     list(APPEND SDL_MIXER_INCLUDES ${VORBIS_INCLUDE_DIR})
     list(APPEND SDL_MIXER_DEFINES MUSIC_OGG)
+    list(APPEND SDL_MIXER_LIBRARIES ${VORBIS_LIB})
 endif()
 
 # Opus support

[ghost]

Thanks Vultraz, but there's no change - same 47 unresolved symbols.
It seems the change to the portfile causes the problem on win64 - if I revert to the previous version of portfile, it compiles fine and uses the supplied sources mpg123, libmodplug, flac, libvorbis and opusfile instead of the ones installed be vcpkg... I'm not too good at figuring out the cmake parts....

[JackBoosY]

After delving into the code, I found that using dynamic links must define -D${PORT}="${PORT_LIBRARY}" to use.