Conversation
WalkthroughReplaces legacy Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User / CMake cache
participant CMake as Top-level CMakeLists.txt
participant CTest as CTest / BUILD_TESTING
participant Tests as test CMake & sources
Note over User,CMake: option resolution
User->>CMake: may set `ZLIB_ENABLE_TESTS` or `BUILD_TESTING`
CMake->>CMake: if(DEFINED ZLIB_ENABLE_TESTS) map to `BUILD_TESTING` and emit deprecation
CMake->>CMake: include(CTest)
Note over CMake,CTest: test gating decision
alt BUILD_TESTING = ON
CMake->>Tests: enable `WITH_GTEST` / `WITH_FUZZERS` / `WITH_BENCHMARKS` / `WITH_BENCHMARK_APPS`
Tests-->>CMake: configure and register test targets (including fuzzers)
else
CMake-->>Tests: test-related options disabled
end
Note over Tests: source-level selection
CMake->>Tests: may define `TEST_STOCK_ZLIB`
alt TEST_STOCK_ZLIB defined
Tests-->>Tests: exclude zlib-ng-only coverage/inflate-table tests
else
Tests-->>Tests: include zlib-ng test paths
end
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CMakeLists.txt(5 hunks)README.md(3 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: mtl1979
Repo: zlib-ng/zlib-ng PR: 1925
File: arch/loongarch/lasxintrin_ext.h:38-65
Timestamp: 2025-07-04T16:59:44.725Z
Learning: In zlib-ng, the policy for supporting old compilers is to maintain compatibility for up to two years after GitHub removes support for them in CI, or when there is no long-time contributor left testing for the architecture if the architecture was not supported by GitHub Actions at all.
Learnt from: mtl1979
Repo: zlib-ng/zlib-ng PR: 1980
File: compress.c:31-33
Timestamp: 2025-10-12T17:01:13.630Z
Learning: In zlib-ng, when ZLIB_COMPAT is defined, z_int32_t is typedef'd as `int` (not int32_t), and zlib.h is included (which uses `int` in function signatures). This allows the implementation to use z_int32_t while maintaining backwards compatibility with the original zlib API that uses `int`. When ZLIB_COMPAT is not defined, z_int32_t is typedef'd as int32_t and zlib-ng.h is used instead.
📚 Learning: 2025-02-21T01:41:50.358Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:14-24
Timestamp: 2025-02-21T01:41:50.358Z
Learning: In zlib-ng's SSE2 vectorized Chorba CRC implementation, the code that calls READ_NEXT macro ensures 16-byte alignment, making explicit alignment checks unnecessary within the macro.
Applied to files:
README.md
📚 Learning: 2025-07-04T16:59:44.725Z
Learnt from: mtl1979
Repo: zlib-ng/zlib-ng PR: 1925
File: arch/loongarch/lasxintrin_ext.h:38-65
Timestamp: 2025-07-04T16:59:44.725Z
Learning: In zlib-ng, the policy for supporting old compilers is to maintain compatibility for up to two years after GitHub removes support for them in CI, or when there is no long-time contributor left testing for the architecture if the architecture was not supported by GitHub Actions at all.
Applied to files:
README.mdCMakeLists.txt
📚 Learning: 2025-10-12T17:01:13.630Z
Learnt from: mtl1979
Repo: zlib-ng/zlib-ng PR: 1980
File: compress.c:31-33
Timestamp: 2025-10-12T17:01:13.630Z
Learning: In zlib-ng, when ZLIB_COMPAT is defined, z_int32_t is typedef'd as `int` (not int32_t), and zlib.h is included (which uses `int` in function signatures). This allows the implementation to use z_int32_t while maintaining backwards compatibility with the original zlib API that uses `int`. When ZLIB_COMPAT is not defined, z_int32_t is typedef'd as int32_t and zlib-ng.h is used instead.
Applied to files:
README.mdCMakeLists.txt
📚 Learning: 2025-04-15T09:20:52.333Z
Learnt from: mtl1979
Repo: zlib-ng/zlib-ng PR: 1904
File: arch/riscv/Makefile.in:12-14
Timestamp: 2025-04-15T09:20:52.333Z
Learning: Optional feature flags in Makefile.in templates should default to empty strings. The configure script should detect feature support and update these variables accordingly during the configuration process.
Applied to files:
CMakeLists.txt
📚 Learning: 2025-04-15T09:30:10.081Z
Learnt from: mtl1979
Repo: zlib-ng/zlib-ng PR: 1904
File: arch/riscv/Makefile.in:12-14
Timestamp: 2025-04-15T09:30:10.081Z
Learning: Feature detection modules like riscv_features.c should not be compiled with feature-specific flags (like RVVFLAG) because they need to be compilable on all systems regardless of feature support. These modules perform runtime detection and must initialize feature availability flags to zero on unsupported systems.
Applied to files:
CMakeLists.txt
🪛 LanguageTool
README.md
[uncategorized] ~103-~103: Do not mix variants of the same word (‘optimisation’ and ‘optimization’) within a single text.
Context: ...| --without-optimizations | Build with optimisations ...
(EN_WORD_COHERENCY)
[uncategorized] ~211-~211: Do not mix variants of the same word (‘optimisation’ and ‘optimization’) within a single text.
Context: ...ut-power8 | ppc: Build with POWER8 optimisations ...
(EN_WORD_COHERENCY)
[uncategorized] ~212-~212: Do not mix variants of the same word (‘optimisation’ and ‘optimization’) within a single text.
Context: ...ut-power9 | ppc: Build with POWER9 optimisations ...
(EN_WORD_COHERENCY)
[uncategorized] ~218-~218: Do not mix variants of the same word (‘optimisation’ and ‘optimization’) within a single text.
Context: ... | loongarch: Build with LSX optimisations ...
(EN_WORD_COHERENCY)
🔇 Additional comments (6)
CMakeLists.txt (3)
51-57: Backward compatibility logic is clean.The introduction of ZLIBNG_ENABLE_TESTS as the canonical option with propagation from ZLIB_ENABLE_TESTS correctly implements the consolidation intent. The dependent options (WITH_GTEST, WITH_FUZZERS, WITH_BENCHMARKS) are properly updated to gate on the new option.
1577-1594: Test gating correctly switches to ZLIBNG_ENABLE_TESTS.The conditional logic properly gates test and benchmark builds on the unified option while enforcing the constraint that tests are incompatible with shared libraries.
1617-1617: Architecture-specific feature info entries correctly added.The new feature info entries for WITH_ARMV6 and WITH_CRC32_VX follow the established pattern and properly expose these architecture-specific options in the feature summary.
Also applies to: 1628-1628
README.md (3)
22-26: Feature bullets accurately document expanded SIMD coverage.The updated SIMD target lists align with the architecture-specific options and expanded feature support reflected in the CMakeLists.txt changes. Documentation is comprehensive and clear.
100-109: Build Options table correctly documents new/refactored options.The additions of ZLIB_ALIASES and ZLIBNG_ENABLE_TESTS, along with the reformatted existing options, accurately reflect the CMakeLists.txt configuration surface. Table layout and descriptions are clear.
194-227: Advanced Build Options table comprehensively documents architecture-specific intrinsics.The restructured table correctly maps CMakeLists.txt architecture-specific options with clear target architecture prefixes. Content accuracy and organization are strong.
Minor note: Static analysis flags mixed British/American English spelling (e.g., "optimisations" on lines 211, 212, 218 vs. "Build with optimisations" on line 103). This mixes with the configure option's American spelling
--without-optimizations. Consider standardizing to one variant codebase-wide in a follow-up cleanup if desired, though this is a pre-existing inconsistency not critical to this PR.
ca62f21 to
afa18d3
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #2001 +/- ##
===========================================
- Coverage 83.31% 82.20% -1.12%
===========================================
Files 161 161
Lines 12960 13612 +652
Branches 3149 3276 +127
===========================================
+ Hits 10798 11190 +392
- Misses 1134 1378 +244
- Partials 1028 1044 +16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
When ZLIB_COMPAT is set, some tests don't make sense, thus should not be unconditionally enabled. I don't see however reason to have two separate options just to enable basic functionality tests. I'm not sure how many tests there is that only make sense when ZLIB_COMPAT is set. |
Those tests should be enabled/disabled according to ZLIB_COMPAT then. That logic is pretty convoluted and not necessary for us. |
I'm not sure if it makes sense to start using this # Backwards compatibility?
if (DEFINED ZLIB_ENABLE_TESTS AND NOT ZLIB_ENABLE_TESTS)
set(BUILD_TESTING OFF)
endif() |
That is a good idea, I wonder how well this change works. It builds fine locally at least :) |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CMakeLists.txt(2 hunks)README.md(1 hunks)test/CMakeLists.txt(3 hunks)test/add-subdirectory-project/CMakeLists.txt(1 hunks)test/example.c(2 hunks)test/fuzz/CMakeLists.txt(1 hunks)test/infcover.c(3 hunks)test/test_shared_ng.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CMakeLists.txt
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2024-09-25T16:25:56.686Z
Learnt from: Dead2
Repo: zlib-ng/zlib-ng PR: 1797
File: inflate.c:81-83
Timestamp: 2024-09-25T16:25:56.686Z
Learning: When `INFLATE_STRICT` is not defined, the variable `state->dmax` is completely removed from the code.
Applied to files:
test/infcover.c
📚 Learning: 2025-08-20T14:34:15.867Z
Learnt from: Dead2
Repo: zlib-ng/zlib-ng PR: 1928
File: gzwrite.c:0-0
Timestamp: 2025-08-20T14:34:15.867Z
Learning: Z_VERSION_ERROR is used by zlib but not by zlib-ng, so it should not be included in error handling for zlib-ng deflateInit2() calls.
Applied to files:
test/infcover.ctest/example.ctest/test_shared_ng.h
📚 Learning: 2024-09-25T16:26:23.643Z
Learnt from: Dead2
Repo: zlib-ng/zlib-ng PR: 1797
File: inflate.c:84-86
Timestamp: 2024-09-25T16:26:23.643Z
Learning: When `INFLATE_ALLOW_INVALID_DISTANCE_TOOFAR_ARRR` is not defined, `state->sane` is completely removed from the codebase and does not need to be initialized.
Applied to files:
test/infcover.c
📚 Learning: 2025-07-04T16:59:44.725Z
Learnt from: mtl1979
Repo: zlib-ng/zlib-ng PR: 1925
File: arch/loongarch/lasxintrin_ext.h:38-65
Timestamp: 2025-07-04T16:59:44.725Z
Learning: In zlib-ng, the policy for supporting old compilers is to maintain compatibility for up to two years after GitHub removes support for them in CI, or when there is no long-time contributor left testing for the architecture if the architecture was not supported by GitHub Actions at all.
Applied to files:
README.mdtest/test_shared_ng.h
📚 Learning: 2025-10-12T17:01:13.630Z
Learnt from: mtl1979
Repo: zlib-ng/zlib-ng PR: 1980
File: compress.c:31-33
Timestamp: 2025-10-12T17:01:13.630Z
Learning: In zlib-ng, when ZLIB_COMPAT is defined, z_int32_t is typedef'd as `int` (not int32_t), and zlib.h is included (which uses `int` in function signatures). This allows the implementation to use z_int32_t while maintaining backwards compatibility with the original zlib API that uses `int`. When ZLIB_COMPAT is not defined, z_int32_t is typedef'd as int32_t and zlib-ng.h is used instead.
Applied to files:
README.mdtest/example.ctest/test_shared_ng.h
📚 Learning: 2025-02-21T01:41:50.358Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:14-24
Timestamp: 2025-02-21T01:41:50.358Z
Learning: In zlib-ng's SSE2 vectorized Chorba CRC implementation, the code that calls READ_NEXT macro ensures 16-byte alignment, making explicit alignment checks unnecessary within the macro.
Applied to files:
test/example.ctest/test_shared_ng.h
📚 Learning: 2025-05-13T06:56:12.407Z
Learnt from: samrussell
Repo: zlib-ng/zlib-ng PR: 1914
File: arch/generic/crc32_chorba_c.c:30-32
Timestamp: 2025-05-13T06:56:12.407Z
Learning: In zlib-ng, pointer casts between z_word_t* and uint64_t* are safe because: 1) On 64-bit platforms, z_word_t is defined as uint64_t, making the cast a no-op; 2) On 32-bit platforms where z_word_t is uint32_t, the code paths using uint64_t* are skipped in favor of 32-bit specific implementations.
Applied to files:
test/test_shared_ng.h
🔇 Additional comments (10)
test/fuzz/CMakeLists.txt (1)
42-44: LGTM - Unconditional test registration is appropriate.The fuzzer tests are now registered unconditionally within this file, which is correct since the parent CMakeLists.txt conditionally includes this directory based on
WITH_FUZZERSand the newBUILD_TESTINGflow.test/test_shared_ng.h (1)
11-18: LGTM - Semantic shift from "enable tests" to "test stock zlib" is appropriate.The change from
#ifdef ZLIBNG_ENABLE_TESTSto#ifndef TEST_STOCK_ZLIBcorrectly reflects the new approach: when not testing against stock zlib, use zlib-ng's native 32-bitdeflatePrime(). When testing stock zlib compatibility, fall back to the 16-bit implementation required by the original zlib API.test/example.c (2)
423-462: LGTM - Correctly excludes zlib-ng specific test.The
test_sync()function is appropriately guarded with#ifndef TEST_STOCK_ZLIBas it expects a specific compressed block layout unique to zlib-ng. The comment on line 426 clarifies this intent.
982-984: LGTM - Consistent guarding of test_sync call.The call to
test_sync()is correctly guarded with the same#ifndef TEST_STOCK_ZLIBcondition as its definition, ensuring consistency.test/infcover.c (2)
621-645: LGTM - Correctly excludes zlib-ng internal test.The
cover_trees()function directly callszng_inflate_table()(line 636, 640), which is internal to zlib-ng. Guarding this with#ifndef TEST_STOCK_ZLIBis appropriate since these internals are not available when testing against stock zlib.
677-679: LGTM - Consistent guarding of cover_trees call.The call to
cover_trees()is correctly guarded with the same#ifndef TEST_STOCK_ZLIBcondition as its definition, ensuring consistency.test/add-subdirectory-project/CMakeLists.txt (1)
8-8: Verify consistency with the main CMakeLists.txt option.This sets
BUILD_TESTSto control test builds, matching the README documentation. However, ensure this aligns with the actual option name used in the parent CMakeLists.txt (verify whether it should beBUILD_TESTSorBUILD_TESTING).test/CMakeLists.txt (3)
57-57: Unconditional infcover test registration - verify this is intentional.The infcover test is now always registered without any conditional guard. Previously (per the AI summary), it was conditionally registered. Ensure this aligns with the intended behavior where the test directory itself is only included when
BUILD_TESTINGis ON in the parent CMakeLists.txt.
178-188: LGTM - Correctly excludes zlib-ng specific tests.The additional test sources (test_adler32.cc, test_crc32.cc, test_inflate_sync.cc, etc.) are appropriately included only when
NOT TEST_STOCK_ZLIB, as these tests exercise zlib-ng specific functionality like CPU intrinsics and internal APIs that are not available in stock zlib.
10-12: TEST_STOCK_ZLIB is checked but not defined in CMakeLists.txt.Verification confirms the concern:
TEST_STOCK_ZLIBat test/CMakeLists.txt:10 is referenced without any definition in the root or test CMakeLists.txt files. The variable is either intended to be passed via cmake command-line (-DTEST_STOCK_ZLIB=ON) or is missing its intended definition. If this is intentional, consider documenting it; otherwise, add anoption()declaration in test/CMakeLists.txt.
382bd9b to
f07d474
Compare
|
I have not tested the functionality of running our tests on stock zlib, don't even know how to do that really, but I added the CMake variable TEST_STOCK_ZLIB that can be used to make tests behave better with stock zlib. Whether that works? IDK |
|
It passes CI at least. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CMakeLists.txt (1)
1597-1597: Feature info references undefined variable.Line 1597 uses
ZLIBNG_ENABLE_TESTSas the status variable foradd_feature_info(), butZLIBNG_ENABLE_TESTSis never defined in the code. This will cause the feature summary to report incorrect status. The actual control variable isBUILD_TESTING.Apply this diff:
-add_feature_info(ZLIBNG_ENABLE_TESTS ZLIBNG_ENABLE_TESTS "Build test binaries") +add_feature_info(ZLIBNG_ENABLE_TESTS BUILD_TESTING "Build test binaries")
🧹 Nitpick comments (1)
CMakeLists.txt (1)
52-54: Consider adding a deprecation message for ZLIB_ENABLE_TESTS.The backward compatibility logic correctly handles
ZLIB_ENABLE_TESTS=OFF, but users who haveZLIB_ENABLE_TESTS=ONmight be surprised that it has no effect (sinceBUILD_TESTINGdefaults to OFF in subdirectory builds). Consider adding a deprecation message to guide users towardBUILD_TESTING:if (DEFINED ZLIB_ENABLE_TESTS AND NOT ZLIB_ENABLE_TESTS) set(BUILD_TESTING OFF) endif() +if (DEFINED ZLIB_ENABLE_TESTS) + message(DEPRECATION "ZLIB_ENABLE_TESTS is deprecated. Please use BUILD_TESTING instead.") +endif()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CMakeLists.txt(2 hunks)README.md(1 hunks)test/CMakeLists.txt(3 hunks)test/add-subdirectory-project/CMakeLists.txt(1 hunks)test/example.c(2 hunks)test/fuzz/CMakeLists.txt(1 hunks)test/infcover.c(3 hunks)test/test_shared_ng.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- README.md
- test/fuzz/CMakeLists.txt
- test/test_shared_ng.h
- test/CMakeLists.txt
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2024-09-25T16:25:56.686Z
Learnt from: Dead2
Repo: zlib-ng/zlib-ng PR: 1797
File: inflate.c:81-83
Timestamp: 2024-09-25T16:25:56.686Z
Learning: When `INFLATE_STRICT` is not defined, the variable `state->dmax` is completely removed from the code.
Applied to files:
test/infcover.c
📚 Learning: 2025-08-20T14:34:15.867Z
Learnt from: Dead2
Repo: zlib-ng/zlib-ng PR: 1928
File: gzwrite.c:0-0
Timestamp: 2025-08-20T14:34:15.867Z
Learning: Z_VERSION_ERROR is used by zlib but not by zlib-ng, so it should not be included in error handling for zlib-ng deflateInit2() calls.
Applied to files:
test/infcover.ctest/example.c
📚 Learning: 2024-09-25T16:26:23.643Z
Learnt from: Dead2
Repo: zlib-ng/zlib-ng PR: 1797
File: inflate.c:84-86
Timestamp: 2024-09-25T16:26:23.643Z
Learning: When `INFLATE_ALLOW_INVALID_DISTANCE_TOOFAR_ARRR` is not defined, `state->sane` is completely removed from the codebase and does not need to be initialized.
Applied to files:
test/infcover.c
📚 Learning: 2025-10-12T17:01:13.630Z
Learnt from: mtl1979
Repo: zlib-ng/zlib-ng PR: 1980
File: compress.c:31-33
Timestamp: 2025-10-12T17:01:13.630Z
Learning: In zlib-ng, when ZLIB_COMPAT is defined, z_int32_t is typedef'd as `int` (not int32_t), and zlib.h is included (which uses `int` in function signatures). This allows the implementation to use z_int32_t while maintaining backwards compatibility with the original zlib API that uses `int`. When ZLIB_COMPAT is not defined, z_int32_t is typedef'd as int32_t and zlib-ng.h is used instead.
Applied to files:
test/example.c
📚 Learning: 2025-02-21T01:41:50.358Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:14-24
Timestamp: 2025-02-21T01:41:50.358Z
Learning: In zlib-ng's SSE2 vectorized Chorba CRC implementation, the code that calls READ_NEXT macro ensures 16-byte alignment, making explicit alignment checks unnecessary within the macro.
Applied to files:
test/example.c
📚 Learning: 2025-04-15T09:20:52.333Z
Learnt from: mtl1979
Repo: zlib-ng/zlib-ng PR: 1904
File: arch/riscv/Makefile.in:12-14
Timestamp: 2025-04-15T09:20:52.333Z
Learning: Optional feature flags in Makefile.in templates should default to empty strings. The configure script should detect feature support and update these variables accordingly during the configuration process.
Applied to files:
CMakeLists.txt
📚 Learning: 2025-07-04T16:59:44.725Z
Learnt from: mtl1979
Repo: zlib-ng/zlib-ng PR: 1925
File: arch/loongarch/lasxintrin_ext.h:38-65
Timestamp: 2025-07-04T16:59:44.725Z
Learning: In zlib-ng, the policy for supporting old compilers is to maintain compatibility for up to two years after GitHub removes support for them in CI, or when there is no long-time contributor left testing for the architecture if the architecture was not supported by GitHub Actions at all.
Applied to files:
CMakeLists.txt
🔇 Additional comments (4)
test/example.c (2)
423-462: LGTM! Guard change aligns with test semantics.The guard change from
#ifdef ZLIBNG_ENABLE_TESTSto#ifndef TEST_STOCK_ZLIBcorrectly implements the intent stated in the comment at line 426—this test expects zlib-ng-specific compressed block layout and should be skipped when testing against stock zlib.
982-984: LGTM! Consistent guard at call site.The guard at the call site correctly matches the function definition guard, ensuring
test_syncis only called when it's compiled.test/infcover.c (1)
621-645: LGTM! Appropriate guards for zlib-ng specific tests.The
#ifndef TEST_STOCK_ZLIBguards correctly exclude thecover_trees()function and its invocation when testing against stock zlib, sincezng_inflate_table()is not available in stock zlib.Also applies to: 677-679
CMakeLists.txt (1)
1577-1592: LGTM! Test gating logic is correct.The migration to
BUILD_TESTINGis properly implemented, and the shared library constraints for gtest and benchmarks are appropriately handled.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CMakeLists.txt (1)
52-55: Enhance backward compatibility to handle both ON and OFF cases.The current logic only maps
ZLIB_ENABLE_TESTS=OFFtoBUILD_TESTING=OFF, but doesn't handle the case where users explicitly setZLIB_ENABLE_TESTS=ON. For complete backward compatibility, consider mapping both states.Apply this diff to handle both cases:
-if (DEFINED ZLIB_ENABLE_TESTS AND NOT ZLIB_ENABLE_TESTS) +if (DEFINED ZLIB_ENABLE_TESTS) message(DEPRECATION "ZLIB_ENABLE_TESTS is deprecated. Please use BUILD_TESTING instead.") - set(BUILD_TESTING OFF) + if(NOT ZLIB_ENABLE_TESTS) + set(BUILD_TESTING OFF) + else() + set(BUILD_TESTING ON) + endif() endif()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CMakeLists.txt(3 hunks)README.md(1 hunks)test/CMakeLists.txt(3 hunks)test/add-subdirectory-project/CMakeLists.txt(1 hunks)test/example.c(2 hunks)test/fuzz/CMakeLists.txt(1 hunks)test/infcover.c(3 hunks)test/test_shared_ng.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- README.md
- test/add-subdirectory-project/CMakeLists.txt
- test/example.c
- test/fuzz/CMakeLists.txt
- test/CMakeLists.txt
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2024-09-25T16:25:56.686Z
Learnt from: Dead2
Repo: zlib-ng/zlib-ng PR: 1797
File: inflate.c:81-83
Timestamp: 2024-09-25T16:25:56.686Z
Learning: When `INFLATE_STRICT` is not defined, the variable `state->dmax` is completely removed from the code.
Applied to files:
test/infcover.c
📚 Learning: 2025-08-20T14:34:15.867Z
Learnt from: Dead2
Repo: zlib-ng/zlib-ng PR: 1928
File: gzwrite.c:0-0
Timestamp: 2025-08-20T14:34:15.867Z
Learning: Z_VERSION_ERROR is used by zlib but not by zlib-ng, so it should not be included in error handling for zlib-ng deflateInit2() calls.
Applied to files:
test/infcover.ctest/test_shared_ng.h
📚 Learning: 2024-09-25T16:26:23.643Z
Learnt from: Dead2
Repo: zlib-ng/zlib-ng PR: 1797
File: inflate.c:84-86
Timestamp: 2024-09-25T16:26:23.643Z
Learning: When `INFLATE_ALLOW_INVALID_DISTANCE_TOOFAR_ARRR` is not defined, `state->sane` is completely removed from the codebase and does not need to be initialized.
Applied to files:
test/infcover.c
📚 Learning: 2025-10-12T17:01:13.630Z
Learnt from: mtl1979
Repo: zlib-ng/zlib-ng PR: 1980
File: compress.c:31-33
Timestamp: 2025-10-12T17:01:13.630Z
Learning: In zlib-ng, when ZLIB_COMPAT is defined, z_int32_t is typedef'd as `int` (not int32_t), and zlib.h is included (which uses `int` in function signatures). This allows the implementation to use z_int32_t while maintaining backwards compatibility with the original zlib API that uses `int`. When ZLIB_COMPAT is not defined, z_int32_t is typedef'd as int32_t and zlib-ng.h is used instead.
Applied to files:
test/test_shared_ng.h
📚 Learning: 2025-05-13T06:56:12.407Z
Learnt from: samrussell
Repo: zlib-ng/zlib-ng PR: 1914
File: arch/generic/crc32_chorba_c.c:30-32
Timestamp: 2025-05-13T06:56:12.407Z
Learning: In zlib-ng, pointer casts between z_word_t* and uint64_t* are safe because: 1) On 64-bit platforms, z_word_t is defined as uint64_t, making the cast a no-op; 2) On 32-bit platforms where z_word_t is uint32_t, the code paths using uint64_t* are skipped in favor of 32-bit specific implementations.
Applied to files:
test/test_shared_ng.h
📚 Learning: 2025-02-21T01:41:50.358Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:14-24
Timestamp: 2025-02-21T01:41:50.358Z
Learning: In zlib-ng's SSE2 vectorized Chorba CRC implementation, the code that calls READ_NEXT macro ensures 16-byte alignment, making explicit alignment checks unnecessary within the macro.
Applied to files:
test/test_shared_ng.h
📚 Learning: 2025-07-04T16:59:44.725Z
Learnt from: mtl1979
Repo: zlib-ng/zlib-ng PR: 1925
File: arch/loongarch/lasxintrin_ext.h:38-65
Timestamp: 2025-07-04T16:59:44.725Z
Learning: In zlib-ng, the policy for supporting old compilers is to maintain compatibility for up to two years after GitHub removes support for them in CI, or when there is no long-time contributor left testing for the architecture if the architecture was not supported by GitHub Actions at all.
Applied to files:
test/test_shared_ng.h
📚 Learning: 2025-04-15T09:20:52.333Z
Learnt from: mtl1979
Repo: zlib-ng/zlib-ng PR: 1904
File: arch/riscv/Makefile.in:12-14
Timestamp: 2025-04-15T09:20:52.333Z
Learning: Optional feature flags in Makefile.in templates should default to empty strings. The configure script should detect feature support and update these variables accordingly during the configuration process.
Applied to files:
CMakeLists.txt
🔇 Additional comments (8)
test/infcover.c (2)
621-645: LGTM! Correct guard for zlib-ng internal API.The guard correctly excludes
cover_trees()when testing with stock zlib, since it callszng_inflate_table()which is a zlib-ng internal function not available in stock zlib.
677-679: LGTM! Consistent guard for function call.The guard correctly excludes the
cover_trees()call when the function itself is not compiled (stock zlib mode), maintaining consistency with the function definition guard.test/test_shared_ng.h (1)
11-18: LGTM! Correct guard for zlib-ng extended API.The guard correctly uses the 32-bit
deflatePrime()path when NOT using stock zlib (zlib-ng native mode), and falls back to two 16-bit calls for stock zlib compatibility, which only supports up to 16 bits per call.CMakeLists.txt (5)
48-48: LGTM! Using standard CMake testing infrastructure.Adding
include(CTest)is the standard CMake way to enable testing support. This provides theBUILD_TESTINGoption (ON by default) and callsenable_testing()automatically.
56-59: LGTM! Correctly updated test dependencies.The dependent options now properly gate on
BUILD_TESTINGinstead of the deprecatedZLIB_ENABLE_TESTS, using the standard CMake testing infrastructure.
1578-1593: LGTM! Testing block correctly updated to use BUILD_TESTING.The test subdirectory and related options are now properly gated on
BUILD_TESTING, consistent with standard CMake testing practices. The updated messages correctly reflect the new gating mechanism.
1598-1598: LGTM! Feature info correctly reports BUILD_TESTING status.The feature summary now correctly reports the status of
BUILD_TESTING, which is the actual control variable for test builds.
1599-1599: LGTM! Clarified sanitizer feature description.Adding "testing" to the description clarifies that sanitizer support is for testing purposes, improving the feature summary readability.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| cmake_dependent_option(WITH_BENCHMARKS "Build test/benchmarks" OFF "ZLIB_ENABLE_TESTS" OFF) | ||
| cmake_dependent_option(WITH_BENCHMARK_APPS "Build application benchmarks" OFF "WITH_BENCHMARKS" OFF) | ||
| if (DEFINED ZLIB_ENABLE_TESTS AND NOT ZLIB_ENABLE_TESTS) | ||
| message(DEPRECATION "ZLIB_ENABLE_TESTS is deprecated. Please use BUILD_TESTING instead.") |
|
Added workaround for BUILD_TESTING not being set until some unclear version (it was used before it actually got documented, and no mention of when it was introduced). |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
README.md (1)
109-109: Correctly documents BUILD_TESTING.The CMake option name now matches the actual variable provided by
include(CTest)in CMakeLists.txt.test/add-subdirectory-project/CMakeLists.txt (1)
8-8: Correctly uses BUILD_TESTING.The cache variable now matches CTest's BUILD_TESTING option (included on line 5).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CMakeLists.txt(4 hunks)README.md(1 hunks)test/CMakeLists.txt(3 hunks)test/add-subdirectory-project/CMakeLists.txt(1 hunks)test/example.c(2 hunks)test/fuzz/CMakeLists.txt(1 hunks)test/infcover.c(3 hunks)test/test_shared_ng.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- test/fuzz/CMakeLists.txt
- test/CMakeLists.txt
- test/infcover.c
- test/example.c
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: mtl1979
Repo: zlib-ng/zlib-ng PR: 1980
File: compress.c:31-33
Timestamp: 2025-10-12T17:01:13.630Z
Learning: In zlib-ng, when ZLIB_COMPAT is defined, z_int32_t is typedef'd as `int` (not int32_t), and zlib.h is included (which uses `int` in function signatures). This allows the implementation to use z_int32_t while maintaining backwards compatibility with the original zlib API that uses `int`. When ZLIB_COMPAT is not defined, z_int32_t is typedef'd as int32_t and zlib-ng.h is used instead.
📚 Learning: 2025-10-12T17:01:13.630Z
Learnt from: mtl1979
Repo: zlib-ng/zlib-ng PR: 1980
File: compress.c:31-33
Timestamp: 2025-10-12T17:01:13.630Z
Learning: In zlib-ng, when ZLIB_COMPAT is defined, z_int32_t is typedef'd as `int` (not int32_t), and zlib.h is included (which uses `int` in function signatures). This allows the implementation to use z_int32_t while maintaining backwards compatibility with the original zlib API that uses `int`. When ZLIB_COMPAT is not defined, z_int32_t is typedef'd as int32_t and zlib-ng.h is used instead.
Applied to files:
test/test_shared_ng.hREADME.md
📚 Learning: 2025-08-20T14:34:15.867Z
Learnt from: Dead2
Repo: zlib-ng/zlib-ng PR: 1928
File: gzwrite.c:0-0
Timestamp: 2025-08-20T14:34:15.867Z
Learning: Z_VERSION_ERROR is used by zlib but not by zlib-ng, so it should not be included in error handling for zlib-ng deflateInit2() calls.
Applied to files:
test/test_shared_ng.h
📚 Learning: 2025-05-13T06:56:12.407Z
Learnt from: samrussell
Repo: zlib-ng/zlib-ng PR: 1914
File: arch/generic/crc32_chorba_c.c:30-32
Timestamp: 2025-05-13T06:56:12.407Z
Learning: In zlib-ng, pointer casts between z_word_t* and uint64_t* are safe because: 1) On 64-bit platforms, z_word_t is defined as uint64_t, making the cast a no-op; 2) On 32-bit platforms where z_word_t is uint32_t, the code paths using uint64_t* are skipped in favor of 32-bit specific implementations.
Applied to files:
test/test_shared_ng.h
📚 Learning: 2025-02-21T01:41:50.358Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:14-24
Timestamp: 2025-02-21T01:41:50.358Z
Learning: In zlib-ng's SSE2 vectorized Chorba CRC implementation, the code that calls READ_NEXT macro ensures 16-byte alignment, making explicit alignment checks unnecessary within the macro.
Applied to files:
test/test_shared_ng.h
📚 Learning: 2025-07-04T16:59:44.725Z
Learnt from: mtl1979
Repo: zlib-ng/zlib-ng PR: 1925
File: arch/loongarch/lasxintrin_ext.h:38-65
Timestamp: 2025-07-04T16:59:44.725Z
Learning: In zlib-ng, the policy for supporting old compilers is to maintain compatibility for up to two years after GitHub removes support for them in CI, or when there is no long-time contributor left testing for the architecture if the architecture was not supported by GitHub Actions at all.
Applied to files:
CMakeLists.txtREADME.md
📚 Learning: 2025-04-15T09:20:52.333Z
Learnt from: mtl1979
Repo: zlib-ng/zlib-ng PR: 1904
File: arch/riscv/Makefile.in:12-14
Timestamp: 2025-04-15T09:20:52.333Z
Learning: Optional feature flags in Makefile.in templates should default to empty strings. The configure script should detect feature support and update these variables accordingly during the configuration process.
Applied to files:
CMakeLists.txt
🔇 Additional comments (6)
test/test_shared_ng.h (1)
11-11: LGTM! Guard correctly switched to TEST_STOCK_ZLIB.The preprocessor guard correctly selects the appropriate deflatePrime implementation: the native 32-bit path for zlib-ng, and the split 16-bit workaround for stock zlib (which only accepts 16 bits per call).
CMakeLists.txt (5)
1-1: Minimum CMake version bump justified by CMP0077 requirement.The bump to 3.13 is necessary for CMP0077 support (see line 52), which allows
option()to honor normal variables—needed for the BUILD_TESTING integration with CTest.
48-48: LGTM! CTest included at appropriate location.Placed correctly before the BUILD_TESTING option declaration (line 52) and among other CMake module includes.
59-62: LGTM! Test-related options correctly depend on BUILD_TESTING.The
cmake_dependent_optiondeclarations properly gate WITH_GTEST, WITH_FUZZERS, WITH_BENCHMARKS, and WITH_BENCHMARK_APPS on BUILD_TESTING.
1581-1598: LGTM! Test gating correctly migrated to BUILD_TESTING.The test enablement logic properly uses BUILD_TESTING and retains the shared library constraints for gtest and benchmarks.
1603-1603: LGTM! Feature info correctly reports BUILD_TESTING.The feature summary now tracks BUILD_TESTING instead of the legacy test flags.
Add CMake variable TEST_STOCK_ZLIB to disable some tests if attempting to run our testsuite on stock zlib. PR depends on CMP0077, introduced by CMake 3.13. Upped minimum compatible CMake version to 3.13, same as we have actually been telling people was the minumum for years on the wiki. Upped upper compatible CMake version to 3.31, my current version.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
README.md (1)
109-109: LGTM! Documentation correctly reflects the CMake option.The BUILD_TESTING option name matches the actual CMake option defined in CMakeLists.txt line 52, and the past review concern about the BUILD_TESTS typo has been addressed.
test/add-subdirectory-project/CMakeLists.txt (1)
8-8: LGTM! Variable name corrected.The BUILD_TESTING variable now correctly matches the CTest-provided option (line 5 includes CTest), and the past critical issue about using BUILD_TESTS has been resolved.
CMakeLists.txt (1)
53-58: Consider handling both ON and OFF cases in deprecation logic.The deprecation mapping only sets
BUILD_TESTING OFFwhenZLIB_ENABLE_TESTSis OFF, but doesn't setBUILD_TESTING ONwhenZLIB_ENABLE_TESTSis ON. While the defaultBUILD_TESTING ON(line 52) handles clean builds, users upgrading with a cachedBUILD_TESTING=OFFwho setZLIB_ENABLE_TESTS=ONwon't see tests enabled.For smoother migration, consider handling both cases:
if (DEFINED ZLIB_ENABLE_TESTS) message(DEPRECATION "ZLIB_ENABLE_TESTS is deprecated. Please use BUILD_TESTING instead.") if (ZLIB_ENABLE_TESTS) set(BUILD_TESTING ON) else() set(BUILD_TESTING OFF) endif() endif()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CMakeLists.txt(4 hunks)README.md(1 hunks)test/CMakeLists.txt(3 hunks)test/add-subdirectory-project/CMakeLists.txt(1 hunks)test/example.c(2 hunks)test/fuzz/CMakeLists.txt(1 hunks)test/infcover.c(3 hunks)test/test_shared_ng.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/infcover.c
- test/example.c
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: mtl1979
Repo: zlib-ng/zlib-ng PR: 1980
File: compress.c:31-33
Timestamp: 2025-10-12T17:01:13.630Z
Learning: In zlib-ng, when ZLIB_COMPAT is defined, z_int32_t is typedef'd as `int` (not int32_t), and zlib.h is included (which uses `int` in function signatures). This allows the implementation to use z_int32_t while maintaining backwards compatibility with the original zlib API that uses `int`. When ZLIB_COMPAT is not defined, z_int32_t is typedef'd as int32_t and zlib-ng.h is used instead.
Learnt from: mtl1979
Repo: zlib-ng/zlib-ng PR: 1925
File: arch/loongarch/lasxintrin_ext.h:38-65
Timestamp: 2025-07-04T16:59:44.725Z
Learning: In zlib-ng, the policy for supporting old compilers is to maintain compatibility for up to two years after GitHub removes support for them in CI, or when there is no long-time contributor left testing for the architecture if the architecture was not supported by GitHub Actions at all.
📚 Learning: 2025-07-04T16:59:44.725Z
Learnt from: mtl1979
Repo: zlib-ng/zlib-ng PR: 1925
File: arch/loongarch/lasxintrin_ext.h:38-65
Timestamp: 2025-07-04T16:59:44.725Z
Learning: In zlib-ng, the policy for supporting old compilers is to maintain compatibility for up to two years after GitHub removes support for them in CI, or when there is no long-time contributor left testing for the architecture if the architecture was not supported by GitHub Actions at all.
Applied to files:
test/CMakeLists.txtCMakeLists.txtREADME.md
📚 Learning: 2025-04-15T09:20:52.333Z
Learnt from: mtl1979
Repo: zlib-ng/zlib-ng PR: 1904
File: arch/riscv/Makefile.in:12-14
Timestamp: 2025-04-15T09:20:52.333Z
Learning: Optional feature flags in Makefile.in templates should default to empty strings. The configure script should detect feature support and update these variables accordingly during the configuration process.
Applied to files:
CMakeLists.txt
📚 Learning: 2025-10-12T17:01:13.630Z
Learnt from: mtl1979
Repo: zlib-ng/zlib-ng PR: 1980
File: compress.c:31-33
Timestamp: 2025-10-12T17:01:13.630Z
Learning: In zlib-ng, when ZLIB_COMPAT is defined, z_int32_t is typedef'd as `int` (not int32_t), and zlib.h is included (which uses `int` in function signatures). This allows the implementation to use z_int32_t while maintaining backwards compatibility with the original zlib API that uses `int`. When ZLIB_COMPAT is not defined, z_int32_t is typedef'd as int32_t and zlib-ng.h is used instead.
Applied to files:
README.mdtest/test_shared_ng.h
📚 Learning: 2025-08-20T14:34:15.867Z
Learnt from: Dead2
Repo: zlib-ng/zlib-ng PR: 1928
File: gzwrite.c:0-0
Timestamp: 2025-08-20T14:34:15.867Z
Learning: Z_VERSION_ERROR is used by zlib but not by zlib-ng, so it should not be included in error handling for zlib-ng deflateInit2() calls.
Applied to files:
test/test_shared_ng.h
📚 Learning: 2025-05-13T06:56:12.407Z
Learnt from: samrussell
Repo: zlib-ng/zlib-ng PR: 1914
File: arch/generic/crc32_chorba_c.c:30-32
Timestamp: 2025-05-13T06:56:12.407Z
Learning: In zlib-ng, pointer casts between z_word_t* and uint64_t* are safe because: 1) On 64-bit platforms, z_word_t is defined as uint64_t, making the cast a no-op; 2) On 32-bit platforms where z_word_t is uint32_t, the code paths using uint64_t* are skipped in favor of 32-bit specific implementations.
Applied to files:
test/test_shared_ng.h
📚 Learning: 2025-02-21T01:41:50.358Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:14-24
Timestamp: 2025-02-21T01:41:50.358Z
Learning: In zlib-ng's SSE2 vectorized Chorba CRC implementation, the code that calls READ_NEXT macro ensures 16-byte alignment, making explicit alignment checks unnecessary within the macro.
Applied to files:
test/test_shared_ng.h
🧬 Code graph analysis (1)
README.md (1)
test/pkgcheck.sh (1)
usage(3-46)
🔇 Additional comments (8)
test/CMakeLists.txt (2)
56-56: LGTM! Test registration simplified.The infcover test is now unconditionally registered, which is appropriate since the entire test/ directory is only processed when BUILD_TESTING is enabled (CMakeLists.txt line 1581). This simplification removes redundant conditional logic.
177-187: LGTM! Conditional test inclusion correctly inverted.The logic correctly excludes zlib-ng-specific tests (adler32_neon, cpu_check_features, etc.) when
TEST_STOCK_ZLIBis defined, as these tests rely on zlib-ng internals and would fail against stock zlib.test/test_shared_ng.h (1)
11-18: LGTM! Guard condition correctly transformed.The preprocessor guard change from
#ifdef ZLIBNG_ENABLE_TESTSto#ifndef TEST_STOCK_ZLIBcorrectly maintains the semantic intent:
- When
TEST_STOCK_ZLIBis NOT defined: use zlib-ng's native 32-bitdeflatePrime- When
TEST_STOCK_ZLIBIS defined: use the stock zlib-compatible 16-bit two-step approachThis transformation aligns with the new testing flag semantics while preserving the original behavior.
CMakeLists.txt (4)
59-62: LGTM! Dependent options correctly structured.The test-related options (WITH_GTEST, WITH_FUZZERS, WITH_BENCHMARKS, WITH_BENCHMARK_APPS) now properly depend on BUILD_TESTING using
cmake_dependent_option, ensuring they're automatically disabled when BUILD_TESTING is OFF while remaining independently configurable when BUILD_TESTING is ON.
1581-1598: LGTM! Test gating migrated to BUILD_TESTING.The test directory inclusion and related warning messages have been correctly updated to use BUILD_TESTING instead of ZLIB_ENABLE_TESTS, maintaining the same logic while adopting the standard CMake convention.
1603-1604: LGTM! Feature summary updated.The feature info declarations correctly reflect the migration from ZLIB_ENABLE_TESTS/ZLIBNG_ENABLE_TESTS to BUILD_TESTING, ensuring the feature summary displays accurate information.
1-1: No action required—code is correct.The code safely handles version compatibility. Line 4-5 uses
if(POLICY CMP0169)to conditionally set the policy only when CMake 3.30+ recognizes it, preventing errors on older versions. The minimum version 3.14 satisfies the CMP0077 (3.13) dependency mentioned at line 52 and likely other requirements. The discrepancy noted in the original review is not an issue—this is correct defensive programming.test/fuzz/CMakeLists.txt (1)
42-44: LGTM! Unconditional test registration appropriate.The fuzz tests are now unconditionally registered within this file, which is correct because:
- The test/ directory is only processed when BUILD_TESTING is ON (CMakeLists.txt line 1581)
- This fuzz/ subdirectory is only included when WITH_FUZZERS is ON (test/CMakeLists.txt line 86)
- WITH_FUZZERS depends on BUILD_TESTING (CMakeLists.txt line 60)
This multi-level gating eliminates the need for redundant conditionals in this file.
You need headers and libz.a from stock zlib, then link the test sources against libz.a from stock zlib. This isn't much different from comparing ABIs between stock zlib and zlib-ng in compat mode as both require pulling stock zlib sources from its Git repository and building within "fetched" zlib-ng repository. |
|
LGTM! |
@mtl1979 Would you mind making a step-by-step of that on the wiki for future reference? New page called "Advanced Testing" or something perhaps, a page that could be used to describe several special testing features like stock zlib, libpng, abi, etc.. |
I'm still figuring out how much changes it requires in the test sources itself... We could short circuit by passing As most of the long-time people know, I'm not very good at making CMake build scripts, so I might need some help with isolating the test sources from rest of the zlib-ng source tree... |
Oh, I thought you were using it this way already. It's more of a nice-to-have than a must-have. Not a big priority IMO. |
Build system of stock zlib has had quite big bugs for years, so using it for anything complex has just failed... I know there is workarounds for a lot of the bugs in stock zlib, but it's matter of knowing which commit in the I know how to do a lot of weird things in principle, but doing some of them will need some edits to more than one source tree. There is always a "hack" way of things and then there is cleaner way of doing things... "Hack" way in this case is just replacing |
Start using CTest and the BUILD_TESTING option provided by it, this increases minimum CMake version to 3.14, same as what we had as minimum on the wiki since 2.1.0 but never enforced.
Also improve and make more clear the partial support for running our tests on the stock zlib, it now uses requires CMake variable TEST_STOCK_ZLIB to be manually set. Improved some of the tests in that relation.
In total, this simplifies the testing options and makes them follow the CTest standard more closely.