Use scikit-build-core instead of scikit-build #95

lkeegan · 2024-03-20T09:47:27Z

clang-format_version.cmake -> clang-format_version.txt
- contains llvm_version.wheel_version, e.g. 18.0.2.1
build logic is now contained in pyproject.toml + CMakeLists.txt
pyproject.toml
- version parsed from clang-format_version.txt (wheel_version is omitted if zero)
CMakeLists.txt
- version is taken from pyproject.toml
- clang-format-diff and git-clang-format are now installed with valid python module names
- specify Release config as build argument of clang-format to ensure we get a release build with msvc
- include Release sub-folder in clang-format binary location when building with msvc
- set LLVM_TARGETS_TO_BUILD empty to reduce llvm compile time by only building default host target instead of the default all targets
remove code to call bundled python scripts, directly point to script entry points in pyproject.toml
remove setup.py and MANIFEST.in
disable fail-fast and use latest runners in CI
resolves Change build system to scikit-build-core? #80

- clang-format_version.cmake -> clang-format_version.txt - contains `llvm_version.wheel_version`, e.g. `18.0.2.1` - build logic is now contained in pyproject.toml + CMakeLists.txt - pyproject.toml - version parsed from clang-format_version.txt (wheel_version is omitted if zero) - CMakeLists.txt - version is taken from pyproject.toml - clang-format-diff and git-clang-format are now installed with valid python module names - specify Release config as build argument of clang-format to ensure we get a release build with msvc - include Release sub-folder in clang-format binary location when building with msvc - set `LLVM_TARGETS_TO_BUILD` empty to reduce llvm compile time by only building default host target instead of the default all targets - remove code to call bundled python scripts, directly point to script entry points in pyproject.toml - remove setup.py and MANIFEST.in - disable fail-fast and use latest runners in CI - resolves #80

lkeegan · 2024-03-21T07:11:47Z

@dokempf the CI now seems to work (apart from the test-pypi upload but I guess that is not related to these changes?): https://github.com/ssciwr/clang-format-wheel/actions/runs/8361576686

lkeegan · 2024-03-21T08:26:57Z

The pypi issue was a bug in this pr, now fixed: https://github.com/ssciwr/clang-format-wheel/actions/runs/8371384624

dokempf

Looks great, I mostly have some questions.

dokempf · 2024-03-21T09:31:09Z

.github/workflows/release.yml

@@ -29,30 +29,31 @@ jobs:
    runs-on: ${{ matrix.os }}

    strategy:
+      fail-fast: false


What is the reason for not failing fast? If the fast builds fail, we can save quite a bit of resources by stopping the slow ones.

This was just for debugging convenience when different builds were failing for different reasons so I wanted to see them all (try to) finish - I'll revert this

dokempf · 2024-03-21T09:32:57Z

CMakeLists.txt

@@ -18,10 +18,14 @@ ExternalProject_add(build-clang-format
  USES_TERMINAL_DOWNLOAD 1
  USES_TERMINAL_CONFIGURE 1
  USES_TERMINAL_BUILD 1
-  CMAKE_ARGS -DCMAKE_BUILD_TYPE=Release -DBUILD_SHARED_LIBS=OFF -DLLVM_ENABLE_ZLIB=OFF -DLLVM_ENABLE_ZSTD=OFF -DLLVM_ENABLE_PROJECTS=clang
-  BUILD_COMMAND ${CMAKE_COMMAND} --build . --target clang-format
+  CMAKE_ARGS -DCMAKE_BUILD_TYPE=Release -DBUILD_SHARED_LIBS=OFF -DLLVM_ENABLE_ZLIB=OFF -DLLVM_ENABLE_ZSTD=OFF -DLLVM_ENABLE_PROJECTS=clang -DLLVM_TARGETS_TO_BUILD=


Can you give some insight into the additional -DLLVM_TARGETS_TO_BUILD=? What does it do?

This is list of CPU targets the clang we are building should be able to generate binaries for, e.g. x86, arm, etc
By default this is all supported targets, setting it to empty means it will only use the host architecture, so there are (slightly) fewer files to compile.

dokempf · 2024-03-21T09:34:56Z

CMakeLists.txt

+if(CMAKE_CXX_COMPILER_ID STREQUAL "MSVC")
+  set(config-subfolder "Release")
+endif()
+set(clang-format-executable ${CMAKE_BINARY_DIR}/llvm/${config-subfolder}/bin/clang-format${CMAKE_EXECUTABLE_SUFFIX})


Do we have any idea why this worked before? Is it a scikit-build vs. -core difference in default CMake arguments?

Looking into this again I think the difference is that scikit-build was using Ninja on windows, and scikit-build-core is using MSVC. I'll try to fix this as ninja is faster and then we don't need this config-subfolder cruft.

This ended up being a bit of a rabbit hole:

scikit-build-core uses the default compiler in your environment (unlike scikit-build which has its own logic for choosing the compiler and generator and ignores your defaults/environment)

it also does cmake --help and parses this to find the default generator

on windows by default this works fine with visual studio generator + mvsc compiler

if you set ninja as the generator via a cmake arg this doesn't alter the cmake --help default generator logic and you get an error due to inconsistent setting of the generator

if you set ninja as the generator via an env var this also modifies the cmake --help default generator and it works, but now the default compiler (at least on GHA windows CI) changes to mingw gcc for whatever reason

so you also need to set up MSVC as the default compiler on windows CI

tldr: ninja is now used on windows ci, but from source builds using the visual studio generator should also work

dokempf · 2024-03-21T09:40:16Z

CMakeLists.txt

    ${CMAKE_BINARY_DIR}/llvm-project/clang/tools/clang-format/git-clang-format
-  DESTINATION clang_format/data/bin
+  RENAME git_clang_format.py
+  DESTINATION clang_format


Interesting approach, but I think it solves all the issues we had in #36.

… config-subfolder logic

…enerator config-subfolder logic" This reverts commit dd88159.

… when compiling

…rms in CI - note: setting this in the scikit-build-core cmake args is not sufficient - scikit-build-core uses the default cmake generator on windows, which is modified by the CMAKE_GENERATOR env var - fix config-subfolder logic to only apply when Visual Studio is used as generator - ensure MSVC is the active default compiler on windows CI - scikit-build-core uses whatever the current default is (while scikit-build always uses MSVC)

lkeegan force-pushed the use_scikit_build_core branch from d5b625e to 720ce5c Compare March 20, 2024 13:08

lkeegan force-pushed the use_scikit_build_core branch from 720ce5c to fa91c60 Compare March 20, 2024 15:29

lkeegan requested a review from dokempf March 21, 2024 07:05

fix project name in pyproject.toml

cef7fa7

dokempf approved these changes Mar 21, 2024

View reviewed changes

lkeegan added 5 commits March 21, 2024 13:27

set CMAKE_GENERATOR to Ninja for all platforms, remove MSVC generator…

dd88159

… config-subfolder logic

Revert "set CMAKE_GENERATOR to Ninja for all platforms, remove MSVC g…

bd5d848

…enerator config-subfolder logic" This reverts commit dd88159.

set CMAKE_BUILD_PARALLEL_LEVEL env var on CI to make MVSC use 4 cores…

c9d2fe5

… when compiling

remove fail-fast from CI

bbdc755

dokempf merged commit afba506 into main Mar 21, 2024
17 checks passed

dokempf deleted the use_scikit_build_core branch March 21, 2024 15:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use scikit-build-core instead of scikit-build #95

Use scikit-build-core instead of scikit-build #95

lkeegan commented Mar 20, 2024 •

edited

Loading

lkeegan commented Mar 21, 2024

lkeegan commented Mar 21, 2024

dokempf left a comment

dokempf Mar 21, 2024

lkeegan Mar 21, 2024

dokempf Mar 21, 2024

lkeegan Mar 21, 2024

dokempf Mar 21, 2024

lkeegan Mar 21, 2024

lkeegan Mar 21, 2024

dokempf Mar 21, 2024

Use scikit-build-core instead of scikit-build #95

Use scikit-build-core instead of scikit-build #95

Conversation

lkeegan commented Mar 20, 2024 • edited Loading

lkeegan commented Mar 21, 2024

lkeegan commented Mar 21, 2024

dokempf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkeegan commented Mar 20, 2024 •

edited

Loading