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

Check triggers cmake error unnecessarily #4150

Closed
wants to merge 2 commits into from

Conversation

greg7mdp
Copy link
Contributor

@greg7mdp greg7mdp commented Apr 21, 2022

Incorrect error for cmake execution on windows when platform unspecified or x64

Context of Change

When using cmake-gui to generate solution files on windows, the default platform is stated to be x64, so it is natural to leave the field unfilled, but even among the proposed choices, Win64 is nowhere to be found, we see only x64.

So whether we leave the field blank, or select x64 in the dropdown, cmake generation fails because it checks for the presence of Win64.

This change makes the generation of solution files more user friendly on windows, by not causing an error unless we find Win32.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Tests (You added tests for code that already exists, or your new feature included in this PR)
  • Documentation Updates
  • Release

@greg7mdp
Copy link
Contributor Author

image

@legleux
Copy link
Collaborator

legleux commented Apr 21, 2022

For Visual Studio 2017 and prior, the default wasn't x64 so generator platform couldn't be left blank.
I vote we don't even bother with 2017, but only use 2019 and 2022.

@greg7mdp
Copy link
Contributor Author

I vote we don't even bother with 2017, but only use 2019 and 2022.

sounds good to me!

@greg7mdp greg7mdp assigned ximinez and unassigned ximinez Apr 22, 2022
@greg7mdp greg7mdp requested review from ximinez and legleux April 22, 2022 21:03
Copy link
Collaborator

@ximinez ximinez left a comment

Choose a reason for hiding this comment

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

Let's get this merged ASAP to minimize hassles as we move to newer versions.

@greg7mdp
Copy link
Contributor Author

For Visual Studio 2017 and prior, the default wasn't x64 so generator platform couldn't be left blank. I vote we don't even bother with 2017, but only use 2019 and 2022.

Thanks, can you approve the PR @legleux ?

@greg7mdp greg7mdp added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Apr 28, 2022
Comment on lines 75 to 78
if ("${CMAKE_GENERATOR}" MATCHES "Visual Studio" AND
NOT ("${CMAKE_GENERATOR}" MATCHES .*Win64.*))
("${CMAKE_GENERATOR_PLATFORM}" MATCHES .*Win32.*))
message (FATAL_ERROR
"Visual Studio 32-bit build is not supported. Use -G\"${CMAKE_GENERATOR} Win64\"")
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about this instead?

if (MSVC AND CMAKE_GENERATOR_PLATFORM STREQUAL "Win32")
  message (FATAL_ERROR
    "Visual Studio 32-bit build is not supported. Use -A x64")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, looks better. I'll do a sanity check and update it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thejohnfreeman I did the change, removing the -A x64 as it does not seem to work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants