Improve CMake build system by madebr · Pull Request #238 · libsdl-org/SDL_image

madebr · 2022-05-16T10:17:21Z

Use 'SDL2IMAGE_' prefix for all options
Create SDL2_image::SDL2_image when building a shared library,
create SDL2_image::SDL2_image-static when building a static library
Aim is to use same CMake layout as SDL_ttf/SDL_mixer
Create libSDL2_imaged.so symbolic link ('d' suffix) (when building debug build type)
Add PrivateSdlFunctions.cmake script for common functionality between SDL_ttf/SDL_mixer
Add FindXXX.cmake scripts for dependencies that don't provide cmake configuration files
Bring jpeg/png saving support to cmake (Define for SAVE_PNG support #235)
Update the git submodules
Test CMake on mingw64 (using system dependencies there)
Add MacOS CI
Create symbolic link at build time + install it
Only install SDL2_image.pc when building a shared SDL2_image (to avoid overwriting when later installing a static SDL2_image)
Allow parallel installation of cmake support of a static and shared SDL2_image
Add a single SDL2IMAGE_VENDORED option to use vendored libraries for everything, or none at all.
Add Macos dylib versioning

I haven't added these scripts to EXTRA_DIST in Makefile.am yet, to allow some maturing + extra tests.

Need review on:

ease of use: the cmake recipe is strict with its configuration options. When an option is configured to be enabled (-DSDL2IMAGE_xxx=ON), the cmake script will complain loudly instead of silently ignoring the option.
vendored libraries: build these as shared or static libraries? Always building these as static libraries makes perhaps most sense.

.github/workflows/main.yml

smcv · 2022-05-19T16:33:22Z

.github/workflows/main.yml

        rm -fr DESTDIR-cmake
        DESTDIR=$(pwd)/DESTDIR-cmake cmake --install build/ --config Release
-        ( cd DESTDIR-cmake; find ) | LC_ALL=C sort -u
+        find DESTDIR-cmake | LC_ALL=C sort -u


This was deliberate: I didn't want the DESTDIR-cmake common prefix generating extra noise when we list the installed files.

Please revert this change in one of your (current) pr's.
The reason I did this was because I wrongly though it failed on Macos here.

Will do. macOS might want ( cd ...; find . ) if its find(1) is less do-what-I-mean than Linux (I'm never sure what subset of the CLI is portable).

smcv · 2022-05-19T16:33:28Z

.github/workflows/main.yml

        mkdir DESTDIR-autotools
        make -j"${parallel}" -C build-autotools install DESTDIR="${curdir}/DESTDIR-autotools" V=1
-        ( cd DESTDIR-autotools; find ) | LC_ALL=C sort -u
+        find DESTDIR-autotools | LC_ALL=C sort -u


smcv · 2022-05-20T13:56:45Z

vendored libraries: build these as shared or static libraries? Always building these as static libraries makes perhaps most sense.

I think so. The possible build configurations for a library (let's say libpng) are:

use the system libpng
- as a static library
- as a shared library, linked with -lpng to be a mandatory dependency
- as a shared library, dynamically loaded with dlopen() at runtime to be a "weak" dependency
use the vendored libpng
- as a static library
- as a shared library, linked with -lpng to be a mandatory dependency
- as a shared library, dynamically loaded with dlopen() at runtime to be a "weak" dependency

But if you're using the vendored libpng, then I don't think the two shared-linking options really make sense: you're paying the compile time and disk space cost of a copy of libpng anyway, whether it's shared (a separate file) or static (copied into the SDL_image library) . Having it as a shared library seems like it just brings more reasons why the overall system can fail, for instance conflicts with a system copy that happens to have the same SONAME.

In particular, building your own vendored shared library for zlib seems like a recipe for disaster, because on Linux and macOS there are likely to be OS components that depend on the OS's zlib. If you release a game that includes SDL_image, then wait 5 years, the OS zlib will probably be newer than your bundled zlib, so other OS components will expect to find that newer version, and will crash if they find a vendored shared library instead.

I like the way SDL_ttf handles this: if it's building a vendored Freetype or Harfbuzz, then the vendored copy is statically linked into the SDL_ttf library. This makes the SDL_ttf library larger than it would be when using an OS copy, but avoids conflicting with an older or newer system copy of Freetype or Harfbuzz.

SDL_ttf also uses C_VISIBILITY_PRESET "hidden" to make sure it doesn't accidentally export symbols from its vendored libraries, making them into merely an implementation detail.

smcv · 2022-05-20T14:01:14Z

I'm not a maintainer, but:

I think if there are things that are functionally necessary (like being able to enable JPEG and PNG saving), it might be good to carve those out into smaller PRs, which will be quicker to review and less likely to conflict with other parallel work like #242.

Major refactoring and stylistic changes are basically guaranteed to conflict with everything else that touches the same files, so it's good to do those when the same files are otherwise quiet.

smcv · 2022-05-20T14:03:27Z

Major refactoring and stylistic changes are basically guaranteed to conflict with everything else that touches the same files, so it's good to do those when the same files are otherwise quiet.

When I've done this sort of refactor in other projects, the way I generally do it is to choose the piece of refactoring that I think is highest-priority or most likely to be accepted quickly, do that as its own PR, get it reviewed, choose the next-highest-priority, and repeat until everything I want has gone in.

I haven't set it up for CMake yet (although it does work) because that would conflict pretty badly with libsdl-org#238. Signed-off-by: Simon McVittie <smcv@collabora.com>

I haven't set it up for CMake yet (although it does work) because that would conflict pretty badly with #238. Signed-off-by: Simon McVittie <smcv@collabora.com>

slouken · 2022-05-20T15:10:23Z

@madebr, I'd like to make an SDL_image release candidate soon. I think this is the only work outstanding before I can do that. Do you have an ETA of having everything settled and ready for public eyes?

smcv · 2022-05-20T15:13:29Z

Is this a blocker for a SDL_image release candidate? At the moment the source code releases aren't going to include CMakeLists.txt anyway.

slouken · 2022-05-20T15:22:46Z

vendored libraries: build these as shared or static libraries? Always building these as static libraries makes perhaps most sense.

For SDL_ttf, using FreeType and HarfBuzz as static libraries makes sense because they are core pieces of the library functionality. For SDL_image and SDL_mixer, the codecs are all optional pieces that can be used, or not, depending on an application's needs.

For pre-built binary redistributables like DLLs for Windows or Frameworks for macOS, I like distributing the dependencies as optional shared libraries that can be present or not, depending on the application they're shipped with.

For building with Linux distributions, it makes sense to build in all supported codecs with dependencies on other codec libraries in the system. Distributions have often chosen to depend on those packages rather than have them be optional.

For building with CMake, this could either be a developer building a custom library for their own needs, or a Linux distribution choosing to use cmake instead of autotools. In the former case, it probably makes sense to use vendored libraries linked statically and defaulting off. In the latter case, the distribution maintainers will set whatever flags makes sense for the distribution.

For Android.mk, which is the build system most closely analogous to CMake, I've chosen to default external dependencies off, and include them as static libraries when they are enabled. I will probably do the same for SDL_mixer for the next release.

slouken · 2022-05-20T15:28:19Z

Is this a blocker for a SDL_image release candidate? At the moment the source code releases aren't going to include CMakeLists.txt anyway.

Yes, I'd like the next SDL library releases to have the improved CMake support.

smcv · 2022-05-20T15:30:13Z

For pre-built binary redistributables like DLLs for Windows or Frameworks for macOS, I like distributing the dependencies as optional shared libraries that can be present or not, depending on the application they're shipped with.

OK, that makes sense. A game bundling your prebuilt SDL_image DLL, but only bundling a subset of your prebuilt dependency DLLs, is a use-case that I hadn't thought about (I'd assumed that everyone who vendors libraries would want to build their own, to have control over their supply chain).

For building with Linux distributions, it makes sense to build in all supported codecs with dependencies on other codec libraries in the system. Distributions have often chosen to depend on those packages rather than have them be optional.

Right, that's what I do in Debian - we have a lot of machinery for managing shared library dependencies and making sure incompatible combinations aren't possible, none of which works when using dlopen(). However, dependencies are "cheap" when you have a system package manager, in a way that they aren't if you have to manage them yourself.

smcv · 2022-05-20T16:22:14Z

#246 sets up the vendored libraries to be built as static libraries if the _SHARED option is not enabled. Does that make sense?

- Use 'SDL2IMAGE_' prefix for all options - Create SDL2_image::SDL2_image when building a shared library, create SDL2_image::SDL2_image-static when building a static library - Use same CMake layout as SDL_ttf/SDL_mixer - Create libSDL2_imaged.so symbolic link (when building debug build type) - Add PrivateSdlFunctions.cmake script for common functionality between SDL_XXX - Add FindXXX.cmake scripts for dependencies that don't provide one - Add CMakeLists.txt + cmake scripts to source distribution - Bring jpeg/png saving support to cmake - Update the git submodules - Test CMake on mingw64 (using system dependencies there) - Add MacOS CI - Create symbolic link at build time + install it - Add Macos versioning - Add single SDL2IMAGE_VENDORED option to enable vendoring of dependencies

madebr · 2022-05-20T16:47:02Z

#246 sets up the vendored libraries to be built as static libraries if the _SHARED option is not enabled. Does that make sense?

This pr does the same.
It makes sense to not let the number of configurable options explode.

madebr · 2022-05-20T16:47:57Z

I'm going to mark this pr as finished so other work can be done on top of it.

madebr marked this pull request as draft May 16, 2022 10:17

madebr mentioned this pull request May 16, 2022

cmake: fixed the use of SDL_image with disabled BUILD_SHARED_LIBS #237

Merged

sezero mentioned this pull request May 17, 2022

Fix for two build errors with CMake #213

Closed

madebr force-pushed the cmake_work branch 4 times, most recently from f491050 to b8f8d1d Compare May 17, 2022 19:51

sezero reviewed May 17, 2022

View reviewed changes

.github/workflows/main.yml Show resolved Hide resolved

madebr force-pushed the cmake_work branch from b8f8d1d to 79df9e6 Compare May 17, 2022 23:43

smcv mentioned this pull request May 19, 2022

CMakeLists.txt never enables saving PNG/JPEG #243

Closed

smcv reviewed May 19, 2022

View reviewed changes

smcv mentioned this pull request May 19, 2022

Add an automated test #242

Merged

slouken pushed a commit that referenced this pull request May 20, 2022

workflows: Run the new automated test for Autotools

734780f

I haven't set it up for CMake yet (although it does work) because that would conflict pretty badly with #238. Signed-off-by: Simon McVittie <smcv@collabora.com>

madebr force-pushed the cmake_work branch 2 times, most recently from 25807ab to ae50092 Compare May 20, 2022 16:43

madebr force-pushed the cmake_work branch from ae50092 to 73bf732 Compare May 20, 2022 16:46

madebr marked this pull request as ready for review May 20, 2022 16:49

slouken merged commit 4ba0cf7 into libsdl-org:main May 20, 2022

madebr deleted the cmake_work branch May 20, 2022 17:25

This was referenced May 20, 2022

cmake: Add build options for whether to support saving PNGs #244

Closed

cmake: Build vendored libraries statically if SHARED not enabled #246

Closed

FtZPetruska mentioned this pull request Jun 17, 2022

[sdl2-image/mixer/net/ttf] Update to exported targets names. microsoft/vcpkg#25284

Closed

Conversation

madebr commented May 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

smcv May 19, 2022

Choose a reason for hiding this comment

Uh oh!

madebr May 20, 2022

Choose a reason for hiding this comment

Uh oh!

smcv May 20, 2022

Choose a reason for hiding this comment

Uh oh!

smcv May 19, 2022

Choose a reason for hiding this comment

Uh oh!

smcv commented May 20, 2022

Uh oh!

smcv commented May 20, 2022

Uh oh!

smcv commented May 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slouken commented May 20, 2022

Uh oh!

smcv commented May 20, 2022

Uh oh!

slouken commented May 20, 2022

Uh oh!

slouken commented May 20, 2022

Uh oh!

smcv commented May 20, 2022

Uh oh!

smcv commented May 20, 2022

Uh oh!

madebr commented May 20, 2022

Uh oh!

madebr commented May 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

madebr commented May 16, 2022 •

edited

Loading

smcv commented May 20, 2022 •

edited

Loading