Replace setuptools with scikit-build-core and CMake.
#73
Conversation
WalkthroughThe pull request introduces significant changes to the build configuration of a C++ project that integrates with Python. Key updates include the transition from Changes
Possibly related PRs
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
pyproject.toml (1)
93-95: Consider adding more CMake configurationsWhile the basic CMake configuration is present, consider adding more specific configurations that might be needed for your C++ extensions:
cmake.targets: Specify explicit targets to buildcmake.args: Add platform-specific compilation flagscmake.install-dir: Control where the built extensions are installedExample addition:
[tool.scikit-build] cmake.build-type = "Release" cmake.version = ">=3.17" +cmake.args = ["-DCMAKE_POSITION_INDEPENDENT_CODE=ON"] +cmake.install-dir = "clp_ffi_py"CMakeLists.txt (3)
18-19: Consider using a version file or environment variableThe hardcoded version "0.0.12" for local builds could become outdated. Consider reading from a version file or environment variable to maintain consistency with the project's actual version.
- set(CLP_FFI_PY_VERSION "0.0.12" CACHE STRING "Use local build version." FORCE) + file(READ "${CMAKE_CURRENT_SOURCE_DIR}/VERSION" CLP_FFI_PY_VERSION) + string(STRIP "${CLP_FFI_PY_VERSION}" CLP_FFI_PY_VERSION) + set(CLP_FFI_PY_VERSION "${CLP_FFI_PY_VERSION}" CACHE STRING "Use version from VERSION file." FORCE)
53-58: Add path existence validationThe source paths are set without validation. Consider adding checks to ensure the directories exist.
set(CPP_SRC_ROOT "src" CACHE STRING "Path to the root of all C++ source code") +if(NOT EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/${CPP_SRC_ROOT}") + message(FATAL_ERROR "Source directory ${CPP_SRC_ROOT} does not exist") +endif()
50-51: Consider specifying minimum Python versionThe Python package requirements should be explicit. Consider adding a version check.
find_package(Python COMPONENTS Interpreter Development.Module REQUIRED) +if(Python_VERSION VERSION_LESS 3.7) + message(FATAL_ERROR "Python 3.7 or newer is required") +endif()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- CMakeLists.txt (1 hunks)
- pyproject.toml (2 hunks)
- setup.py (0 hunks)
💤 Files with no reviewable changes (1)
- setup.py
🧰 Additional context used
🔇 Additional comments (5)
pyproject.toml (2)
Line range hint
2-95: Ensure test coverage for the new build systemSince this is a WIP PR transitioning the build system, it's crucial to have comprehensive testing in place.
Let's check for existing tests:
#!/bin/bash # Look for build-related tests echo "Checking for build system tests..." fd -t f -e py -e sh test build_test # Check if we have CI workflows testing the build echo "Checking CI workflows..." fd -t f . .github/workflows/ -e yml -e yamlWould you like me to help create a test plan or generate test cases specifically for validating the new build system configuration?
2-3: Verify build system dependencies are sufficientThe transition to scikit-build-core as the sole build requirement needs verification to ensure all necessary build dependencies are covered, especially for a WIP change.
Let's check the project structure and requirements:
✅ Verification successful
Let me generate new shell scripts to better verify the build system requirements.
This script should help us understand:
- If there are C++ components requiring compilation
- The presence of Python extension modules
- The complete build configuration
- CI workflow details that might indicate additional build dependencies
Build system dependencies are correctly configured
The transition to scikit-build-core as the sole build requirement is appropriate for this project based on the following findings:
- The project contains extensive C++ source files and headers under
src/clp_ffi_py/- CMake is properly configured with a minimum version requirement (>=3.17)
- The CI workflow successfully builds wheels for multiple platforms (Linux, macOS, Windows) and architectures
- No Cython files are present that would require additional build dependencies
- The build configuration in pyproject.toml includes all necessary settings for scikit-build-core to handle the CMake-based build process
The single dependency on scikit-build-core is sufficient as it handles all the necessary build system requirements for this C++ extension module project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for C++ source files and CMake configuration echo "Checking for C++ sources and CMake files..." fd -e cpp -e hpp -e h fd CMakeLists.txt # Check if we have any Cython files that might need additional build requirements echo "Checking for Cython files..." fd -e pyx -e pxd # Look for any other potential build dependencies echo "Checking for other build-related files..." fd -g "*.c" -g "*.so" -g "*.dylib" -g "*.dll"Length of output: 449
Script:
#!/bin/bash # Check for C++ and CMake files with better error handling echo "=== C++ and CMake files ===" find . -type f \( -name "*.cpp" -o -name "*.hpp" -o -name "*.h" -o -name "CMakeLists.txt" \) 2>/dev/null || true echo -e "\n=== Python extension modules ===" find . -type f -name "setup.py" 2>/dev/null || true find . -type f -name "_*.pyx" 2>/dev/null || true echo -e "\n=== Build configuration ===" cat pyproject.toml 2>/dev/null || true echo -e "\n=== CI Build Workflow ===" cat .github/workflows/build_wheels.yml 2>/dev/null || trueLength of output: 10579
CMakeLists.txt (3)
27-39: LGTM! Robust macOS compatibility handlingThe macOS deployment target configuration is well-implemented with proper version checking and clear status messages.
105-110: LGTM! Proper installation configurationThe installation setup correctly uses components and respects the installation flag.
88-91: 🛠️ Refactor suggestionConsider C++17 compatibility mode
C++20 might not be widely supported on all platforms. Consider providing a fallback to C++17 if C++20 features aren't strictly necessary.
- cxx_std_20 + $<IF:$<BOOL:${SUPPORTS_CXX20}>,cxx_std_20,cxx_std_17>Add this check before the target:
include(CheckCXXCompilerFlag) check_cxx_compiler_flag("-std=c++20" SUPPORTS_CXX20)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- .github/workflows/build_wheels.yml (2 hunks)
- CMakeLists.txt (1 hunks)
🧰 Additional context used
🔇 Additional comments (9)
CMakeLists.txt (7)
1-3: LGTM: CMake version requirement is well-documented and appropriate.The minimum version requirement of 3.17 is properly justified by the need for
Python_SOABIsupport, while still meeting scikit-build-core's minimum requirement of 3.15.
5-25: LGTM: Well-structured build configuration with clear context separation.The configuration properly handles both scikit-build-core and standalone builds with appropriate cache variables and warnings.
27-34: LGTM: Proper project setup with necessary PIC configuration.The project languages and PIC settings are correctly configured for building Python extensions.
47-89: LGTM: Well-structured library target configuration.The library target is properly configured with appropriate sources, C++20 standard, and include directories.
91-96: LGTM: Proper conditional installation setup.The installation configuration correctly uses components and respects the installation flag.
36-43: 🛠️ Refactor suggestionVerify path existence and add path validation.
While the path configuration is well-structured, it's recommended to validate path existence to prevent build failures.
#!/bin/bash # Description: Verify that all configured source paths exist # Expected: All paths should exist in the repository echo "Verifying source paths..." for path in "src" "src/clp_ffi_py" "src/clp/components/core" "src/clp/components/core/src/clp"; do if [ ! -d "$path" ]; then echo "Warning: Directory '$path' does not exist" fi doneConsider adding path validation:
# Add after path definitions foreach(path IN ITEMS ${CPP_SRC_ROOT} ${CLP_FFI_PY_SRC_ROOT} ${CLP_CORE_ROOT} ${CLP_SRC_ROOT}) if(NOT EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/${path}") message(FATAL_ERROR "Required path does not exist: ${path}") endif() endforeach()
82-82: 🛠️ Refactor suggestionConsider adding submodule dependency checks.
The inclusion of submodules directory suggests external dependencies, but their presence isn't verified.
Consider adding submodule checks:
# Add before including submodules directory if(EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/.gitmodules") find_package(Git QUIET) if(GIT_FOUND) execute_process( COMMAND ${GIT_EXECUTABLE} submodule status WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR} OUTPUT_VARIABLE SUBMODULE_STATUS ERROR_QUIET ) if(NOT SUBMODULE_STATUS) message(WARNING "Submodules not initialized. Please run: git submodule update --init") endif() endif() endif()✅ Verification successful
Let me gather more information about the CMake configuration and submodules to make a proper assessment.
Let me check if the submodule is properly initialized and verify the directory structure.
The review comment's suggestion is valid and necessary
Based on the verification results:
- The repository has an uninitialized submodule (indicated by the
-prefix in the submodule status)- The CLP core directory is missing, which is required for the build
- The CMakeLists.txt references paths within the submodule that don't exist yet
The original review comment correctly identifies the need for submodule dependency checks, as the build would fail without properly initialized submodules. The suggested CMake code would help developers by providing a clear warning message when submodules aren't initialized.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for submodules initialization # Expected: Submodules should be initialized if they exist echo "Checking submodules..." if [ -f .gitmodules ]; then # List submodules and their status git submodule status fiLength of output: 877
Script:
#!/bin/bash # Check for .gitmodules and CMake files echo "Checking for .gitmodules and CMake configuration..." if [ -f .gitmodules ]; then echo "Content of .gitmodules:" cat .gitmodules fi echo -e "\nChecking CMake files..." find . -name CMakeLists.txt -type f -exec echo "=== {} ===" \; -exec cat {} \; echo -e "\nChecking directory structure..." ls -laLength of output: 5804
Script:
#!/bin/bash echo "Checking submodule status..." git submodule status echo -e "\nVerifying CLP core directory structure..." ls -la src/clp/components/core 2>/dev/null || echo "CLP core directory not found" echo -e "\nChecking if submodule needs initialization..." git submodule update --init --recursive --dry-runLength of output: 1571
.github/workflows/build_wheels.yml (2)
208-208: Verify cibuildwheel compatibility with scikit-build-coreThe update to
cibuildwheel@v2.21.3aligns with the transition to scikit-build-core. However, please ensure:
- The new version supports all required build configurations
- Any new environment variables or settings required by scikit-build-core are properly configured
✅ Verification successful
Let me gather more information about the cibuildwheel configuration and scikit-build-core integration.
Based on the gathered information, I can now provide the final response:
cibuildwheel v2.21.3 is properly configured for scikit-build-core
The verification shows that:
- The
pyproject.tomlcorrectly configures scikit-build-core as the build backend- The cibuildwheel configuration in
pyproject.tomlincludes all necessary settings:
- Build types and architectures are properly defined
- musllinux images are configured to support C++20
- CMake settings are specified via
tool.scikit-buildsection- The workflow file correctly uses the matrix strategy for all supported platforms
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for scikit-build-core specific configurations rg -A 5 "CIBW_.*CONFIGURE|CIBW_.*CMAKE|CIBW_.*BUILD" .github/workflows/build_wheels.ymlLength of output: 274
Script:
#!/bin/bash # Check for build system configurations and dependencies echo "=== Checking pyproject.toml for build system config ===" cat pyproject.toml echo -e "\n=== Checking for cibuildwheel config file ===" fd -g "cibuildwheel.toml|.cibuildwheel.toml" echo -e "\n=== Checking workflow file for build configurations ===" cat .github/workflows/build_wheels.ymlLength of output: 9199
75-75: Verify macOS version compatibility requirementsThe update from
macos-12tomacos-13for all macOS builds could impact users on older systems. Consider:
- Documenting the minimum supported macOS version in the project's README
- Verifying that the built wheels maintain compatibility with older macOS versions if required
Also applies to: 77-77, 79-79, 81-81, 83-83, 85-85, 87-87, 89-89, 91-91, 93-93, 95-95, 97-97, 99-99, 101-101, 103-103, 105-105
✅ Verification successful
Let me gather more information about the macOS compatibility requirements and build configurations.
Based on the gathered information, I can now provide the final response:
Update to macOS 13 runner is appropriate and well-configured
The change from
macos-12tomacos-13for the GitHub Actions runner is appropriate because:
- The project uses scikit-build-core with modern CMake requirements (3.17+)
- The macOS wheel building configuration in pyproject.toml is properly set up with support for multiple architectures (x86_64, universal2, arm64)
- No explicit macOS deployment target is set, which means it defaults to the minimum supported by the Python version being built
- The project already requires Python >= 3.7, which ensures broad compatibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if minimum macOS version is documented rg -i "macos|osx|darwin.*(?:version|requirement|compatibility)" README.mdLength of output: 274
Script:
#!/bin/bash # Check for any macOS-specific build configurations in the workflow rg -A 5 'CIBW_ARCHS.*macos|CIBW_ENVIRONMENT.*macos|MACOSX_DEPLOYMENT_TARGET' .github/workflows/build_wheels.yml # Check setup files for macOS deployment target fd -e py setup fd CMakeLists.txt -x cat {} # Look for any macOS-specific configurations in pyproject.toml fd pyproject.toml -x cat {}Length of output: 6507
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
pyproject.toml (1)
96-98: Consider documenting build configuration requirements.The CMake configuration looks good. However, it would be helpful to document these requirements in the README.md file to help developers set up their environment correctly.
Would you like me to help generate the documentation for these build requirements?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- .github/workflows/build_wheels.yml (1 hunks)
- CMakeLists.txt (1 hunks)
- pyproject.toml (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/build_wheels.yml
- CMakeLists.txt
🧰 Additional context used
🔇 Additional comments (1)
pyproject.toml (1)
2-3: Verify scikit-build-core compatibility with existing CI/CD pipelines.The transition to scikit-build-core is aligned with the PR objectives. However, we should ensure this change is compatible with existing CI/CD pipelines and development environments.
Let's verify the dependency specifications:
✅ Verification successful
Let me gather more specific information about the CI/CD configuration and the CMake integration.
This will help us verify the build system compatibility more thoroughly by examining:
- CI configuration files to ensure they're prepared for scikit-build-core
- CMake integration details
- Any existing build scripts that might need updates
Let me check the task file and build script to complete our verification.
This will help us verify:
- Task runner configuration for build processes
- Build script compatibility
- Any additional Python package requirements that might interact with scikit-build-core
Let me check the development requirements to complete our verification.
This will help us verify that the necessary build dependencies are properly configured in the development requirements.
The scikit-build-core transition is properly configured and compatible
Based on the comprehensive verification:
- CMake configuration properly integrates with scikit-build-core (minimum version 3.15)
- Build system is correctly configured in Taskfile.yml using Python's build module
- Development requirements include necessary build dependencies (build>=0.8.0, cibuildwheel>=2.16.2)
- No conflicting setuptools references remain in the codebase
- The build infrastructure properly handles both scikit-build-core and local build scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining setuptools dependencies that might conflict # with the new build system # Test 1: Search for any remaining setuptools references echo "Checking for setuptools references..." rg -l 'setuptools' # Test 2: Verify scikit-build-core version availability echo "Checking scikit-build-core version availability..." rg -l 'scikit-build-core'Length of output: 270
Script:
#!/bin/bash # Check CI configuration files and CMake integration # Look for CI configuration files echo "Checking CI configuration files..." fd -e yml -e yaml # Check CMake configuration for scikit-build integration echo "Checking CMake configuration..." cat CMakeLists.txt # Check for any build-related scripts echo "Checking build-related scripts..." fd -e sh -e bash -e ps1 buildLength of output: 4224
Script:
#!/bin/bash # Check build configuration details # Check Taskfile.yml content echo "Checking Taskfile.yml..." cat Taskfile.yml # Check build script content echo "Checking build script..." cat docs/build.sh # Check Python package requirements echo "Checking additional Python requirements..." fd -e txt -e in requirementsLength of output: 3813
Script:
#!/bin/bash # Check development requirements echo "Checking development requirements..." cat requirements-dev.txt echo "Checking if build package is included..." rg "build" requirements-dev.txtLength of output: 586
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
pyproject.toml (1)
96-98: LGTM! Consider documenting CMake configuration choices.The CMake configuration is appropriate for the build system transition. Consider adding a comment explaining why Release build type was chosen and the minimum CMake version requirement.
Add documentation above the section:
+# Configure CMake build settings +# - Release build type optimizes the binary size and performance +# - CMake 3.15+ is required for proper scikit-build-core integration [tool.scikit-build] cmake.build-type = "Release" cmake.version = ">=3.15"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- pyproject.toml (3 hunks)
🧰 Additional context used
🔇 Additional comments (2)
pyproject.toml (2)
2-3: LGTM! Build system configuration is properly updated.The transition to scikit-build-core is well-configured with an appropriate version constraint. The alphabetization suggestion from the previous review has been acknowledged.
67-69: Duplicate comment: MacOS deployment target configuration should be moved.This comment has already been addressed by a previous review. Please refer to the existing discussion about moving this configuration to PR #74.
LinZhihao-723
changed the title
setup.py with CMake and scikit-build-core.setuptools by scikit-build-core with CMake.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
pyproject.toml (2)
2-3: LGTM! Consider alphabetizing the fields.The transition to scikit-build-core aligns well with the PR objectives. The version requirement ensures compatibility.
Consider alphabetizing these fields as previously suggested:
-build-backend = "scikit_build_core.build" -requires = ["scikit-build-core >= 0.10.7"] +requires = ["scikit-build-core >= 0.10.7"] +build-backend = "scikit_build_core.build"
96-106: LGTM! Minor typo in comment.The scikit-build configuration is well-structured and properly documented. The CMake version requirement and build type are appropriate for the project needs.
There's a typo in the comment on line 100:
-# distribution. This list is to explict exclude files not specified in `.gitignore`. +# distribution. This list is to explicitly exclude files not specified in `.gitignore`.CMakeLists.txt (3)
19-20: Consider moving CMAKE_EXPORT_COMPILE_COMMANDS earlier.The compile commands export setting should ideally be placed right after cmake_minimum_required for better visibility and to ensure it's set before any project configuration.
cmake_minimum_required(VERSION 3.15) +set(CMAKE_EXPORT_COMPILE_COMMANDS ON CACHE BOOL + "Enable/Disable output of compile commands during generation." FORCE) set(CLP_FFI_PY_INSTALL_LIBS ON CACHE BOOL "Whether to install the built libraries." FORCE)
35-36: Consider using a more descriptive library name.The library name "native" is quite generic. Consider using a more descriptive name that reflects its purpose, such as "clp_ffi_py_ir_native" or similar.
-set(CLP_FFI_PY_LIB_IR "native" CACHE STRING "The name of IR native library") +set(CLP_FFI_PY_LIB_IR "clp_ffi_py_ir_native" CACHE STRING "The name of IR native library")
117-119: Consider versioned installation paths.The current installation path might cause conflicts when multiple versions are installed. Consider including the version in the installation path.
- DESTINATION ${CLP_FFI_PY}/ir + DESTINATION ${CLP_FFI_PY}/${CLP_FFI_PY_VERSION}/ir
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- CMakeLists.txt (1 hunks)
- pyproject.toml (3 hunks)
🔇 Additional comments (5)
pyproject.toml (1)
67-69:⚠️ Potential issueRemove MacOS deployment target configuration.
According to previous discussions, this configuration should be moved to PR #74 to maintain clear separation of concerns.
Apply this diff to remove these lines:
-[tool.cibuildwheel.macos.environment] -MACOSX_DEPLOYMENT_TARGET = "10.15" -Likely invalid or redundant comment.
CMakeLists.txt (4)
1-2: LGTM: Appropriate minimum CMake version.The minimum version of 3.15 is well-chosen as scikit-build-core handles FindPython backporting for older versions.
23-30: LGTM: Proper project configuration with PIC support.The project is correctly configured with both C++ and C languages, and position-independent code is appropriately enabled for shared library compilation.
38-41: LGTM: Appropriate C++ standard selection.The C++20 requirement is well-specified with proper visibility scope.
62-96: Consider proper header installation configuration.While headers are included for CLion support, there's no explicit configuration for installing public headers. If this library is meant to be used as a dependency by other C++ code, consider:
- Separating public and private headers
- Configuring proper header installation
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
CMakeLists.txt (4)
39-45: Consider reordering REQUIRED keyword.For better readability and consistency with CMake conventions, consider placing REQUIRED before COMPONENTS.
find_package( Python - REQUIRED - COMPONENTS + COMPONENTS + REQUIRED Interpreter Development.Module )
47-52: Consider using a more descriptive library name.The current name "native" is quite generic. Consider using a more descriptive name that reflects its purpose, such as "clp_ffi_py_ir".
56-58: Use consistent variable naming convention.For better maintainability, consider using the project prefix for all path variables:
-set(SRC_ROOT "${CMAKE_CURRENT_SOURCE_DIR}/src") -set(CLP_FFI_PY_LIB_SRC_DIR "${SRC_ROOT}/clp_ffi_py") -set(CLP_CORE_ROOT "${SRC_ROOT}/clp/components/core") +set(CLP_FFI_PY_SRC_ROOT "${CMAKE_CURRENT_SOURCE_DIR}/src") +set(CLP_FFI_PY_LIB_SRC_DIR "${CLP_FFI_PY_SRC_ROOT}/clp_ffi_py") +set(CLP_FFI_PY_CORE_ROOT "${CLP_FFI_PY_SRC_ROOT}/clp/components/core")
62-63: Fix typo and clarify header comment.The comment has a typo and could be clearer about header inclusion:
-# Note: we don't add headers hear since they are not considered to be a part of the project. +# Note: CLP core headers are not included as they are treated as external dependencies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- CMakeLists.txt (1 hunks)
🔇 Additional comments (3)
CMakeLists.txt (3)
1-31: LGTM! Well-structured project configuration.The conditional logic effectively handles both scikit-build-core and IDE/development scenarios, with clear documentation and appropriate version constraints.
33-37: LGTM! Proper project setup with PIC enabled.The project declaration and PIC configuration are correctly set up for building C-extensions.
108-136: LGTM! Well-configured target setup.The target configuration is correct, with proper include directory handling and conditional installation.
CMakeLists.txt
Outdated
|
|
||
| target_compile_features(${CLP_FFI_PY_LIB_IR} PRIVATE cxx_std_20) | ||
|
|
||
| set(SRC_ROOT "${CMAKE_CURRENT_SOURCE_DIR}/src" CACHE STRING "Root path of C++ source code") |
There was a problem hiding this comment.
@LinZhihao-723 Seems this comment wasn't addressed?
Co-authored-by: kirkrodrigues <2454684+kirkrodrigues@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
Taskfile.yml (1)
42-44: LGTM: Build configuration files are properly trackedThe addition of CMakeLists.txt and pyproject.toml to the sources list ensures proper rebuilds when build configurations change.
Consider tracking additional build-related files that might affect the build process:
- "CMakeLists.txt" - "clp_ffi_py/**/*" - "pyproject.toml" + - "cmake/**/*.cmake" # Track any additional CMake modules - "src/**/*"CMakeLists.txt (2)
1-33: LGTM! Well-structured project setup.The CMake version range and conditional project setup are well-organized. The code effectively handles both scikit-build-core and standalone CMake builds.
Consider adding a brief comment explaining why 3.26 was chosen as the minimum version, such as:
+# CMake 3.26+ provides enhanced Python support and modern build features cmake_minimum_required(VERSION 3.26...3.30)
130-137: Add documentation for the installation path.While the installation logic is correct, it would be helpful to document why the library is installed under the
irsubdirectory.if(CLP_FFI_PY_INSTALL_LIBS) + # Install the native IR library under the 'ir' subdirectory to maintain + # a clean package structure and avoid naming conflicts install( TARGETS ${CLP_FFI_PY_LIB_IR}.github/workflows/build_wheels.yml (1)
64-66: Use long-form option for list-expansion flag.For consistency with other flags, consider using the long-form option:
- - run: | - gersemi --check --line-length 100 --list-expansion favour-expansion CMakeLists.txt + - run: | + gersemi --check --line-length 100 --list-expansion-mode favour-expansion CMakeLists.txt
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- .github/workflows/build_wheels.yml (3 hunks)
- CMakeLists.txt (1 hunks)
- Taskfile.yml (2 hunks)
- pyproject.toml (3 hunks)
🔇 Additional comments (5)
pyproject.toml (2)
2-3: LGTM: Build system configuration is properly updatedThe transition to scikit-build-core is well-configured with an appropriate minimum version requirement.
68-70: LGTM: MacOS deployment target is well-documentedThe MacOS deployment target is properly set and documented with a clear explanation of the requirement for std::from_chars support.
Taskfile.yml (1)
32-32: LGTM: Build directory configuration is properly isolatedThe SKBUILD_BUILD_DIR environment variable is correctly configured to use the wheel build directory, ensuring proper isolation of build artifacts.
Let's verify the build directory configuration:
✅ Verification successful
Build directory configuration is correctly implemented
The SKBUILD_BUILD_DIR environment variable is properly configured and aligns with scikit-build-core's requirements:
- It's correctly set to use WHEEL_BUILD_DIR (build/dist)
- The pyproject.toml confirms scikit-build-core as the build backend
- No conflicting build directory configurations exist in CMake
- The build hierarchy is well-structured with clear separation between different build artifacts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the build directory configuration aligns with scikit-build-core requirements # Test: Check if pyproject.toml references the same build directory rg -l 'SKBUILD_BUILD_DIR' pyproject.toml # Test: Ensure no conflicting build directory configurations exist rg -l 'build-dir|build_dir' pyproject.toml CMakeLists.txtLength of output: 100
Script:
#!/bin/bash # Let's verify the build configuration more comprehensively # Check pyproject.toml content for build settings echo "=== pyproject.toml contents ===" cat pyproject.toml # Check Taskfile.yml for build directory usage echo -e "\n=== Taskfile.yml build directory references ===" rg "BUILD_DIR" Taskfile.yml # Check for any CMake build directory settings echo -e "\n=== CMake build directory settings ===" rg "BUILD_DIR|CMAKE_BINARY_DIR" CMakeLists.txt # Look for any other build-related environment variables echo -e "\n=== Other build-related environment variables ===" rg "export.*BUILD" Taskfile.ymlLength of output: 3673
CMakeLists.txt (1)
109-128: LGTM! Well-organized target configuration.The target configuration is properly structured with:
- Clear source organization
- Appropriate system header marking
- Correct library linking
.github/workflows/build_wheels.yml (1)
10-10: LGTM: Path changes align with build system transition.The addition of
CMakeLists.txtto the workflow triggers is consistent with the transition to a CMake-based build system using scikit-build-core.Also applies to: 23-23
pyproject.toml
Outdated
|
|
||
| [tool.scikit-build] | ||
| cmake.build-type = "Release" | ||
| logging.level = "INFO" | ||
| # By default, sdist-build-core uses `.gitignore` to select what to exclude from the source | ||
| # distribution. This list is to explicitly exclude files not specified in `.gitignore`. | ||
| sdist.exclude = [ | ||
| ".*", | ||
| "cmake-build-*", | ||
| "docs", | ||
| "Taskfile.yml", | ||
| ] |
There was a problem hiding this comment.
💡 Codebase verification
Exclusion patterns need adjustment to prevent excluding essential files
The current exclusion patterns are too broad and would exclude necessary files:
.*pattern would exclude all files starting with a dot, including essential files like.gitignore- Current patterns match source files that should be included in the distribution:
docs/README.md- Documentation filesCMakeLists.txt- Build configuration- Other essential Python and C++ source files
Recommend updating the exclusion patterns to be more specific:
sdist.exclude = [
".git",
".github",
"cmake-build-*",
"docs/build",
"docs/src",
"Taskfile.yml",
]🔗 Analysis chain
LGTM: Scikit-build configuration is comprehensive
The configuration includes all necessary components:
- Appropriate build type for distribution
- Recommended logging level
- Well-documented exclusion list for source distribution
Let's verify the exclusion patterns work as intended:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify exclusion patterns don't accidentally exclude required files
# Description: Check if any important files match the exclusion patterns
echo "Checking for potentially problematic exclusions..."
for pattern in ".*" "cmake-build-*" "docs" "Taskfile.yml"; do
echo "Testing pattern: $pattern"
fd "$pattern"
done
Length of output: 3526
CMakeLists.txt
Outdated
| set(CLP_FFI_PY_SRC_ROOT "${CMAKE_CURRENT_SOURCE_DIR}/src") | ||
| set(CLP_FFI_PY_LIB_SRC_DIR "${CLP_FFI_PY_SRC_ROOT}/clp_ffi_py") | ||
| set(CLP_CORE_ROOT "${CLP_FFI_PY_SRC_ROOT}/clp/components/core") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize path variable naming.
For consistency with the project's naming convention, all variables should be prefixed with CLP_FFI_PY_.
set(CLP_FFI_PY_SRC_ROOT "${CMAKE_CURRENT_SOURCE_DIR}/src")
set(CLP_FFI_PY_LIB_SRC_DIR "${CLP_FFI_PY_SRC_ROOT}/clp_ffi_py")
-set(CLP_CORE_ROOT "${CLP_FFI_PY_SRC_ROOT}/clp/components/core")
+set(CLP_FFI_PY_CORE_ROOT "${CLP_FFI_PY_SRC_ROOT}/clp/components/core")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| set(CLP_FFI_PY_SRC_ROOT "${CMAKE_CURRENT_SOURCE_DIR}/src") | |
| set(CLP_FFI_PY_LIB_SRC_DIR "${CLP_FFI_PY_SRC_ROOT}/clp_ffi_py") | |
| set(CLP_CORE_ROOT "${CLP_FFI_PY_SRC_ROOT}/clp/components/core") | |
| set(CLP_FFI_PY_SRC_ROOT "${CMAKE_CURRENT_SOURCE_DIR}/src") | |
| set(CLP_FFI_PY_LIB_SRC_DIR "${CLP_FFI_PY_SRC_ROOT}/clp_ffi_py") | |
| set(CLP_FFI_PY_CORE_ROOT "${CLP_FFI_PY_SRC_ROOT}/clp/components/core") |
| set(CLP_FFI_PY_LIB_IR "native") | ||
| python_add_library( | ||
| ${CLP_FFI_PY_LIB_IR} | ||
| MODULE | ||
| WITH_SOABI | ||
| ) | ||
|
|
||
| target_compile_features(${CLP_FFI_PY_LIB_IR} PRIVATE cxx_std_20) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a more descriptive library name.
The library name "native" is quite generic and might cause confusion in larger projects. Consider using a more specific name that reflects its purpose.
-set(CLP_FFI_PY_LIB_IR "native")
+set(CLP_FFI_PY_LIB_IR "clp_ffi_py_native")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| set(CLP_FFI_PY_LIB_IR "native") | |
| python_add_library( | |
| ${CLP_FFI_PY_LIB_IR} | |
| MODULE | |
| WITH_SOABI | |
| ) | |
| target_compile_features(${CLP_FFI_PY_LIB_IR} PRIVATE cxx_std_20) | |
| set(CLP_FFI_PY_LIB_IR "clp_ffi_py_native") | |
| python_add_library( | |
| ${CLP_FFI_PY_LIB_IR} | |
| MODULE | |
| WITH_SOABI | |
| ) | |
| target_compile_features(${CLP_FFI_PY_LIB_IR} PRIVATE cxx_std_20) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
README.md (2)
Line range hint
11-13: Consider consolidating Python version compatibility informationThe Python version requirements are currently mentioned in two places: the important notice and the compatibility section. Consider consolidating this information into a single section to avoid potential confusion and maintenance overhead.
Here's a suggested reorganization:
## Compatibility -Tested on Python 3.7, 3.8, 3.11 and 3.12, and it should work on any Python -version >= 3.7. +> [!IMPORTANT] +> This project requires Python 3.7 or higher and is no longer compatible with Python 3.6. +> +> Tested on Python versions: +> - 3.7 +> - 3.8 +> - 3.11 +> - 3.12Also applies to: 31-33
🧰 Tools
🪛 Markdownlint
299-299: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
300-300: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
302-302: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
303-303: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
302-303: Maintain consistent markdown list styleThe contributing section uses asterisks (*) for lists while other sections use dashes (-). Consider using dashes consistently throughout the document.
-* [gersemi][9]: `gersemi -i -l 100 --list-expansion favour-expansion CMakeLists.txt` +- [gersemi][9]: `gersemi -i -l 100 --list-expansion favour-expansion CMakeLists.txt` - * This formats CMakeLists.txt. You should review and add any changes to your PR. + - This formats CMakeLists.txt. You should review and add any changes to your PR.🧰 Tools
🪛 Markdownlint
302-302: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
303-303: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- README.md (2 hunks)
🧰 Additional context used
🪛 Markdownlint
README.md
302-302: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
303-303: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
🔇 Additional comments (1)
README.md (1)
302-303: LGTM: Build tool documentation updatesThe addition of gersemi for CMake formatting aligns well with the transition to scikit-build-core and CMake-based builds.
🧰 Tools
🪛 Markdownlint
302-302: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
303-303: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
CMakeLists.txt (1)
47-52: Consider using a more descriptive library name.The library name "native" is quite generic. Consider using a more specific name that reflects its purpose, such as "clp_ffi_py_native".
-set(CLP_FFI_PY_LIB_IR "native") +set(CLP_FFI_PY_LIB_IR "clp_ffi_py_native")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- .gitignore (1 hunks)
- CMakeLists.txt (1 hunks)
- pyproject.toml (3 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🔇 Additional comments (6)
CMakeLists.txt (6)
1-31: LGTM! Well-structured project configuration.The conditional setup effectively handles both scikit-build-core and IDE scenarios, with appropriate version constraints and clear documentation.
33-37: LGTM! Proper project declaration with PIC enabled.The project declaration is correct, and the position-independent code setting is appropriately configured with a clear explanation.
56-61: LGTM! Well-organized directory structure.The source directories are properly defined using absolute paths, and the subdirectory inclusion is correct.
62-107: LGTM! Clear separation of source files with helpful documentation.The source files are well-organized into core and library components, with clear documentation explaining the header file handling strategy.
109-128: LGTM! Proper target configuration with well-documented include handling.The target configuration is correct, with appropriate source inclusion and well-documented system header handling to suppress compiler warnings.
130-137: LGTM! Proper conditional installation setup.The installation configuration correctly handles the conditional installation and follows Python package conventions for the installation path.
LinZhihao-723
changed the title
setuptools by scikit-build-core with CMake.setuptools with scikit-build-core and CMake.
References
Description
This PR replaces
setuptoolsby scikit-build-core.scikit-build-coreuses CMake to build C-extensions and install the built libraries. This PR adds a CMake file following the guidelines fromscikit-build-core's official documentation. As a result, the old setuptool-based build files are all removed, includingsetup.pyandMANIFEST. With the new build system, the manual build should still work (withpython3 -m build), as well ascibuildwheelGitHub workflow. This PR also adds linting guideline + CI check for CMake configs.Something to notice:
scikit-build-core, we still relies on gitsubmodule to manage dependencies at this moment.MANIFESTwithsetuptools.sci-kit-build-coreto build and install C-extension shared libraries properly. To enable us loading the CMake project into CLion easily, or to generate a compile DB, we also make it work withoutscikit-build-core, but in that way the libraries can't be automatically installed.Validation performed
Summary by CodeRabbit
Summary by CodeRabbit
New Features
gersemifor formatting CMake files.Bug Fixes
Chores
setup.pyfile as part of the transition to the new build system.MANIFEST.infile, affecting package distribution inclusions.Taskfile.ymlfor improved build process..gitignorefor build artifacts generated by CMake.