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

Update MSVC toolset version #1768

Merged
merged 4 commits into from
Jun 28, 2024
Merged

Update MSVC toolset version #1768

merged 4 commits into from
Jun 28, 2024

Conversation

varunagrawal
Copy link
Collaborator

Based on the Windows image from the Github Actions images repo, the minimum toolset version for MSVC should be 14.40.

This PR updates our CI to follow this.

@varunagrawal varunagrawal added ci Related to the Continuous Integration pipeline windows Related to Windows labels Jun 25, 2024
@varunagrawal varunagrawal requested a review from ProfFan June 25, 2024 22:04
@varunagrawal varunagrawal self-assigned this Jun 25, 2024
@varunagrawal varunagrawal requested review from gchenfc and dellaert June 25, 2024 22:05
@varunagrawal
Copy link
Collaborator Author

I found the issue. It has to do with numpy 2.0.0. The tests pass perfectly with numpy 1.26.4 (the latest v1 release).
The best way to fix this would be to update pybind11 to 2.13.1. Pybind11 2.12 added support for numpy v2 so that should allow us to be forward compatible.

attn: @dellaert @ProfFan

Copy link
Member

@gchenfc gchenfc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems reasonable. If it is relevant, it might be worth mentioning in the INSTALL.md:

- Set the `Toolset` to `msvc_x64_x64`. If you know what toolset you require, then skip this step.

but based on what I understand, this is just a CI thing and not an actual install requirement version change so no change is necessary.

Also, re: pybind upgrade, this also seems reasonable if you want to go for it. Will that be a separate PR or is it closely related enough to warrant combining in / replacing this PR?

@ProfFan
Copy link
Collaborator

ProfFan commented Jun 28, 2024

Pybind update should be done with the script

@varunagrawal
Copy link
Collaborator Author

Pybind update should be done with the script

Yes I'll use the script but it first needs to be updated in the wrap project. Can you please take a look at the PR I made there?

@varunagrawal
Copy link
Collaborator Author

Made a temporary fix by restricting the numpy version to be below v2.0.0.

@varunagrawal varunagrawal merged commit 2af2c96 into develop Jun 28, 2024
31 checks passed
@varunagrawal varunagrawal deleted the update-windows-ci branch June 28, 2024 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Related to the Continuous Integration pipeline windows Related to Windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants