Skip to content

cmake CI tests refactor #4406

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

Merged
merged 10 commits into from
Jun 9, 2025
Merged

Conversation

Cyan4973
Copy link
Contributor

@Cyan4973 Cyan4973 commented Jun 8, 2025

- Create new .github/workflows/cmake-tests.yml with all cmake-related jobs
- Move cmake-build-and-test-check, cmake-source-directory-with-spaces, and cmake-visual-2022 jobs
- Remove cmake tests from dev-short-tests.yml to improve organization
- Maintain same trigger conditions and test configurations
- Add dedicated concurrency group for cmake tests

This separation allows cmake tests to run independently and makes
the CI configuration more modular and easier to maintain.
@Cyan4973 Cyan4973 force-pushed the separate-cmake-tests branch from 62b0737 to b922774 Compare June 8, 2025 21:44
Cyan4973 added 2 commits June 8, 2025 22:19
and comment out windows arm64 tests due to unacceptably long queue time
@Cyan4973 Cyan4973 force-pushed the separate-cmake-tests branch from 38fb11b to 9c866c9 Compare June 8, 2025 23:57
@Cyan4973 Cyan4973 force-pushed the separate-cmake-tests branch 8 times, most recently from 646305e to 75abb8b Compare June 9, 2025 03:20
@Cyan4973
Copy link
Contributor Author

Cyan4973 commented Jun 9, 2025

So far, I've been able to reproduce the issue described in #4405,
but I haven't been able to fix it cleanly.

Specifically, whenever I change the script to fix the cmake build issue when ZSTD_BUILD_TESTS is disabled, then cmake on Windows + Visual fails.

@ThomasDevoogdt : so far, I've not been able to have the new cmake build correctly on all platforms when only C is enabled but not C++.

- Split monolithic 235-line CMakeLists.txt into focused modules
- Main file reduced to 78 lines with clear section organization
- Created 5 specialized modules:
  * ZstdVersion.cmake - CMake policies and version management
  * ZstdOptions.cmake - Build options and platform configuration
  * ZstdDependencies.cmake - External dependency management
  * ZstdBuild.cmake - Build targets and validation
  * ZstdPackage.cmake - Package configuration generation

Benefits:
- Improved readability and maintainability
- Better separation of concerns
- Easier debugging and modification
- Preserved 100% backward compatibility
- All existing build options and targets unchanged

The refactored build system passes all tests and maintains
identical functionality while being much easier to understand
and maintain.
@Cyan4973 Cyan4973 force-pushed the separate-cmake-tests branch 2 times, most recently from d15a83b to e6c738b Compare June 9, 2025 06:15
@ThomasDevoogdt
Copy link
Contributor

ThomasDevoogdt commented Jun 9, 2025

@Cyan4973 I checked your branch:

thomas@thomas-Precision-7670:~/external/zstd/build/cmake/ninja$ CXX=/bin/false cmake -GNinja ../
-- ZSTD VERSION: 1.5.8
-- The C compiler identification is GNU 13.3.0
-- The ASM compiler identification is GNU
-- Found assembler: /usr/bin/cc
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Setting build type to 'Release' as none was specified.
-- CMAKE_INSTALL_PREFIX: /usr/local
-- CMAKE_INSTALL_LIBDIR: lib
-- ZSTD_LEGACY_SUPPORT enabled
-- Multi-threading support enabled
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD
-- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Success
-- Found Threads: TRUE
-- Configuring done (0.3s)
-- Generating done (0.0s)
-- Build files have been written to: /home/thomas/external/zstd/build/cmake/ninja
thomas@thomas-Precision-7670:~/external/zstd/build/cmake/ninja$ ninja
[103/103] Creating library symlink lib/libzstd.so.1 lib/libzstd.so
thomas@thomas-Precision-7670:~/external/zstd/build/cmake/ninja$ 

Compilation seems to be fine with the proposed changes.

Btw, Buildroot explicitly exports CXX to /bin/false for all the tool-chains that don't support C++, see commit buildroot/buildroot@b34e0d2.

My fault that I broke something for Windows, but I also can't fix/check anything there, as I don't have access right now to any Windows PC.

@Cyan4973 Cyan4973 force-pushed the separate-cmake-tests branch from e6c738b to 11838e0 Compare June 9, 2025 06:27
@Cyan4973
Copy link
Contributor Author

Cyan4973 commented Jun 9, 2025

More specifically, the combination that fails now is Windows + Visual + clang-cl

@Cyan4973 Cyan4973 force-pushed the separate-cmake-tests branch 2 times, most recently from b11377f to 49fe2ec Compare June 9, 2025 06:46
@Cyan4973 Cyan4973 force-pushed the separate-cmake-tests branch from 72b59f0 to b6dc292 Compare June 9, 2025 06:59
Cyan4973 added 2 commits June 9, 2025 07:09
by removing processing of resource files in this case
@Cyan4973
Copy link
Contributor Author

Cyan4973 commented Jun 9, 2025

OK, all issues seem finally fixed now

@ThomasDevoogdt
Copy link
Contributor

Compilation seems to be fine with the proposed changes.

OK, all issues seem finally fixed now

Still fine!

overkill and leaky to transport a test result just in one place.
@Cyan4973 Cyan4973 merged commit 5e6bdf5 into facebook:dev Jun 9, 2025
103 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Recent commit break cmake building on Windows
3 participants