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

Fix CMake on Windows with Visual Studio #945

Merged
merged 1 commit into from Aug 16, 2023

Conversation

here-abarany
Copy link
Contributor

Avoid setting CMAKE_BUILD_TYPE default when multi-config generators are used.

Avoid setting CMAKE_BUILD_TYPE STRINGS property if it is not a cache variable. This avoids CMake errors if CMAKE_BUILD_TYPE isn't set on the command-line.

Closes #932

Avoid setting CMAKE_BUILD_TYPE default when multi-config generators are
used.

Avoid setting CMAKE_BUILD_TYPE STRINGS property if it is not a cache
variable. This avoids CMake errors if CMAKE_BUILD_TYPE isn't set on the
command-line.

Closes libgeos#932
@pramsey
Copy link
Member

pramsey commented Aug 10, 2023

Well, this builds/test OK on my local env (MacOX 13.5, cmake version 3.24.4).

Copy link
Member

@hobu hobu left a comment

Choose a reason for hiding this comment

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

LGTM. Setting a default CMAKE_BUILD_TYPE in your config still isn't worth it to me, but that's the policy that GEOS has taken going forward.

@here-abarany
Copy link
Contributor Author

Are there any other reviews needed before merging in this PR? I don't have permissions to merge, so someone else will need to press the button.

@pramsey pramsey merged commit 87bf94a into libgeos:3.12 Aug 16, 2023
27 checks passed
@pramsey
Copy link
Member

pramsey commented Aug 16, 2023

Was hoping @dbaston would weigh in, but time and tide...

@dbaston
Copy link
Member

dbaston commented Aug 16, 2023

Not much experience building with MSVC, sorry. If it works for the author and Howard, it should be fine.

@here-abarany here-abarany deleted the cmake-fix branch August 16, 2023 18:42
@here-abarany
Copy link
Contributor Author

@pramsey I should note that this was merged into the 3.12 branch, so it should be cherry-picked onto main at some point. Not sure if that's something you want to handle or if you want me to submit another PR.

mwtoews pushed a commit that referenced this pull request Aug 17, 2023
Avoid setting CMAKE_BUILD_TYPE default when multi-config generators are
used.

Avoid setting CMAKE_BUILD_TYPE STRINGS property if it is not a cache
variable. This avoids CMake errors if CMAKE_BUILD_TYPE isn't set on the
command-line.

Closes #932
@mwtoews
Copy link
Contributor

mwtoews commented Aug 17, 2023

Cherry-picked to main with 40da3a6

@robe2 robe2 added this to the 3.12.1 milestone Nov 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants