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.

Sign up for GitHub

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

Jump to bottom

Fix compatibility with other MSVC versions by explicitly converting to string #2570

Merged
merged 1 commit into from
Mar 26, 2024
Merged

Fix compatibility with other MSVC versions by explicitly converting to string #2570

merged 1 commit into from
Mar 26, 2024

Conversation

timkpaine
Copy link
Member

This is a really minor bug, but causes some annoyance during our build process and is the reason we have to explicitly set PSP_GENERATOR in a few places.

@timkpaine timkpaine requested a review from texodus March 20, 2024 18:41
@timkpaine timkpaine added the enhancement Feature requests or improvements label Mar 20, 2024
@timkpaine timkpaine changed the title Explicitly convert msvc version to string Fix compatibility with other MSVC versions by explicitly converting to string Mar 20, 2024
@texodus
Copy link
Member

texodus commented Mar 25, 2024

What is the bug? What is PSP_GENERATOR? How do I know not to revert this in the future?

@timkpaine
Copy link
Member Author

timkpaine commented Mar 25, 2024 •
edited
Loading

PSP_GENERATOR is the user-configureable override (similar to e.g. PSP_DEBUG) for the cmake-generator used by the C++ build system. This doesn't matter too much on Mac and Linux, where you can likely use makefiles or ninja interchangeably, but on Windows you may want to override the default generator to be a certain version of Visual Studio for compatibility with python or other libraries. In the perspective codebase, we use this here and its also used in e.g. conda-forge or when building from source.

We have always hardcoded it in our CI, which is why we likely never saw the issue. It's also possible that we hardcoded it originally BECAUSE of this bug. Regardless, this fixes the Visual Studio detection logic so that by default we build against the same VS that your python built against (whereas before the bug caused it to always use the default because of the wrong key type)

@texodus texodus added bug Concrete, reproducible bugs and removed enhancement Feature requests or improvements labels Mar 25, 2024
texodus
texodus approved these changes Mar 25, 2024
View reviewed changes
Copy link
Member

@texodus texodus left a comment

Choose a reason for hiding this comment

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

Looks good!

@texodus texodus merged commit 055d5b3 into finos:master Mar 26, 2024
13 checks passed
GabrielCoderz added a commit to versatil-ti/perspective-new that referenced this pull request Apr 29, 2024
@GabrielCoderz
Revert "Merge pull request finos#2570 from Point72/tkp/msvc"
cd54b6f 
This reverts commit 055d5b3, reversing
changes made to 35abc1e.
@timkpaine timkpaine deleted the tkp/msvc branch July 16, 2024 03:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@texodus texodus texodus approved these changes

Assignees
No one assigned
Labels
bug Concrete, reproducible bugs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@timkpaine @texodus