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

Add support for packaging VTK SDK as a wheel #2

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

AlexyPellegrini
Copy link
Collaborator

@AlexyPellegrini AlexyPellegrini commented Mar 25, 2024

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?

@jcfr
Copy link
Member

jcfr commented Mar 26, 2024

we are supposed to propagate the version using the dynamic metadata, for now it seems to always be v0.1

There are two aspects to consider:

  • The version of the SDK to download will be hardcoded in the CMakeLists.txt
  • The version version associated with the wheel will be "dynamically" set based on the git tag

This pulls a tar.xz archive from https://vtk.org/files/wheel-sdks and
package it as a wheel using scikit-build-core.

This wheel will then be added to a pip repository to be fetch through build
requirements.
@AlexyPellegrini
Copy link
Collaborator Author

Hmm it seems that pylint triggers on VTK files, should we disable it since we won't have any python code?

@AlexyPellegrini
Copy link
Collaborator Author

we are supposed to propagate the version using the dynamic metadata, for now it seems to always be v0.1

There are two aspects to consider:

  • The version of the SDK to download will be hardcoded in the CMakeLists.txt
  • The version version associated with the wheel will be "dynamically" set based on the git tag

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 ?

src/vtk_sdk/cmake/__init__.py Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
cmake/vtk-sdk-urls.cmake Outdated Show resolved Hide resolved
@jcfr
Copy link
Member

jcfr commented Mar 27, 2024

a script [...] to update the version in different files, is this something that should be done for this repo too?

Definitely, we should look into adding a script called update_vtk_sdk_version.py that would be similar to update_cmake_version.py and also have a logic similar to the following:

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

jcfr added 2 commits March 27, 2024 10:53
Do not rely on scikit-build-core automatic package discovery and
explicitly install content of the vtk-sdk archive into a dedicated
sub-directory.
@jcfr
Copy link
Member

jcfr commented Mar 27, 2024

Hmm it seems that pylint triggers on VTK files, should we disable it since we won't have any python code?

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.

Copy link

codecov bot commented Mar 27, 2024

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 ☂️

@AlexyPellegrini
Copy link
Collaborator Author

AlexyPellegrini commented Mar 27, 2024

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.
There is an additional "cpN" in the windows url, I think that's because on WIndows the Python SOABI already contains "cpN" at the beginning.

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)

jcfr added 2 commits March 27, 2024 11:57
Anticipating the introduction of checksum verification, the vtk-sdk archive
version will be a static parameter.
@jcfr jcfr force-pushed the add-sdk-build branch 3 times, most recently from 1b5a7bc to 8e1ac88 Compare March 27, 2024 17:46
@jcfr jcfr linked an issue Mar 27, 2024 that may be closed by this pull request
7 tasks
@AlexyPellegrini
Copy link
Collaborator Author

I'm not sure how I am supposed to fix this error:
tests/test_find_package.py:10: error: Cannot find implementation or library stub for module named "virtualenv" [import-not-found]
I tried to add virtualenv as a test dependency, but it does not seem to fix the issue.

@AlexyPellegrini AlexyPellegrini force-pushed the add-sdk-build branch 7 times, most recently from bbfd242 to 37784fc Compare March 29, 2024 15:44
@AlexyPellegrini
Copy link
Collaborator Author

I'm not sure how I am supposed to fix this error: tests/test_find_package.py:10: error: Cannot find implementation or library stub for module named "virtualenv" [import-not-found] I tried to add virtualenv as a test dependency, but it does not seem to fix the issue.

It seems that virtualenv has no type information so I added an exclusion in the code.
I also added it in the tests' requirements and the CI additional_dependencies

I'm not sure I use the best approach, but it seems to work! At least on Mac and Windows. There is an issue on Ubuntu because it can't find OpenGL

@AlexyPellegrini
Copy link
Collaborator Author

Ready to merge!

@AlexyPellegrini AlexyPellegrini force-pushed the add-sdk-build branch 8 times, most recently from 45860bb to 4a91650 Compare February 4, 2025 15:07
Version is both fetch from build system and hardcoded to ensure everything
is coherent.
@AlexyPellegrini
Copy link
Collaborator Author

@jcfr Can you please do a quick review for this PR so we can move forward.

I removed hash check because hash text files are not generated automatically, so they are not available for latest versions of VTK. Let's address this issue in another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for re-packaging and distributing existing VTK wheel SDKs
2 participants