Skip to content
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

Replace setuptools with scikit-build-core and CMake. #73

Merged
merged 28 commits into from
Oct 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
b4d0a54
Migrate to cmake build system
LinZhihao-723 Oct 23, 2024
72ef9d7
Fix for macos
LinZhihao-723 Oct 23, 2024
5e0ca90
Update to macos13 and latest cibuildwheel
LinZhihao-723 Oct 23, 2024
9924b86
Now let's try to set env variable in pyproject.toml
LinZhihao-723 Oct 23, 2024
8ee1e10
Revert back to macos12. Leave it to another PR
LinZhihao-723 Oct 23, 2024
9ea9529
Apply code review comments
LinZhihao-723 Oct 23, 2024
231693e
Refactor cmake
LinZhihao-723 Oct 23, 2024
8e83339
Set the min cmake version to scikit's minimum requirement
LinZhihao-723 Oct 23, 2024
f84edd0
Config scikit to package source distribution properly
LinZhihao-723 Oct 23, 2024
35fc78f
Remove excluded src/clp
LinZhihao-723 Oct 23, 2024
c04f8b6
Fix cmake indentation
LinZhihao-723 Oct 24, 2024
8ddfe72
Add cmake linting
LinZhihao-723 Oct 24, 2024
f0db8ce
Merge with oss main
LinZhihao-723 Oct 24, 2024
fab7aa2
Apply code review comments
LinZhihao-723 Oct 24, 2024
721d20b
Apply coderabit's comment
LinZhihao-723 Oct 24, 2024
25414e6
Remove the comment from previous PR
LinZhihao-723 Oct 24, 2024
b042432
Apply suggestions from code review
LinZhihao-723 Oct 25, 2024
e288ceb
Apply code review comments to the cmake file
LinZhihao-723 Oct 25, 2024
02ee863
CRLF to LF
LinZhihao-723 Oct 25, 2024
a773b05
Update workflow file
LinZhihao-723 Oct 25, 2024
d91e8c9
Update task files
LinZhihao-723 Oct 25, 2024
ad05a36
Update pyproject.toml
LinZhihao-723 Oct 25, 2024
d152431
Fix typo
LinZhihao-723 Oct 25, 2024
6aefe22
Apply suggestions from code review
LinZhihao-723 Oct 26, 2024
2e9fe51
Update cmake
LinZhihao-723 Oct 26, 2024
5c76ab3
Merge branch 'oss_main' into scikit-build-core-itegration
LinZhihao-723 Oct 26, 2024
73deb5f
Rename root to dir
LinZhihao-723 Oct 27, 2024
d6b87dc
Apply code review comments
LinZhihao-723 Oct 27, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions .github/workflows/build_wheels.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,11 @@ on:
- ".clang-tidy"
- ".github/workflows/build_wheels.yml"
- ".gitmodules"
- "CMakeLists.txt"
- "clp_ffi_py/**"
- "MANIFEST.in"
- "pyproject.toml"
- "README.md"
- "requirements-dev.txt"
- "setup.py"
- "src/**"
- "tests/**"
push:
Expand All @@ -21,12 +20,11 @@ on:
- ".clang-tidy"
- ".github/workflows/build_wheels.yml"
- ".gitmodules"
- "CMakeLists.txt"
- "clp_ffi_py/**"
- "MANIFEST.in"
- "pyproject.toml"
- "README.md"
- "requirements-dev.txt"
- "setup.py"
- "src/**"
- "tests/**"
schedule:
Expand Down Expand Up @@ -63,6 +61,9 @@ jobs:
- run: |
find src/clp_ffi_py/ -type f | xargs clang-format --dry-run --Werror

- run: |
gersemi --check --line-length 100 --list-expansion favour-expansion CMakeLists.txt

build_wheels:
needs: [linters]
name: Build ${{ matrix.build.name }}
Expand Down
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,6 @@ wheelhouse/
unittest-logs/

# Dev
cmake-build-*
compile_commands.json
.mypy_cache
137 changes: 137 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
cmake_minimum_required(VERSION 3.26...3.30)

# Define the variables necessary to build and install the project differently depending on whether
# the project is being configured from scikit-build-core. If configured from outside
# scikit-build-core, we need to use placeholder values for the variables that are usually defined
# by scikit-build-core.
if(DEFINED SKBUILD_PROJECT_NAME)
set(CLP_FFI_PY_INSTALL_LIBS ON CACHE BOOL "Enable installing the built libraries." FORCE)
set(CLP_FFI_PY_PROJECT_NAME
${SKBUILD_PROJECT_NAME}
CACHE STRING
"The name of the project parsed by scikit-build-core."
FORCE
)
set(CLP_FFI_PY_VERSION
${SKBUILD_PROJECT_VERSION}
CACHE STRING
"The version of the project parsed by scikit-build-core."
FORCE
)
else()
set(CLP_FFI_PY_INSTALL_LIBS OFF CACHE BOOL "Disable installing the built libraries." FORCE)
set(CLP_FFI_PY_PROJECT_NAME "clp-ffi-py" CACHE STRING "Use a placeholder project name." FORCE)
set(CLP_FFI_PY_VERSION "0.0.0" CACHE STRING "Use a placeholder version." FORCE)
set(CMAKE_EXPORT_COMPILE_COMMANDS
ON
CACHE BOOL
"Enable/Disable output of compile commands during generation."
FORCE
)
endif()

project(${CLP_FFI_PY_PROJECT_NAME} LANGUAGES CXX VERSION ${CLP_FFI_PY_VERSION})

# C-extension modules will be compiled as shared libraries. To enable dynamic linking, all libraries
# (including the static ones) must be compiled with the position-independent-code option.
set(CMAKE_POSITION_INDEPENDENT_CODE ON CACHE BOOL "Compile libraries using PIC.")

find_package(
Python
REQUIRED
COMPONENTS
Interpreter
Development.Module
)

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)
Comment on lines +47 to +54
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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)


set(CLP_FFI_PY_SRC_DIR "${CMAKE_CURRENT_SOURCE_DIR}/src")
set(CLP_FFI_PY_LIB_SRC_DIR "${CLP_FFI_PY_SRC_DIR}/clp_ffi_py")
set(CLP_FFI_PY_CLP_CORE_DIR "${CLP_FFI_PY_SRC_DIR}/clp/components/core")

add_subdirectory(${CLP_FFI_PY_CLP_CORE_DIR}/src/clp/string_utils)

# NOTE: We don't add headers here since CLP core is technically a library we're using, not a part of
# this project.
set(CLP_FFI_PY_CLP_CORE_SOURCES
${CLP_FFI_PY_CLP_CORE_DIR}/src/clp/BufferReader.cpp
${CLP_FFI_PY_CLP_CORE_DIR}/src/clp/ffi/ir_stream/decoding_methods.cpp
${CLP_FFI_PY_CLP_CORE_DIR}/src/clp/ffi/ir_stream/encoding_methods.cpp
${CLP_FFI_PY_CLP_CORE_DIR}/src/clp/ffi/ir_stream/utils.cpp
${CLP_FFI_PY_CLP_CORE_DIR}/src/clp/ffi/encoding_methods.cpp
${CLP_FFI_PY_CLP_CORE_DIR}/src/clp/ir/parsing.cpp
${CLP_FFI_PY_CLP_CORE_DIR}/src/clp/ReaderInterface.cpp
)

# NOTE: We include headers to ensure IDEs like CLion load the project properly.
set(CLP_FFI_PY_LIB_IR_SOURCES
${CLP_FFI_PY_LIB_SRC_DIR}/error_messages.hpp
${CLP_FFI_PY_LIB_SRC_DIR}/ExceptionFFI.hpp
${CLP_FFI_PY_LIB_SRC_DIR}/ir/native/deserialization_methods.cpp
${CLP_FFI_PY_LIB_SRC_DIR}/ir/native/deserialization_methods.hpp
${CLP_FFI_PY_LIB_SRC_DIR}/ir/native/LogEvent.hpp
${CLP_FFI_PY_LIB_SRC_DIR}/ir/native/Metadata.cpp
${CLP_FFI_PY_LIB_SRC_DIR}/ir/native/Metadata.hpp
${CLP_FFI_PY_LIB_SRC_DIR}/ir/native/PyDeserializer.cpp
${CLP_FFI_PY_LIB_SRC_DIR}/ir/native/PyDeserializer.hpp
${CLP_FFI_PY_LIB_SRC_DIR}/ir/native/PyDeserializerBuffer.cpp
${CLP_FFI_PY_LIB_SRC_DIR}/ir/native/PyDeserializerBuffer.hpp
${CLP_FFI_PY_LIB_SRC_DIR}/ir/native/PyFourByteSerializer.cpp
${CLP_FFI_PY_LIB_SRC_DIR}/ir/native/PyFourByteSerializer.hpp
${CLP_FFI_PY_LIB_SRC_DIR}/ir/native/PyLogEvent.cpp
${CLP_FFI_PY_LIB_SRC_DIR}/ir/native/PyLogEvent.hpp
${CLP_FFI_PY_LIB_SRC_DIR}/ir/native/PyMetadata.cpp
${CLP_FFI_PY_LIB_SRC_DIR}/ir/native/PyMetadata.hpp
${CLP_FFI_PY_LIB_SRC_DIR}/ir/native/PyQuery.cpp
${CLP_FFI_PY_LIB_SRC_DIR}/ir/native/PyQuery.hpp
${CLP_FFI_PY_LIB_SRC_DIR}/ir/native/Query.cpp
${CLP_FFI_PY_LIB_SRC_DIR}/ir/native/Query.hpp
${CLP_FFI_PY_LIB_SRC_DIR}/ir/native/serialization_methods.cpp
${CLP_FFI_PY_LIB_SRC_DIR}/ir/native/serialization_methods.hpp
${CLP_FFI_PY_LIB_SRC_DIR}/modules/ir_native.cpp
${CLP_FFI_PY_LIB_SRC_DIR}/Py_utils.cpp
${CLP_FFI_PY_LIB_SRC_DIR}/Py_utils.hpp
${CLP_FFI_PY_LIB_SRC_DIR}/PyObjectCast.hpp
${CLP_FFI_PY_LIB_SRC_DIR}/PyObjectUtils.hpp
${CLP_FFI_PY_LIB_SRC_DIR}/Python.hpp
${CLP_FFI_PY_LIB_SRC_DIR}/utils.cpp
${CLP_FFI_PY_LIB_SRC_DIR}/utils.hpp
)

target_sources(
${CLP_FFI_PY_LIB_IR}
PRIVATE
${CLP_FFI_PY_CLP_CORE_SOURCES}
${CLP_FFI_PY_LIB_IR_SOURCES}
)

# NOTE: We mark the include directories below as system headers so that the compiler (including
# `clang-tidy`) doesn't generate warnings from them.
target_include_directories(
${CLP_FFI_PY_LIB_IR}
SYSTEM
PRIVATE
${CLP_FFI_PY_CLP_CORE_DIR}/src
${CLP_FFI_PY_CLP_CORE_DIR}/submodules
)

target_include_directories(${CLP_FFI_PY_LIB_IR} PRIVATE ${CLP_FFI_PY_SRC_DIR})

target_link_libraries(${CLP_FFI_PY_LIB_IR} PRIVATE clp::string_utils)

if(CLP_FFI_PY_INSTALL_LIBS)
install(
TARGETS
${CLP_FFI_PY_LIB_IR}
DESTINATION
${CLP_FFI_PY_PROJECT_NAME}/ir
)
endif()
9 changes: 0 additions & 9 deletions MANIFEST.in

This file was deleted.

9 changes: 3 additions & 6 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,8 @@ formatting tools (found in [pyproject.toml]):
* [ruff][10]: `ruff check --fix clp_ffi_py tests`
* This performs linting according to PEPs. You should review and add any
changes to your PR.
* [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.

Note that `docformatter` should be run before `black` to give Black the
[last][12].
Expand All @@ -312,11 +314,6 @@ other package management tools such as `apt-get`:
rules specified in `.clang-tidy`, and sends suggestions corresponding to
each warning. Developers should manually review all the warnings and try
with their best effort to fix the reasonable ones.
* [bear][9]: `bear python setup.py build`
* This tool generates a JSON compilation database on the project's root
directory named `compile_commands.json`. This file will be used by
clang-tidy to execute the static analysis. It also helps IDEs to configure
code completion (such as VSCode).

[1]: https://github.com/y-scope/clp/tree/main/components/core
[2]: https://github.com/y-scope/clp
Expand All @@ -326,7 +323,7 @@ other package management tools such as `apt-get`:
[6]: https://clang.llvm.org/docs/ClangFormatStyleOptions.html
[7]: https://cibuildwheel.readthedocs.io/en/stable/
[8]: https://clang.llvm.org/extra/clang-tidy/
[9]: https://github.com/rizsotto/Bear
[9]: https://github.com/BlankSpruce/gersemi
[10]: https://beta.ruff.rs/docs/
[11]: https://docformatter.readthedocs.io/en/latest/
[12]: https://docformatter.readthedocs.io/en/latest/faq.html#interaction-with-black
Expand Down
3 changes: 3 additions & 0 deletions Taskfile.yml
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ tasks:
- "rm -rf '{{.OUTPUT_DIR}}'"
- |-
. "{{.WHEEL_VENV_DIR}}/bin/activate"
export SKBUILD_BUILD_DIR="{{.WHEEL_BUILD_DIR}}"
python3 -m build --outdir "{{.OUTPUT_DIR}}"
# Checksum the generated files (this command must be last)
- |-
Expand All @@ -38,7 +39,9 @@ tasks:
- "{{.BUILD_DIR}}/submodules.md5"
- "{{.BUILD_DIR}}/wheel-venv.md5"
- "{{.TASKFILE}}"
- "CMakeLists.txt"
- "clp_ffi_py/**/*"
- "pyproject.toml"
- "src/**/*"
status:
- "test -f '{{.CHECKSUM_FILE}}'"
Expand Down
21 changes: 14 additions & 7 deletions pyproject.toml
Original file line number Diff line number Diff line change
@@ -1,10 +1,6 @@
[build-system]
requires = [
"setuptools >= 59.0",
"setuptools_scm",
"wheel",
]
build-backend = "setuptools.build_meta"
build-backend = "scikit_build_core.build"
requires = ["scikit-build-core >= 0.10.7"]

[project]
name = "clp_ffi_py"
Expand Down Expand Up @@ -69,7 +65,7 @@ archs = ["auto", "aarch64"]
archs = ["x86_64", "universal2", "arm64"]

[tool.cibuildwheel.macos.environment]
# This should be kept in sync with the environment variable in setup.py.
# We need 10.15 since CLP core uses std::from_chars.
MACOSX_DEPLOYMENT_TARGET = "10.15"

[tool.cibuildwheel.windows]
Expand Down Expand Up @@ -97,3 +93,14 @@ line-length = 100
[tool.ruff.lint]
select = ["E", "I", "F"]
isort.order-by-type = false

[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 = [
".*",
"docs",
"Taskfile.yml",
]
1 change: 1 addition & 0 deletions requirements-dev.txt
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ cibuildwheel>=2.16.2
# Lock to v18.x until we can upgrade our code to meet v19's formatting standards.
clang-format~=18.1
docformatter>=1.7.5
gersemi>=0.16.2
mypy>=1.10.0
mypy-extensions>=1.0.0
packaging>=21.3
Expand Down
Loading
Loading