-
Notifications
You must be signed in to change notification settings - Fork 3
Add support for packaging VTK SDK as a wheel #2
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
Conversation
b3264a7 to
e6b97de
Compare
There are two aspects to consider:
|
3b56e25 to
674648a
Compare
|
Hmm it seems that pylint triggers on VTK files, should we disable it since we won't have any python code? |
Ok, so we will tag later I guess. For now the version I hardcoded is 9.2.5. If we hardcode the version maybe I can bring back the hashes of the archives. Can we use the python 3.8 archive version for any newerpython version, or should each version match exactly ? |
Definitely, we should look into adding a script called vtk_version = "9.3.0"
for py_abi_tag in ["cp38-cp38", "cp39-cp39", "cp310-cp310", "cp311-cp311", "cp312-cp312"]:
for platform_tag in ["macosx_10_10_x86_64", "macosx_11_0_arm64", "manylinux_2_17_x86_64.manylinux2014_x86_64", "win_amd64"]:
url = f"https://vtk.org/files/wheel-sdks/vtk-wheel-sdk-{vtk_version}-{py_abi_tag}-{platform_tag}.tar.xz"
# Download files
# -> Adapt from https://github.com/girder/slicer_package_manager/blob/342b48d7b6fdf407731513c30f5dc147eca30d5b/tests/__init__.py#L324-L334
# Compute checksum
# -> See "computeFileChecksum" at https://github.com/girder/slicer_package_manager/blob/342b48d7b6fdf407731513c30f5dc147eca30d5b/tests/__init__.py#L232-L254 |
I ended up addressing the "root" cause by installing the content of the archive in a dedicated directory without relying on the automatic discovery of wheel packages. |
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
|
Thank you for making the required changes! It seems that the mac runners fail because the platform tag (macosx_12_0...) is not the same as the one on the sdk repo. We probably need to the same thing as we do for linux. I'm working on the script to generate the cmake urls file so we will get rid of the problem by changing the logic I think (since the URLs will be hardcoded) |
8e1ac88 to
47488c9
Compare
|
Before moving forward with the integration, we should add a test checking that a simple project can be built against the generated wheel. Add files like the following: Consider borrowing some of the testing logic found here:
In this simple test, the |
ea7aca1 to
85ca0b8
Compare
|
I'm not sure how I am supposed to fix this error: |
37784fc to
9a840d6
Compare
d6511e7 to
61a1344
Compare
- Explicitly set setuptools_scm to derive the version from the nearest tag
name only:
git describe --tags --abbrev=0 --match "*[0-9]*"
This yields a clean tag (e.g., 9.5.0) without distance/hash,
making wheels/tag-based releases deterministic.
- In CMake, compare it with VTK_VERSION and emit a warning if they differ.
Trade-off: by removing distance/hash, non-tag builds will look like the
nearest tag. This is intentional here to keep package versions aligned
with SDK tags.
Co-authored-by: Jean-Christophe Fillion-Robin <[email protected]>
This drops support for Python 3.8 and 3.9 as they are nearing or have reached end-of-life, and adds support for Python 3.12 and 3.13: - Python 3.8 is end-of-life since 2024-10-07 - Python 3.9 will be end-of-life at the end of 2025-10 See https://devguide.python.org/versions Co-authored-by: Jean-Christophe Fillion-Robin <[email protected]>
This adds a test project that verifies the ability to use `find_package(VTK REQUIRED)` in a scikit-build-core-based Python package. - Adds `CMakeLists.txt` and `pyproject.toml` under `tests/packages/find_package` - Creates a minimal Python package (`vtk_simple`) as the test subject - Implements `test_find_package.py` to build the wheel using `pip wheel` in an isolated virtualenv with a custom wheelhouse if provided - Install vtk-sdk package before using it Co-authored-by: Jean-Christophe Fillion-Robin <[email protected]>
c46bc96 to
f84bd0a
Compare
412e940 to
872d254
Compare
Since `Python_SOABI` is set to value like `cpython-XYZ-darwin`, instead checks the value of `CMAKE_SYSTEM_PROCESSOR`.
|
@AlexyPellegrini Can you check that the wheels1 work as expected. Then we will merge & tag so that those are uploaded on PyPI. cc: @finetjul Footnotes |
NotesChecksums: I re-inroduced the use of checksums. This is critical to ensure integrity. After locally downloading the wheels .. the following is helpful: CI & Runners:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for packaging the VTK SDK as a Python wheel by downloading pre-built VTK SDK archives, extracting them, and installing their contents along with CMake configuration files. The implementation removes the previous C++ example code and introduces new infrastructure for distributing VTK SDKs as first-class Python wheels with scikit-build-core integration.
Key changes:
- Replaces C++ example code with VTK SDK packaging infrastructure
- Adds CMake logic to download and install VTK SDK archives based on platform/Python version
- Updates minimum Python version from 3.8 to 3.10 and adds Python 3.13 support
- Introduces comprehensive testing using virtual environments to verify
find_package(VTK)functionality
Reviewed Changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| CMakeLists.txt | Implements VTK SDK download/extraction logic and CMake configuration file installation |
| cmake/vtk-sdk-urls.cmake | Defines platform-specific URLs and SHA256 checksums for VTK SDK archives |
| cmake/vtk-config.cmake.in | Template for CMake config file that redirects to extracted SDK |
| cmake/vtk-config-version.cmake.in | Template for CMake version config file |
| src/vtk_sdk/cmake/init.py | Entry point module for scikit-build-core cmake.prefix |
| src/vtk_sdk/init.py | Updated module description from cmake-module to cmake.prefix |
| tests/test_find_package.py | New test infrastructure for verifying VTK SDK can be found via CMake |
| tests/packages/find_package/* | Test package that uses find_package(VTK) to validate SDK integration |
| pyproject.toml | Updates Python version requirements, entry points, and test dependencies |
| .github/workflows/ci.yml | Updates CI to test Python 3.10-3.13 and adds wheel building checks |
| .github/workflows/cd.yml | Adds SDist testing and distribution checks |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Change `plaform_tag` to `platform_tag` Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
|
Hi @jcfr, Thx for working on this! Will this repo be retired once the changes on VTK are cleanly integrated? |
|
Indeed, once the corresponding capabilities are in upstream VTK, there would be no need to publish the vtk-sdk wheels from this project. I was initially thinking we could publish also publish a custom scikit build backend along side the SDK but this should instead be done in a dedicated GitHub project. Indeed, the goal being to simplify how project like the following are externally built by leveraging |
This is the first batch of code to build the sdk wheel!
The URL of the SDK is composed from the target information, and no hash is checked for now, it was easier to test since my python versions mismatch between windows and linux :)
I removed the c++ code from the repo too.
I'm not sure how we are supposed to propagate the version using the dynamic metadata, for now it seems to always be v0.1.
Also I see that on "cmake-python-distributions" there is a script to update the version in different files, is this something that should be done for this repo too?