Merged
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR updates the configuration for rustup by revising the valid Windows targets and ensuring that the workflow files reflect these changes during the build and publish processes.
- Updated pyproject.toml to remove the unsupported "i586-pc-windows-msvc" target.
- Switched wheel building in CI workflows to utilize cibuildwheel.
- Added explicit rustup target addition commands in both the build and publish workflows.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pyproject.toml | Updates the Windows target list for rustup with valid targets. |
| .github/workflows/python-package.yml | Transitions wheel building to cibuildwheel and adds rustup target commands. |
| .github/workflows/package-publish.yml | Adds a corresponding rustup target command for the publish workflow. |
Comments suppressed due to low confidence (3)
pyproject.toml:44
- Confirm that the removal of the 'i586-pc-windows-msvc' target is intentional and aligns with the list of valid Windows targets for the project.
windows.before-all = "rustup target add aarch64-pc-windows-msvc i686-pc-windows-msvc x86_64-pc-windows-msvc"
.github/workflows/python-package.yml:89
- Ensure that this added rustup target command is consistent with the targets defined in pyproject.toml and necessary for the CI build environment.
rustup target add aarch64-pc-windows-msvc i686-pc-windows-msvc x86_64-pc-windows-msvc
.github/workflows/package-publish.yml:64
- Verify that the inclusion of this rustup target command in the publish workflow is necessary and that it matches the configuration in pyproject.toml.
rustup target add aarch64-pc-windows-msvc i686-pc-windows-msvc x86_64-pc-windows-msvc
| python -m build --outdir wheelhouse | ||
| - name: Build wheels with cibuildwheel | ||
| run: | | ||
| python -m pip install --upgrade pip |
There was a problem hiding this comment.
[nitpick] Consider adding an inline comment explaining the switch to cibuildwheel and the rationale for locking to version 2.22.0 to aid future maintainers.
Suggested change
| python -m pip install --upgrade pip | |
| python -m pip install --upgrade pip | |
| # Locking cibuildwheel to version 2.22.0 for compatibility and stability reasons. | |
| # Future maintainers should test thoroughly before upgrading to a newer version. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Update of the pyproject.toml configuration for rustup for valid windows targets.