Modernize cmake file and do cmake-format #1004
Conversation
|
@madler: Can you look this updated PR? |
| @@ -235,3 +235,6 @@ if(NOT SKIP_INSTALL_FILES) | |||
| install(FILES zlib.3 DESTINATION "${CMAKE_INSTALL_MANDIR}/man3") | |||
| install(FILES ${ZLIB_PC} DESTINATION "${ZLIB_INSTALL_PKGCONFIG_DIR}") | |||
| endif() | |||
|
|
|||
| set(CPACK_GENERATOR TGZ) | |||
There was a problem hiding this comment.
The CPack generator shouldn't be set here. Let the consumer decide which method they want to used to package.
There was a problem hiding this comment.
That set only the default and may be changed with cpack -g ...
|
hi folks 👋, another one here hoping to help improve CMake's support on zlib, this PR looks interesting! |
| cmake_minimum_required(VERSION 2.4.4...3.15.0) | ||
| set(CMAKE_ALLOW_LOOSE_LOOP_CONSTRUCTS ON) | ||
| # CMake 3.21 or later is required for PROJECT_IS_TOP_LEVEL | ||
| cmake_minimum_required(VERSION 3.21...3.30) |
There was a problem hiding this comment.
@madler to confirm, but if we have to be able conservative with CMake upgrades we could do:
if (${CMAKE_SOURCE_DIR} STREQUAL ${CMAKE_CURRENT_SOURCE_DIR})
set(PROJECT_IS_TOP_LEVEL TRUE)
endif()
and have the same PROJECT_IS_TOP_LEVEL 👍
There was a problem hiding this comment.
And what minimum Version do you want?
There was a problem hiding this comment.
I think it's very safe to deprecate anything below 3.0.0 (released ten years ago).
3.10.0 is probably "safe enough" and added support to ARM on MSVC.
but of course it's really up to @madler to decide
| add_library(zlib::zlib ALIAS zlib) | ||
| # Note also that CMake's own FindZLIB.cmake module defines a slightly different | ||
| # imported target named ZLIB::ZLIB (the upper/lowercase differences are meaningful). | ||
| add_library(ZLIB::ZLIB ALIAS zlib) |
There was a problem hiding this comment.
Unfortunately this only works with zlib integrated with add_subdirectory or FetchContent... if you compile the lib and install it the generated zlibConfig.cmake is like this (this is from macOS):
#----------------------------------------------------------------
# Generated CMake target import file for configuration "Release".
#----------------------------------------------------------------
# Commands may need to know the format version.
set(CMAKE_IMPORT_FILE_VERSION 1)
# Import target "zlib::zlib" for configuration "Release"
set_property(TARGET zlib::zlib APPEND PROPERTY IMPORTED_CONFIGURATIONS RELEASE)
set_target_properties(zlib::zlib PROPERTIES
IMPORTED_LINK_INTERFACE_LANGUAGES_RELEASE "C"
IMPORTED_LOCATION_RELEASE "${_IMPORT_PREFIX}/lib/libz.a"
)
list(APPEND _cmake_import_check_targets zlib::zlib )
list(APPEND _cmake_import_check_files_for_zlib::zlib "${_IMPORT_PREFIX}/lib/libz.a" )
# Commands beyond this point should not need to know the version.
set(CMAKE_IMPORT_FILE_VERSION)
It won't create the upper case alias, so other targets that try to link to ZLIB::ZLIB won't find it. I don't like these upper case names either, but there is a lot of people out there using them, so I think upper case is the one we should keep.
There was a problem hiding this comment.
May also add an alias in config.cmake.in?
There was a problem hiding this comment.
I like the idea, but personally I think it would make this PR too big. Maybe split the .cmake.in stuff and the github actions related stuff?
| include(CMakePackageConfigHelpers) | ||
|
|
||
| set(ZLIB_INSTALL_CMAKEDIR "${CMAKE_INSTALL_LIBDIR}/cmake/zlib") | ||
| set(ZLIB_INSTALL_PKGCONFIG_DIR "${CMAKE_INSTALL_LIBDIR}/share/pkgconfig") |
There was a problem hiding this comment.
this ends up making [install dir]/lib/share/pkgconfig/zlib.pc, I think it should be [install dir]/share/pkgconfig/zlib.pc
| set(INSTALL_BIN_DIR "${CMAKE_INSTALL_PREFIX}/bin" CACHE PATH "Installation directory for executables") | ||
| set(INSTALL_LIB_DIR "${CMAKE_INSTALL_PREFIX}/lib" CACHE PATH "Installation directory for libraries") | ||
| set(INSTALL_INC_DIR "${CMAKE_INSTALL_PREFIX}/include" CACHE PATH "Installation directory for headers") | ||
| set(INSTALL_MAN_DIR "${CMAKE_INSTALL_PREFIX}/share/man" CACHE PATH "Installation directory for manual pages") | ||
| set(INSTALL_PKGCONFIG_DIR "${CMAKE_INSTALL_PREFIX}/share/pkgconfig" CACHE PATH "Installation directory for pkgconfig (.pc) files") |
There was a problem hiding this comment.
it's ok to remove these, but then zlib.pc.cmakein should be updated to:
prefix=@CMAKE_INSTALL_PREFIX@
exec_prefix=@CMAKE_INSTALL_PREFIX@
libdir=${prefix}/lib
sharedlibdir=${prefix}/lib
includedir=${prefix}/include
Name: zlib
Description: zlib compression library
Version: @VERSION@
Requires:
Libs: -L${libdir} -L${sharedlibdir} -lz
Cflags: -I${includedir}
| cmake_minimum_required(VERSION 2.4.4...3.15.0) | ||
| set(CMAKE_ALLOW_LOOSE_LOOP_CONSTRUCTS ON) | ||
| # CMake 3.21 or later is required for PROJECT_IS_TOP_LEVEL | ||
| cmake_minimum_required(VERSION 3.21...3.30) |
There was a problem hiding this comment.
I think it's very safe to deprecate anything below 3.0.0 (released ten years ago).
3.10.0 is probably "safe enough" and added support to ARM on MSVC.
but of course it's really up to @madler to decide
|
My intention was to help, but now I am going to lose my patience |
|
@madler: What do you think? People close PR because no activity from you :/ It is important to look all CMAKE PRs... |
|
@ClausKlein: Can you check latest comments of this ticket? |
No description provided.