conda-forge / sdl2_image-feedstock

A conda-smithy repository for sdl2_image.
BSD 3-Clause "New" or "Revised" License
1 stars 7 forks source link

Windows: sdl2_image version 2.8.2 is missing the SDL2_image::SDL2_image target; version 2.8.0 works #36

Open certik opened 1 month ago

certik commented 1 month ago

Version 2.8.2:

-- Configuring done (1.7s)
CMake Error at CMakeLists.txt:21 (target_link_libraries):
  Target "example1" links to:

    SDL2_image::SDL2_image

  but the target was not found.  Possible reasons include:

    * There is a typo in the target name.
    * A find_package call is missing for an IMPORTED target.
    * An ALIAS target is missing.

-- Generating done (0.1s)
CMake Generate step failed.  Build files cannot be regenerated correctly.

Version 2.8.0 works perfectly.

certik commented 1 month ago

I figured out what the issue is. The CMake config files from sdl2_image require exactly libavif="1.0.4" (any 1.0.* would work), which if added to dependencies, then everything works.

So either the requirement should be relaxed, or the libavif version should be restricted in conda.

h-vetinari commented 1 month ago

So either the requirement should be relaxed, or the libavif version should be restricted in conda.

Would be better IMO to loosen the CMake config with a small patch. Could you send a PR?

certik commented 1 month ago

Here is the upstream code:

https://github.com/libsdl-org/SDL_image/blob/b6fe4176d55e047f4b93af1eb1d09d8ede6731f3/SDL2_imageConfig.cmake.in#L48

As you can see, they require libavif 1.0 and if it is not found, they try an older version @LIBAVIF_MINIMUM_VERSION@ which for me was something like 0.9.3 (I can't remember) and when that fails, they return(), which prevents to execute the code below below that: https://github.com/libsdl-org/SDL_image/blob/b6fe4176d55e047f4b93af1eb1d09d8ede6731f3/SDL2_imageConfig.cmake.in#L82, which sets up the SDL2_image::SDL2_image target, and then in my main cmake the target is not found.

To fix that, we can remove the version restriction. But is that a good idea? We can also remove the return() statement, which also seemed to make it work. But I don't know the background here and the intended logic, as well as the range of which libavif libraries are supported.

For SDL3 the code is here:

https://github.com/libsdl-org/SDL_image/blob/b88be01d57450740b305667f61f5ec22e9706a7a/cmake/SDL3_imageConfig.cmake.in#L82

But I haven't used SDL3 yet.

@h-vetinari let me know how to proceed.

h-vetinari commented 1 month ago

I'm confused now, first you said it pins libavif to patch level, and now only to 1.0? If it's just a missing dependency, we can also skip the patching and just ensure that libavif is there as a runtime dependency?

certik commented 1 month ago

I wrote:

require exactly libavif="1.0.4" (any 1.0.* would work),

Since locally I pinned libavif to exactly 1.0.4 and that worked, but since the cmake code says find_package(libavif 1.0 QUIET), I am guessing that any 1.0.* version would work, although I haven't tested it. However version libavif=1.1.1 does not work, and that is the default package that got installed on my system initially and why it failed above. Hopefully that clarifies it.

we can also skip the patching and just ensure that libavif is there as a runtime dependency?

It seems it has to be there as a runtime dependency, but it must be exactly 1.0.*, not 1.1.*.

So the real question is why is it looking exactly for libavif 1.0 and not just libavif? Is it not working with libavif 1.1? I think I tested it, and it seemed to work, but I do not actually use the AVIF format at all, so I don't know.

My inclination is that since upstream is very clearly specifying libavif 1.0, then I would require exactly libavif 1.0.* as a dependency. That should fix it. We can report it upstream for them to relax it, but I would think it's their problem, not ours.

h-vetinari commented 1 month ago

Looking at the work of @conda-forge/libavif, it appears that both libavif 1.1 and libavif 1.0 depend on libavif16, which -- according to the recipe -- is supposed to represent the SOVERSION. Based on the run-export, this also only enforces that this SOVERSION stays the same, IOW, 1.0 & 1.1 are supposed to be compatible.

I guess that explains why we end up pulling in 1.1 into an environment with sdl2_image, which then trips up the CMake checks that expect 1.0. For compatibility with all our other packaging, I think we should patch out the .0 portion of the upstream expectations, otherwise sdl2_image is going to be hard install with anything else that expects libavif.

Would be good if @conda-forge/libavif could comment about the expected compatibility though? 🙏

hmaarrfk commented 1 month ago

Yes generally speaking we split the package to try to make it easier to maintain. thought the discrepancy between ABI and Version number is annoying...