[sdl2-mixer]Remove useless dependency link libraries.

[JackBoosY]

Since the library method using features in sdl2-mixer is dynamically loaded (using the function SDL_LoadFunction), remove all links that depend on the feature library.

Related: #7356.

[JackBoosY]

@lethal-guitar Can you test this?

Thanks.

[lethal-guitar]

@JackBoosY Will do, thanks!

[JackBoosY]

@lethal-guitar Does this PR fix this issue?

[lethal-guitar]

@JackBoosY sorry, I didn't have a chance to test this yet. I'll get back to you as soon as I can! Sometime this week should be doable.

[lethal-guitar]

@JackBoosY sorry for the delay! I finally found the time to test this properly, and I'm somewhat puzzled. I have a version of SDL2_mixer.dll which has the dependencies mentioned in my issue (see screenshot), but I haven't been able to reproduce this problem locally (the DLL In question was built on CI by AppVeyor).

I have tested the master branch of vcpkg as well as the 2019.06 tag (since the DLL I have is dated 1st of June), and a random commit from before the 2019.06 tag. For each of these commits, I ran:

bootstrap-vcpkg.bat
vcpkg remove sdl2-mixer:x64-windows
vcpkg.exe install sdl2-mixer:x64-windows --triplet x64-windows

But the resulting SDL2_mixer.dll never exhibited the dependencies observed in the other DLL.

Do you know if I might be missing a step in my testing? If not, I think we can close this PR as well as close #7356, as it seems the problem is already fixed on master.

[JackBoosY]

@lethal-guitar I think you missed these features in the installation list.
You need to install sdl2-mixer using the command vcpkg.exe install sdl2-mixer[core,libflac,mpg123,libmodplug,libvorbis,opusfile].

[lethal-guitar]

@JackBoosY I see, thanks! I'll give it another try tonight.

[lethal-guitar]

@JackBoosY @cbezault I was finally able to get back to this (apologies again for the delay).

I can confirm that the changes on this branch result in a SDL2_mixer.dll without load-time dependencies, even when using the command line that enables the corresponding features. The master branch on the other hand does exhibit these dependencies, when enabling the features (see screenshots below).

I have also tested the resulting DLL with my project, and found that it works fine.

What I don't know is how this change affects users who need these additional features, so it might be worth confirming with someone else that they still work as expected.

Dependency Walker

Current master:

This branch:

[JackBoosY]

@lethal-guitar Thanks for testing.
I think this PR should fix this issue.

[Vermeille]

This absolutely doesn't work for me for mpg123.
I guess SDL2_mixer is now built without support for those dependencies, so:

• the preprocessor flags aren't set
• the headers aren't here when compiling

Am I doing smth wrong?

[Vermeille]

Okay so I compared with the commit prior to this one, and yeah, it impacts things. It doesn't build the parts of the code that links against those dlls.

So if the dll linking is not acceptable to you, maybe we can revert to the previous stage and remove only the target_add_library commands? Or we can always build SDL2_mixer against all optional dependencies, but it doesn't sound like "pay only for what you use" and increases build time.

target_link_library also had the neat effect of adding those audio libraries into the installation process. An app depending on sdl mixer would get those installed along all the other runtime requirements

[JackBoosY]

@Vermeille As @lethal-guitar said, even with some features, we should NOT add these dependent ports to the static link list of sdl2-mixer.

[lethal-guitar]

@JackBoosY If I understand correctly, what @Vermeille is saying is that at the moment, it's not possible at all to use these features, since the code that dynamically loads the libraries seems to be excluded from the build. It's true that the libraries shouldn't be linked at compile time, but the library should still be able to dynamically load them.

As I mentioned in my comment:

What I don't know is how this change affects users who need these additional features, so it might be worth confirming with someone else that they still work as expected.

Seems that this PR did indeed break the usage of these features. I don't use these optional features myself, so I didn't test how this PR affects the usage of them, hence my comment.

In an ideal world, we would keep the behavior of copying the DLLs into the installation directory when the corresponding features are enabled, but avoid linking to these DLLs at compile time to avoid the hard requirement.

[Vermeille]

@lethal-guitar Absolutely. It doesn't build the code looking for those dlls.

So I guess we should go back to the previous build file, but exclude the target_link_libraries to not link them to sdl mixer.
However I'd still very like to have CMake consider them as part of the runtime requirements so that they're seamlessly installed when calling install() ; but I have no idea how to do that.

[JackBoosY]

@Vermeille I know what you said, we should not link these dependencies to sdl2-mixer, but we need to add these header files to compile them, right?

[Vermeille]

@JackBoosY Absolutely.