Address deprecated cmake version warning.#1812
Conversation
WalkthroughThe changes involve updates to multiple Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CMake
participant BuildSystem
User->>CMake: Configure project
CMake->>BuildSystem: Set minimum version (3.5.1...3.29.0)
CMake->>BuildSystem: Set C standard (C11)
CMake->>BuildSystem: Add options (WITH_GZFILEOP, ZLIB_COMPAT, ...)
CMake->>BuildSystem: Check architecture-specific features
CMake->>BuildSystem: Install headers and libraries
BuildSystem-->>User: Build complete
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
test/fuzz/CMakeLists.txt (1)
Line range hint
42-49: Consider modern CMake practices for test configurationWhile the test configuration works, consider these improvements for better maintainability:
- Replace
file(GLOB)with explicit test file lists to ensure deterministic builds- Consider using
add_test(NAME ... COMMAND ... WORKING_DIRECTORY ...)for better test isolationHere's a suggested improvement:
- file(GLOB FUZZER_TEST_FILES ${PROJECT_SOURCE_DIR}/*) - set(FUZZER_COMMAND ${CMAKE_CROSSCOMPILING_EMULATOR} $<TARGET_FILE:${FUZZER}> ${FUZZER_TEST_FILES}) - add_test(NAME ${FUZZER} COMMAND ${FUZZER_COMMAND}) + # Define test files explicitly + set(FUZZER_TEST_FILES + ${PROJECT_SOURCE_DIR}/test1.txt + ${PROJECT_SOURCE_DIR}/test2.txt + ) + add_test(NAME ${FUZZER} + COMMAND ${CMAKE_CROSSCOMPILING_EMULATOR} $<TARGET_FILE:${FUZZER}> ${FUZZER_TEST_FILES} + WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR})CMakeLists.txt (2)
Line range hint
11-24: Consider improving error message formattingThe C standard configuration is well-implemented, but the error message formatting could be improved for better readability.
- MESSAGE(FATAL_ERROR "CMAKE_C_STANDARD:STRING=${CMAKE_C_STANDARD} not in known standards list\n ${VALID_C_STANDARDS}") + message(FATAL_ERROR "CMAKE_C_STANDARD:STRING=${CMAKE_C_STANDARD} not in known standards list:\n ${VALID_C_STANDARDS}")
Line range hint
26-35: Enhance version parsing robustnessThe version parsing could benefit from error handling to catch cases where the regex patterns don't match.
file(READ ${CMAKE_CURRENT_SOURCE_DIR}/zlib.h.in _zlib_h_contents) string(REGEX REPLACE ".*#define[ \t]+ZLIB_VERSION[ \t]+\"([0-9]+.[0-9]+.[0-9]+).*\".*" "\\1" ZLIB_HEADER_VERSION ${_zlib_h_contents}) +if(NOT ZLIB_HEADER_VERSION) + message(FATAL_ERROR "Failed to extract ZLIB_VERSION from zlib.h.in") +endif() string(REGEX REPLACE ".*#define[ \t]+ZLIBNG_VERSION[ \t]+\"([-0-9A-Za-z.]+)\".*" "\\1" ZLIBNG_HEADER_VERSION ${_zlib_h_contents}) +if(NOT ZLIBNG_HEADER_VERSION) + message(FATAL_ERROR "Failed to extract ZLIBNG_VERSION from zlib.h.in") +endif()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
CMakeLists.txt(1 hunks)test/add-subdirectory-project/CMakeLists.txt(1 hunks)test/fuzz/CMakeLists.txt(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/add-subdirectory-project/CMakeLists.txt
🔇 Additional comments (4)
test/fuzz/CMakeLists.txt (2)
Line range hint 3-41: Verify fuzzing configuration with newer CMake versions
While the fuzzing configuration looks solid, since we're now explicitly supporting CMake versions up to 3.29.0, we should verify that:
- The find_library behavior remains consistent across versions
- The target_link_libraries usage is optimal for modern CMake
Let's check for any potential improvements available in newer CMake versions:
✅ Verification successful
Based on the search results, I can now generate the final response:
Fuzzing configuration is compatible with modern CMake versions
The current fuzzing configuration follows modern CMake practices and is compatible with the supported CMake version range:
- The
find_libraryusage for FuzzingEngine is appropriate as it's an external dependency - The codebase consistently uses modern CMake target-based commands
- Target visibility specifiers (PRIVATE/PUBLIC) are properly used throughout the codebase where needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for modern CMake practices in fuzzing configuration
# Expected: Identify if we can leverage newer CMake features
# Check for any find_library usage patterns that could be improved
rg "find_library\(" --type cmake
# Look for target_link_libraries patterns
rg "target_link_libraries\(" --type cmake -A 2
# Check for any existing INTERFACE, PUBLIC, or PRIVATE visibility specifiers
rg "(INTERFACE|PUBLIC|PRIVATE)" --type cmake
Length of output: 7087
1-1: LGTM: CMake version range update looks correct.
The update to use the version range syntax 3.5.1...3.29.0 aligns with the PR objective and follows CMake's recommended practice. This change:
- Maintains backward compatibility with CMake 3.5.1
- Sets policies up to CMake 3.29.0
- Addresses the deprecated version warning
Let's verify the CMake policy behavior:
CMakeLists.txt (2)
1-4: Well-implemented CMake version range!
The change from a single version to a version range 3.5.1...3.29.0 effectively addresses the deprecated CMake version warnings while maintaining backward compatibility. The fallback for CMake versions below 3.12 is handled correctly.
Line range hint 6-9: Good policy management!
The conditional setting of CMP0169 to OLD is well-implemented, ensuring backward compatibility while safely handling cases where the policy doesn't exist in older CMake versions.
| cmake_minimum_required(VERSION 3.5.1) | ||
| cmake_minimum_required(VERSION 3.5.1...3.29.0) | ||
| if(CMAKE_VERSION VERSION_LESS 3.12) | ||
| cmake_policy(VERSION ${CMAKE_VERSION}) |
There was a problem hiding this comment.
I think this is not necessary with minimum version specified in x...y format.
There was a problem hiding this comment.
From: https://cmake.org/cmake/help/latest/command/cmake_minimum_required.html
"If the running version of CMake is older than 3.12, the extra ... dots will be seen as version component separators, resulting in the ... part being ignored and preserving the pre-3.12 behavior of basing policies on ."
There was a problem hiding this comment.
Also, it might make sense to start requiring CMake 3.12 as a minimum? Up to maintainers here.
There was a problem hiding this comment.
We test zlib-ng on wide range of Linux versions and some of them don't have cmake 3.12... We avoid adding unnecessary binary packages or recompiling from sources as the current CI workflows are already pushing limits of the runners and any unnecessary additions would cause unnecessary failure. People say building newer cmake works if glibc is at least some version without actually checking what glibc version is used on each distribution and release... I've seen more than once that the bundled version is one less than what people assume it would be.
It is out of scope of each PR to repeat all the unfixed bugs in new releases when we have already downgraded CI instances to use older releases that actually can build zlib-ng successfully.
There was a problem hiding this comment.
This change is compatible with versions of cmake before 3.12. Jus to be clear.
|
A few questions; To be clear, I ask out of ignorance, not because I think anything is actually wrong. I think that this PR would be better for backwards compatibility, so unless someone has objections I will probably be merging this one instead of #1810. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1812 +/- ##
============================================
+ Coverage 32.15% 77.75% +45.60%
============================================
Files 67 133 +66
Lines 5754 10253 +4499
Branches 1239 2660 +1421
============================================
+ Hits 1850 7972 +6122
+ Misses 3644 1278 -2366
- Partials 260 1003 +743 ☔ View full report in Codecov by Sentry. |
The intention of this patch was to address the warning and not change behavior. CMake version >3.29 will still work with the CMake code but the new changes in behavior or policies of CMake will not be in effect. This preserved the current behavior of CMake and makiesthis PR a bug fix and not change in behavior.
This CMake file had not policies applied. I did not investigate further to determine what would be correct behavior of the file.
|
|
Are the failing builds chronic? Does the PR need to be rebased onto master? |
The S390X builds are down and requires some manual work to get it work again after a long pause where a working github actions runner was unavailable on S390X. The rest would hopefully be fixed with a rebase. |
Use cmake_minimum_required(VERSION <min>...<policy_max>) syntax to set the policy at the same time as the compatibile CMake version.
d9e5714 to
53ce99e
Compare
Use cmake_minimum_required(VERSION ...<policy_max>) syntax to set the policy at the same time as the compatibile CMake version.
The CMake documentation for this call is here:
https://cmake.org/cmake/help/latest/command/cmake_minimum_required.html
Of note is the following:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation