Skip to content

Conversation

fancycode
Copy link
Member

The introduction of different components like SDL2::SDL2 / SDL2::SDL2main happened in libsdl-org/SDL@dd1d8ab (2.0.12).

Also use detection code from https://wiki.libsdl.org/SDL2/README-cmake to figure out when to link against SDL2::SDLmain.

Only tested on different versions of Ubuntu, so more testing on Windows should be done before merging.

@kmilos
Copy link
Contributor

kmilos commented Sep 1, 2025

Focal is EOL, there is no need to put effort into it IMHO.

@fancycode
Copy link
Member Author

Focal is EOL, there is no need to put effort into it IMHO.

I know (it's still in Ubuntu Pro support though) but unfortunately I still didn't get to update one of my dev machines yet so I looked into the recent linker errors.

@kmilos
Copy link
Contributor

kmilos commented Sep 1, 2025

it's still in Ubuntu Pro support though

For security patches only, not supporting the most modern libs...

The changes around SDL2main are interesting though. 👍

common.h)
target_link_libraries(heif-view PRIVATE heif SDL2::SDL2main SDL2::SDL2)
target_link_libraries(heif-view PRIVATE heif)
target_include_directories(heif-view PRIVATE ${libheif_SOURCE_DIR})
Copy link
Contributor

@kmilos kmilos Sep 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this new explicit addition needed? AFAICT, the heif target should already carry over its include path definitions... Is something not working?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This got removed in 56e85c0 in the switch to the SDL2 components but is there for the other example apps, so I figured it might have been removed by accident.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it might have been removed by accident

It looks like it indeed, but it shouldn't be needed IMHO. It'd be interesting to see if it could also be removed elsewhere in a separate commit...

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.

2 participants