Fix sdl2-gfx, sdl2-image, sdl2-mixer, sdl2-net, and sdl2-ttf forcing debug lib first (SDL2d)

[amaiorano]

When using sdl2-net on Windows, release builds end up linking against SDL2d.lib since it comes first in find_library. Removing all of this since it's not necessary. If using SDL2-net, it's reasonable to assume the client is already also linking against SDL2. Note that this was Windows-only, so for cross-platform builds, explicitly linking against SDL2 was already necessary.

[Rastaban]

Could you also bump the CONTROL version number up to 2.0.1-6? This will force a rebuild on vcpkg upgrade

[msftclas]

All CLA requirements met.

[Rastaban]

I am seeing a new build failure on arm64-windows, x64-windows, and x86-windows with this PR. Can you look into them?

[amaiorano]

I am seeing a new build failure on arm64-windows, x64-windows, and x86-windows with this PR. Can you look into them?

I suppose you're referring to the Vcpkg-PR-Eager failure? It seems I'm not authorized to access it. It asks me to log in with a Microsoft account; but when I use the same one that I use to sign into Visual Studio, it tells me:

[amaiorano]

I haven't been able to look at the tests, but I did take a look at the CMakeLists.txt files under ports/ for sdl2-gfx, sdl2-ttf, sdl2-image, and sdl2-mixer, and all of these do the same thing as sdl2-net. They all search for the SDL2 lib like so:

find_library(SDL_LIBRARY NAMES SDL2d SDL2)

In my case, I'm using sdl2-net, and what ends up happening is that because it searches for SDL2d before SDL2, it ends up finding it:

As you can see, my Release build picks up SDL2d.lib. It also picks up SDL2.lib below, but that comes from my dependency on sdl2 directly.

I'm not sure what the right solution is. I tried something else: I modified the sdl2-net port CMakeLists.txt so that instead of find_library(SDL_LIBRARY NAMES SDL2d SDL2), it does find_package(SDL2 CONFIG REQUIRED) instead, and rather than add ${SDL_LIBRARY_NAMES} to target_link_libraries, it now adds SDL2::SDL2-static. So basically, using the modern config pattern to depend on SDL2 instead of the old find_library method. This works for me locally, and has the added benefit of only adding the dependency on SDL2.lib (or SDL2d.lib) once. I'll push this as a new commit, and we'll see if that passes tests.

[amaiorano]

Looks like Vcpkg-PR-Eager has passed on this second commit. If you think this is good, I could apply this same fix to sdl2-gfx, sdl2-ttf, sdl2-image, and sdl2-mixer.

[Rastaban]

This looks good to me. Do you want to apply the other changes in this PR?

[amaiorano]

This looks good to me. Do you want to apply the other changes in this PR?

Alright, will do, and will rewrite history and update the PR name Will first push a separate commit to see it passes tests; afterwards, I'll squash all down to a single commit.

Note that I don't use the other libs, so we'll have to rely on the tests, but really, if it works for one, it should work for the others.

[amaiorano]

Btw, I didn't get a response about not being able to access Vcpkg-PR-Eager. Is there something I need to do?

[Rastaban]

Vcpkg-PR-Eager is hosted on a Microsoft Internal Azure DevOps instance, its access is not controlled by the vcpkg team so we have been pasting the results into PRs. We are working through the steps to get it moved to https://dev.azure.com/vcpkg/ where we will be able to open it up to the community. This will hopefuly happen soon.

[past-due]

@amaiorano: What about conditionally linking to SDL2::SDL2 and SDL2::SDL2-static depending on the value of VCPKG_LIBRARY_LINKAGE?

if(VCPKG_LIBRARY_LINKAGE STREQUAL "static")
   # instruct CMakeLists.txt to link to: SDL2::SDL2-static
else()
   # instruct CMakeLists.txt to link to: SDL2::SDL2
endif()

EDIT: Whoops. Didn't realize this was in CMakeLists.txt files, and not the port file. Checking VCPKG_LIBRARY_LINKAGE will presumably either have to be done in the portfile, or passed-through via a configure of the CMakeLists.txt before copying it to the source directory. However, it may be possible to condition on BUILD_SHARED_LIBS inside the CMakeLists.txt to match the SDL2 linkage with the SDL2-net/gfx/etc linkage?

[amaiorano]

@amaiorano: What about conditionally linking to SDL2::SDL2 and SDL2::SDL2-static depending on the value of VCPKG_LIBRARY_LINKAGE?

Ah, I thought vcpkg only created static libraries. I didn't know about VCPKG_LIBRARY_LINKAGE. I can certainly do as you propose. @Rastaban does that make sense to you as well?

[amaiorano]

TBH, I don't even understand why SDL2 would require us to specify either SDL2-static or just SDL2 - can't it figure it out based on the CMake environment? Locally, I tried modifying my project to use SDL2::SDL2, despite only building static libs in vcpkg, and it worked...

[past-due]

vcpkg has a wrapper-workaround (in the SDL2 port: see sdl2/vcpkg-cmake-wrapper.cmake) that defines one as the other if one is missing.

I suppose we could just rely on the presence of that wrapper, and keep things as they are?

[amaiorano]

Okay, I added support for both static and dynamic SDL2 libs. I've squashed all and force pushed, updated title, etc. Hopefully this passes tests. @past-due would you be able to test this locally as well?

[past-due]

@amaiorano: I'm not sure that VCPKG_LIBRARY_LINKAGE is available in the CMakeLists.txt by default (I believe it's just defined in the portfile). See my edited comment above. Your changes will work, but I think they may always be hitting the else() case and then relying on the sdl2/vcpkg-cmake-wrapper.cmake.

@Rastaban: Thoughts on this (and the above)? Should we bother trying to configure the "appropriate" SDL2 target by linkage, or just pick one and rely on the sdl2/vcpkg-cmake-wrapper.cmake?

[cenit]

What about using the CMake official (but reversed in logic ;) ) BUILD_SHARED_LIBS, which is perfectly managed by vcpkg and correctly passed to the toolchain as true for shared builds?

[Neumann-A]

will probably be solved by #5543

[amaiorano]

Hey everyone,

So do we wait for #5543 to land, or do we proceed with this change?

[Rastaban]

I lean towards waiting for the other PR to land.

[cbezault]

#5543 does not actually resolve the issue this PR is addressing. I'm still looking at the static/dynamic question more closely before merging.

[amaiorano]

Thanks for wrapping this up and merging!