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

Fixing cmake configuration for multi-config generators #450

Open
wants to merge 3 commits into
base: devel
Choose a base branch
from

Conversation

i-sultan
Copy link

@i-sultan i-sultan commented Aug 9, 2020

The assumption made throughout ospray cmake files is that CMAKE_BUILD_TYPE will always contain the build config type. The problem is that in multi-config generators (such as vs and xcode), the build type is only selected during build time, and setting CMAKE_BUILD_TYPE should have no impact.

This gets problematic if the user selects Debug during build time through --config, while CMAKE_BUILD_TYPE is not set, so it takes the default value of Release, causing a mismatch between the actual build type and the one set through CMAKE_BUILD_TYPE. Another scenario is when a user switches from Debug to Release builds, then they need to switch the CMAKE_BUILD_TYPE as well to keep it consistent with the actual config type, which is not the usual process in multi-config.

The solution proposed here relies on the cmake generator-expression ($) to derive the config type for both single and multi configuration types. It also enables debugging for windows since it was disabled before, for no good reason that I am aware off. The final impact is that we remove the default setting for CMAKE_BUILD_TYPE since it should use whatever is default for the generator.

The only downside comes to single-config builds, which is that we had to remove some variables from the UI (the cmake_build_type through cycling CONFIGURATION_TYPES), and also remove OSPRAY_BUILD_RELEASE/DEBUG/RELWITHDEBINFO from the config files, since we cannot use configure_file to support these as they rely now on generator expressions which are evaluated post config.

This change has been tested with both single (make) and multi-config (vs) and worked fine in both cases.

If not taking this pull request, I suggest at least removing the win32 condition from ispc debug, and clarifying in the documentation that multi-config generators (such as vs) need to set the CMAKE_BUILD_TYPE to be consistent with the chosen config type, otherwise some inconsistencies will happen (including the case where CMAKE_BUILD_TYPE is not set at all).

@jeffamstutz
Copy link
Contributor

Just wanted to say thanks for this! We are busy with SIGGRAPH preparations right now, but this will get incorporated ASAP after that...we haven't forgotten about it. :)

@i-sultan
Copy link
Author

Sure thing, Jeff! Looking forward to the magic you guys will show us at Siggraph this year.

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.

2 participants