Trying to build wheels for more systems#61
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the version to 3.0.0-beta.2 and expands the CI/CD pipeline to build Python wheels for multiple platforms (Linux, Windows, and macOS) instead of just Linux.
- Version bumped from 3.0.0-beta.1 to 3.0.0-beta.2
- Added dedicated build jobs for Windows and macOS platforms
- Updated artifact upload/download to handle wheels from all three platforms
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| python/src/sista/init.pyi | Updated version string to 3.0.0-beta.2 |
| python/src/sista/init.py | Updated version string to 3.0.0-beta.2 |
| .github/workflows/python-publish.yml | Added Windows and macOS build jobs, updated artifact handling for multi-platform wheels |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| steps: | ||
| - uses: actions/checkout@v4 | ||
|
|
There was a problem hiding this comment.
The make install command will fail on Windows runners because Make is not pre-installed on windows-latest runners. While the Makefile includes Windows-specific install targets, the Make tool itself needs to be installed first. Consider either: installing Make via chocolatey (choco install make), using nmake with Visual Studio, or implementing a PowerShell-based installation script for Windows.
| - name: Install Make | |
| run: choco install make -y | |
| - name: Upload wheels (windows) | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: release-wheels | ||
| name: release-wheels-windows | ||
| path: python/wheelhouse/ |
There was a problem hiding this comment.
The upload path is inconsistent with where cibuildwheel will create the wheelhouse directory. Since cibuildwheel is run from the root directory without changing to the python directory, it will create the wheelhouse directory in the repository root, not in python/wheelhouse/. This will cause the upload to fail because python/wheelhouse/ won't exist. Either change to the python directory before running cibuildwheel, or update the upload path to just 'wheelhouse/'.
| - name: Upload wheels (macos) | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: release-wheels-macos | ||
| path: python/wheelhouse/ |
There was a problem hiding this comment.
The upload path is inconsistent with where cibuildwheel will create the wheelhouse directory. Since cibuildwheel is run from the root directory without changing to the python directory, it will create the wheelhouse directory in the repository root, not in python/wheelhouse/. This will cause the upload to fail because python/wheelhouse/ won't exist. Either change to the python directory before running cibuildwheel, or update the upload path to just 'wheelhouse/'.
| - name: Build wheels | ||
| # https://cibuildwheel.pypa.io/en/stable/configuration/#environment-variables | ||
| run: | | ||
| cibuildwheel --output-dir wheelhouse |
There was a problem hiding this comment.
The cibuildwheel command is not being executed from the correct directory. The Linux build job changes to the python directory before running cibuildwheel (line 48: cd python), but this Windows job doesn't. Since pyproject.toml is located in the python directory, cibuildwheel needs to be run from there to find the package configuration. Either change the working directory before running cibuildwheel or specify the path as an argument.
| run: | | ||
| python -m pip install cibuildwheel | ||
|
|
||
| - name: Build wheels | ||
| run: | |
There was a problem hiding this comment.
The cibuildwheel command is not being executed from the correct directory. The Linux build job changes to the python directory before running cibuildwheel (line 48: cd python), but this macOS job doesn't. Since pyproject.toml is located in the python directory, cibuildwheel needs to be run from there to find the package configuration. Either change the working directory before running cibuildwheel or specify the path as an argument.
| run: | | |
| python -m pip install cibuildwheel | |
| - name: Build wheels | |
| run: | | |
| run: | | |
| cd python | |
| python -m pip install cibuildwheel | |
| - name: Build wheels | |
| run: | | |
| cd python |
|
|
||
| steps: | ||
| - uses: actions/checkout@v4 | ||
|
|
There was a problem hiding this comment.
The make install command will fail on Windows runners as they don't come with Make pre-installed. Windows runners typically use cmd.exe or PowerShell, and while the Makefile includes Windows support, the Make tool itself is not available by default. Consider using a Windows-specific installation approach or installing Make (e.g., via choco install make) before attempting to run this command.
| - name: Install Make on Windows | |
| run: choco install make -y |
…d step for the Python module
…ave researched until now, and the windows-latest lacks the MinGW toolchain
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| PKG_CONFIG_PATH: ${{ github.workspace }}/python/_install/lib/pkgconfig | ||
| DYLD_LIBRARY_PATH: ${{ github.workspace }}/python/_install/lib | ||
| CIBW_BEFORE_BUILD_MACOS: "" # To prevent cibuildwheel from trying to install Sista again | ||
| MACOSX_DEPLOYMENT_TARGET: 15.0 # delocate.libsana.DelocationError: Library dependencies do not satisfy target MacOS version 11.0 |
There was a problem hiding this comment.
The MACOSX_DEPLOYMENT_TARGET is set to 15.0, which is macOS Sequoia released in September 2024. This is an extremely recent version that will exclude most macOS users. Consider using a lower target like 11.0 or 12.0 to maintain broader compatibility with older macOS systems. If there's a specific technical reason for requiring 15.0, it should be documented.
| MACOSX_DEPLOYMENT_TARGET: 15.0 # delocate.libsana.DelocationError: Library dependencies do not satisfy target MacOS version 11.0 | |
| MACOSX_DEPLOYMENT_TARGET: 11.0 # Baseline macOS target; see delocate.libsana.DelocationError note for dependencies that must support 11.0 |
There was a problem hiding this comment.
That's what the output error told me to do, tho. We may try retrocompatibility later.
| ] if not is_windows else [ | ||
| r"C:\Program Files\Sista\include" | ||
| ], |
There was a problem hiding this comment.
Hardcoded path "C:\Program Files\Sista\include" may not work in all Windows environments, especially when spaces in paths can cause issues with some build tools. Additionally, this path won't match the PREFIX used in the commented-out Windows build job which uses "${{ github.workspace }}\python\_install". Consider making this configurable via an environment variable or using a path without spaces.
| ] if not is_windows else [ | ||
| r"C:\Program Files\Sista\lib" | ||
| ], |
There was a problem hiding this comment.
Hardcoded path "C:\Program Files\Sista\lib" may not work in all Windows environments. This should be consistent with the include_dirs configuration and ideally made configurable via an environment variable to support different installation locations.
No description provided.