Skip to content

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

Merged
texodus merged 1 commit intoperspective-dev:masterfrom
Point72:tkp/msvc
Mar 26, 2024
Merged

Fix compatibility with other MSVC versions by explicitly converting to string#2570
texodus merged 1 commit intoperspective-dev:masterfrom
Point72:tkp/msvc

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

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
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 perspective-dev:master Mar 26, 2024
GabrielCoderz added a commit to versatil-ti/perspective-new that referenced this pull request Apr 29, 2024
@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

Labels

bug Concrete, reproducible bugs

Development

Successfully merging this pull request may close these issues.

2 participants