Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add missing options to cmake script #189

Merged
merged 1 commit into from May 10, 2022
Merged

Conversation

madebr
Copy link
Contributor

@madebr madebr commented Aug 28, 2021

don't merge yet

Known issues that need to be fixed before merging can occur:

  • cmake cannot build with vendored dependencies: Add missing options to cmake script #189 (comment)
  • on mingw with dynamic libraries + system libraries, the cmake script uses the import library instead of the dll
  • on Linux with dynamic libraries, the cmake script might select the shared library with the "longest version name". It should
    e.g. for libpng my system packages provides:
    /usr/lib64/libpng16.so -> libpng16.so.16.37.0
    /usr/lib64/libpng16.so.16 -> libpng16.so.16.37.0
    /usr/lib64/libpng16.so.16.37.0
    
    Because /usr/lib64/libpng16.so links to libpng16.so.16.37.0, it selects that one (and /usr/lib64/libpng16.16 is ignored).
  • consider testing CMake on mingw, as demonstrated in Add missing options to cmake script #189 (comment)

This pr improves (imho) the cmake build script by:

  • adding options for enabling/disabling each loader
  • allow switching between vendored and system libraries for jpeg/libpng/libwebp/libtiff/zlib
  • make the file names of the created libraries/files in a prefix the same
  • installs a SDL2_image-config.cmake file to enable cmake users to do find_package(SDL2_image CONFIG) and use a SDL2::SDL2_image target.
  • for compatibility, also create a SDL2_image.pc file
  • I think this provides a better solution then Adding PIC for compiling with cmake on Unix-like #187 to add PIC => I have added a BUILD_PIC option. (Adding PIC for compiling with cmake on Unix-like #187 was caused by libjpeg building a static library without PIC, which cannot be used inside a shared library)
  • adds support for dynamically loading libraries (instead of linking to them at build time)

See #189 (comment) for current issues.

When building using system dependencies, the install prefix looks like:

prefix
├── include
│   └── SDL2
│       └── SDL_image.h
├── lib64
│   ├── cmake
│   │   └── SDL2_image
│   │       ├── SDL2_image-config.cmake
│   │       ├── SDL2_image-targets.cmake
│   │       └── SDL2_image-targets-noconfig.cmake
│   ├── libSDL2_image-2.0.so -> libSDL2_image-2.0.so.0
│   ├── libSDL2_image-2.0.so.0 -> libSDL2_image-2.0.so.0.6.0
│   ├── libSDL2_image-2.0.so.0.6.0
│   ├── libSDL2_image.so -> libSDL2_image-2.0.so
│   └── pkgconfig
│       └── SDL2_image.pc
└── share
    └── licenses
        └── SDL2_image
            └── LICENSE.txt

9 directories, 10 files

When building+installing all dependencies, the install prefix looks like:

prefix
├── include
│   └── SDL2
│       └── SDL_image.h
├── lib64
│   ├── cmake
│   │   └── SDL2_image
│   │       ├── SDL2_image-config.cmake
│   │       ├── SDL2_image-targets.cmake
│   │       └── SDL2_image-targets-noconfig.cmake
│   ├── libjpeg.so
│   ├── libpng16.so -> libpng16.so.16
│   ├── libpng16.so.16 -> libpng16.so.16.37.0
│   ├── libpng16.so.16.37.0
│   ├── libSDL2_image-2.0.so -> libSDL2_image-2.0.so.0
│   ├── libSDL2_image-2.0.so.0 -> libSDL2_image-2.0.so.0.6.0
│   ├── libSDL2_image-2.0.so.0.6.0
│   ├── libSDL2_image.so -> libSDL2_image-2.0.so
│   ├── libtiff.so -> libtiff.so.5
│   ├── libtiff.so.5 -> libtiff.so.5.6.0
│   ├── libtiff.so.5.6.0
│   ├── libz.so -> libz.so.1
│   ├── libz.so.1 -> libz.so.1.2.11
│   ├── libz.so.1.2.11
│   └── pkgconfig
│       └── SDL2_image.pc
└── share
    └── licenses
        └── SDL2_image
            └── LICENSE.txt

9 directories, 20 files

The prefix for a shared mingw build:

prefix
├── bin
│   └── SDL2_image.dll
├── include
│   └── SDL2
│       └── SDL_image.h
├── lib
│   ├── cmake
│   │   └── SDL2_image
│   │       ├── SDL2_image-config.cmake
│   │       ├── SDL2_image-targets.cmake
│   │       └── SDL2_image-targets-noconfig.cmake
│   ├── libSDL2_image.dll.a
│   └── pkgconfig
│       └── SDL2_image.pc
└── share
    └── licenses
        └── SDL2_image
            └── LICENSE.txt

10 directories, 8 files

Tested on gcc@Linux and mingw@Linux.

Some feedback would be appreciated.

TODO:

  • Compatibility with Cmake's FindSDL_image? Or it that one only looking for 1.x?

@madebr
Copy link
Contributor Author

madebr commented Sep 28, 2021

I just tested with Visual Studio 16 2019, which results in the following install prefix:

.
├── bin
│   └── SDL2_image.dll
├── include
│   └── SDL2
│       └── SDL_image.h
├── lib
│   ├── cmake
│   │   └── SDL2_image
│   │       ├── SDL2_image-config.cmake
│   │       ├── SDL2_image-targets.cmake
│   │       └── SDL2_image-targets-release.cmake
│   ├── pkgconfig
│   │   └── SDL2_image.pc
│   └── SDL2_image.lib
└── share
    └── licenses
        └── SDL2_image
            └── LICENSE.txt

10 directories, 10 files

@Wolf1098
Copy link

Wolf1098 commented Oct 2, 2021

Imho the term vendor used here is a bit ambiguous, it could cause inadvertent confusion, package-maintainer bundled libs or something would be a better term

@Wolf1098
Copy link

Wolf1098 commented Oct 2, 2021

and also be very careful with visibility... see https://gcc.gnu.org/wiki/Visibility - Problems with C++ exceptions (please read!) and potentially opencv/opencv#7565

@Wolf1098
Copy link

Wolf1098 commented Oct 2, 2021

would also recommend keeping more of the inline comments, makes it easier to track the changes

@madebr
Copy link
Contributor Author

madebr commented Oct 2, 2021

Imho the term vendor used here is a bit ambiguous, it could cause inadvertent confusion, package-maintainer bundled libs or something would be a better term

package-maintainer is another thing.
dnf/yum/apt-get are package managers that provide packages.
Here, we're adding an option to not use package managers and use a private library.

Perhaps you're right that _VENDORED is not a correct term as we don't modify the external libraries.
But I don't know a better naming for it.. So please do a suggestion.

and also be very careful with visibility... see https://gcc.gnu.org/wiki/Visibility - Problems with C++ exceptions (please read!) and potentially opencv/opencv#7565

SDL_image is a pure c library without a c++ interface.
I don't think the section about c++ exceptions applies to it.
SDL_image also only used the c interface of the 3rd party libraries.
No c++ Exception should ever be visible to the user of this library.

@madebr
Copy link
Contributor Author

madebr commented Oct 2, 2021

would also recommend keeping more of the inline comments, makes it easier to track the changes

You mean adding comments to the cmake script?
There were none before, so there is nothing to keep.

Btw, does this cmake script work on your machine and build for your targets?

@slouken
Copy link
Collaborator

slouken commented May 9, 2022

Can you rebase this on the latest code? Does it make sense to make similar changes to the SDL_mixer and SDL_ttf libraries?

Thanks!

@madebr
Copy link
Contributor Author

madebr commented May 9, 2022

The creation of this pr was inspired by the lack of configuration options to enable/disable the various backends + use system packages.
I briefly looked at sdl_mixer and sdl_ttf and think both would benefit by receiving a similar makeover:

The CI failure is because not all dependencies of sdl_image can be used as a subproject without change.
Because current master has added the external dependencies as git submodules (instead of vendored as it did before), these cannot get patched easily to fix little cmake problems.

I just found out all dependencies are vendored as github project in the github libsdl-org organization, so I am able to patch those without bothering upstream.
This won't be possible for the dependencies of libjxl, as that one has an extra set of dependencies: [deps.sh](https://github.com/libsdl-org/libjxl/blob/main/deps.sh).
Of those, brotli and highway need an extra patch.

The following patches need to be applied to fix the dependencies (+dependencies of dependencies):

brotli.patch

diff --git a/CMakeLists.txt b/CMakeLists.txt
index a8ea872..dc6d092 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -175,14 +175,14 @@ endforeach()
 
 foreach(lib IN LISTS BROTLI_SHARED_LIBS BROTLI_STATIC_LIBS)
   target_link_libraries(${lib} ${LIBM_LIBRARY})
-  set_property(TARGET ${lib} APPEND PROPERTY INCLUDE_DIRECTORIES ${BROTLI_INCLUDE_DIRS})
+  set_property(TARGET ${lib} APPEND PROPERTY INCLUDE_DIRECTORIES "$<BUILD_INTERFACE:${BROTLI_INCLUDE_DIRS}>")
   set_target_properties(${lib} PROPERTIES
     VERSION "${BROTLI_ABI_COMPATIBILITY}.${BROTLI_ABI_AGE}.${BROTLI_ABI_REVISION}"
     SOVERSION "${BROTLI_ABI_COMPATIBILITY}")
   if(NOT BROTLI_EMSCRIPTEN)
     set_target_properties(${lib} PROPERTIES POSITION_INDEPENDENT_CODE TRUE)
   endif()
-  set_property(TARGET ${lib} APPEND PROPERTY INTERFACE_INCLUDE_DIRECTORIES "${BROTLI_INCLUDE_DIRS}")
+  set_property(TARGET ${lib} APPEND PROPERTY INTERFACE_INCLUDE_DIRECTORIES "$<BUILD_INTERFACE:${BROTLI_INCLUDE_DIRS}>")
 endforeach()
 
 if(NOT BROTLI_EMSCRIPTEN)

jpeg.patch

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 3b49a85..34d487f 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -1,7 +1,7 @@
 cmake_minimum_required(VERSION 3.1)
 project(jpeg C)
 
-add_library(jpeg STATIC)
+add_library(jpeg)
 target_sources(jpeg PRIVATE
 		jaricom.c jcapimin.c jcapistd.c jcarith.c jccoefct.c jccolor.c
 		jcdctmgr.c jchuff.c jcinit.c jcmainct.c jcmarker.c jcmaster.c
@@ -26,4 +26,4 @@ target_sources(jpeg PRIVATE jidctint.c jidctfst.c)
 
 target_compile_definitions(jpeg PRIVATE -DAVOID_TABLES)
 
-target_include_directories(jpeg PUBLIC .)
+target_include_directories(jpeg PUBLIC $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}>)

libhighway.patch

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 69a8e61..0ed776b 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -167,7 +167,7 @@ endif()  # !MSVC
 add_library(hwy STATIC ${HWY_SOURCES})
 target_compile_options(hwy PRIVATE ${HWY_FLAGS})
 set_property(TARGET hwy PROPERTY POSITION_INDEPENDENT_CODE ON)
-target_include_directories(hwy PUBLIC ${CMAKE_CURRENT_LIST_DIR})
+target_include_directories(hwy PUBLIC "$<BUILD_INTERFACE:${CMAKE_CURRENT_LIST_DIR}>")
 
 add_library(hwy_contrib STATIC ${HWY_CONTRIB_SOURCES})
 target_compile_options(hwy_contrib PRIVATE ${HWY_FLAGS})

libjxl.patch

diff --git a/lib/jxl.cmake b/lib/jxl.cmake
index c0290ab..3c0c6c6 100644
--- a/lib/jxl.cmake
+++ b/lib/jxl.cmake
@@ -500,8 +500,9 @@ target_link_libraries(jxl PUBLIC ${JPEGXL_COVERAGE_FLAGS})
 target_link_libraries(jxl PRIVATE ${JPEGXL_INTERNAL_SHARED_LIBS})
 # Shared library include path contains only the "include/" paths.
 target_include_directories(jxl PUBLIC
-  "${CMAKE_CURRENT_SOURCE_DIR}/include"
-  "${CMAKE_CURRENT_BINARY_DIR}/include")
+  "$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>"
+  "$<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}/include>"
+  "$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>")
 set_target_properties(jxl PROPERTIES
   VERSION ${JPEGXL_LIBRARY_VERSION}
   SOVERSION ${JPEGXL_LIBRARY_SOVERSION}
diff --git a/third_party/brotli b/third_party/brotli
--- a/third_party/brotli
+++ b/third_party/brotli
@@ -1 +1 @@
-Subproject commit 35ef5c554d888bef217d449346067de05e269b30
+Subproject commit 35ef5c554d888bef217d449346067de05e269b30-dirty
diff --git a/third_party/skcms.cmake b/third_party/skcms.cmake
index 61b1ca5..f518c10 100644
--- a/third_party/skcms.cmake
+++ b/third_party/skcms.cmake
@@ -32,10 +32,10 @@ if(JPEGXL_BUNDLE_SKCMS)
   target_compile_options(skcms-obj PRIVATE -DJPEGXL_BUNDLE_SKCMS=1)
   if(MSVC)
     target_compile_options(skcms-obj
-      PRIVATE /FI${CMAKE_SOURCE_DIR}/lib/jxl/enc_jxl_skcms.h)
+      PRIVATE /FI${PROJECT_SOURCE_DIR}/lib/jxl/enc_jxl_skcms.h)
   else()
     target_compile_options(skcms-obj
-      PRIVATE -include${CMAKE_SOURCE_DIR}/lib/jxl/enc_jxl_skcms.h)
+      PRIVATE -include${PROJECT_SOURCE_DIR}/lib/jxl/enc_jxl_skcms.h)
   endif()
 endif()
 

libtiff.patch

diff --git a/CMakeLists.txt b/CMakeLists.txt
index efe857df..5cc32d20 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -680,7 +680,7 @@ install(FILES ${CMAKE_CURRENT_BINARY_DIR}/libtiff-4.pc
 
 # Includes used by libtiff (and tests)
 if(ZLIB_INCLUDE_DIRS)
-  list(APPEND TIFF_INCLUDES ${ZLIB_INCLUDE_DIRS})
+  list(APPEND TIFF_INCLUDES $<BUILD_INTERFACE:${ZLIB_INCLUDE_DIRS}>)
 endif()
 if(DEFLATE_INCLUDE_DIR)
   list(APPEND TIFF_INCLUDES ${DEFLATE_INCLUDE_DIR})
diff --git a/libtiff/CMakeLists.txt b/libtiff/CMakeLists.txt
index 080685db..0c1607ae 100644
--- a/libtiff/CMakeLists.txt
+++ b/libtiff/CMakeLists.txt
@@ -116,9 +116,9 @@ add_library(tiff ${tiff_SOURCES} ${tiff_HEADERS} ${nodist_tiff_HEADERS}
             ${tiff_port_SOURCES} libtiff.def)
 target_include_directories(tiff
     PUBLIC
-        $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}>
-        $<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}>
-        ${TIFF_INCLUDES}
+        "$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}>"
+        "$<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}>"
+        "$<BUILD_INTERFACE:${TIFF_INCLUDES}>"
 )
 target_link_libraries(tiff ${TIFF_LIBRARY_DEPS})
 set_target_properties(tiff PROPERTIES SOVERSION ${SO_COMPATVERSION})

Some of these patches might not be needed when we'ld use ExternalProject instead of add_subdirectory. But I have never used this command before, so I don't know its limits.

@madebr
Copy link
Contributor Author

madebr commented May 9, 2022

@madebr madebr force-pushed the cmake_work branch 2 times, most recently from 72b6b9a to bc942d7 Compare May 9, 2022 20:23
CMakeLists.txt Outdated Show resolved Hide resolved
@madebr
Copy link
Contributor Author

madebr commented May 10, 2022

When allowing system libraries on CI, the build succeeds: https://github.com/madebr/SDL_image/actions/runs/2300423858

CMakeLists.txt Outdated
@@ -139,7 +139,7 @@ option(SUPPORT_XPM "Support loading XPM images" ON)
option(SUPPORT_XV "Support loading XV images" ON)
option(SUPPORT_WEBP "Support loading WEBP images" OFF)

option(BUILD_SHOWIMAGE "Build the showimage sample program" OFF)
option(BUILD_PROGRAMS "Build the SDL2_image sample program" ON)
Copy link

Choose a reason for hiding this comment

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

Maybe rename the option to reflect that those are samples and not tools?

@slouken
Copy link
Collaborator

slouken commented May 10, 2022

Sorry, I was poking around in CMakeLists.txt without this PR in mind. Can you rebase your changes, and I'll start working on the upstream patches?

@slouken
Copy link
Collaborator

slouken commented May 10, 2022

Also, please feel free to submit a similar makeover for SDL_ttf and SDL_mixer. I won't touch those CMakeLists.txt until you're done with them.

slouken added a commit that referenced this pull request May 10, 2022
This reverts commit 600a331.

This is in favor of #189
@slouken slouken merged commit f9a635c into libsdl-org:main May 10, 2022
@slouken
Copy link
Collaborator

slouken commented May 10, 2022

I've merged your changes and flipped STB on by default, and disabled AVIF, JXL, and WEBP by default, to match default loaders built on other platforms.

What needs to be done for the upstream changes you have listed above?

@madebr
Copy link
Contributor Author

madebr commented May 11, 2022

For SDL_image, the following needs to be done:

The following libraries are dependencies of libjxl. I haven't checked thoroughly whether they are really needed, or whether they can be disabled.
But both uses seem plausible (libhighway for vectorization and and brotli for compression)

I created a pull request for all issues. So apart from awaiting reviews & hoping for approval, nothing should be done (I think).

@madebr madebr deleted the cmake_work branch May 11, 2022 00:17
@slouken
Copy link
Collaborator

slouken commented May 11, 2022

For SDL_image, the following needs to be done:

@sezero, can you investigate what we need to do to update to v4.3.0?

The following libraries are dependencies of libjxl. I haven't checked thoroughly whether they are really needed, or whether they can be disabled. But both uses seem plausible (libhighway for vectorization and and brotli for compression)

I created a pull request for all issues. So apart from awaiting reviews & hoping for approval, nothing should be done (I think).

Okay, libhighway and brotli are both needed for libjxl. We can wait a bit and see if those patches will be accepted. If not, we can fork them and update the version we pull from libsdl-org.

@madebr madebr restored the cmake_work branch May 11, 2022 01:02
@madebr madebr deleted the cmake_work branch May 11, 2022 01:02
@sezero
Copy link
Contributor

sezero commented May 11, 2022

For SDL_image, the following needs to be done:

@sezero, can you investigate what we need to do to update to v4.3.0?

Can't we just apply the patch shown above?

@madebr
Copy link
Contributor Author

madebr commented May 11, 2022

Can't we just apply the patch shown above?

For the purpose of CMake install, sure.

@sezero
Copy link
Contributor

sezero commented May 11, 2022

Can't we just apply the patch shown above?

For the purpose of CMake install, sure.

What else purpose do we really need libtiff-4.3.0?
(4.2.0 to 4.3.0 changes are practially only deprecation of some libtiff-
defined types in favor of stdint.h types and inttypes.h functionality.)

@slouken
Copy link
Collaborator

slouken commented May 11, 2022

Can't we just apply the patch shown above?

For the purpose of CMake install, sure.

Done!

@slouken
Copy link
Collaborator

slouken commented May 11, 2022

@madebr, are you planning on making similar changes for SDL_ttf and SDL_mixer?

@sezero
Copy link
Contributor

sezero commented May 12, 2022

string(JOIN ...) seems to require cmake >= 3.12. We should either bump our
minimum required cmake or find a replacement solution for it.

@madebr
Copy link
Contributor Author

madebr commented May 12, 2022

We need at least cmake 3.14 to support the generator expressions in install(CODE).
I bumped SDL_ttf's version to that.
SDL_image needs the same change.

@sezero
Copy link
Contributor

sezero commented May 12, 2022

OK, just bumped it to 3.14 in SDL_image too.

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.

None yet

5 participants