Make hctbuild cmake gen consistent with asserts #5881
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
A slightly complex series of events causes invocations of hctbuild after the first to enable asserts in release builds. This sets the build type even when it isn't needed to avoid it. Because the build type was not being set when generating for visual studio in hctbuild, when generating the cmake files using CMakeLists, it would produce an alarming message saying it was defaulting to debug, but, in fact, since it didn't care for the purposes of sln file generation, that wasn't actually a problem, just confusing. It did set a variable to enable asserts, but didn't do this for release variant builds The real problem occurs if hctbuild is invoked again, at that point, instead of defaulting to Debug, it leaves the build type empty because of another variable set in the cache. Additionally, since the first invocation enabled asserts, that also comes from the cache and so code that strips NDEBUG from all build variants is invoked because the build type is, in fact, not debug, it's an empty string.
python3kgae reviewed Oct 17, 2023
python3kgae approved these changes Oct 17, 2023
bob80905 approved these changes Oct 17, 2023
damyanp approved these changes Oct 17, 2023
pow2clk pushed a commit to pow2clk/DirectXShaderCompiler that referenced this pull request Oct 18, 2023
The previous PR microsoft#5881 didn't do what it meant to and had a lot of debug code due to a mistaken commit. This reverts that commit and restores the setting of the build type flag
hekota reviewed Oct 18, 2023
| message(CMAKE_BUILD_TYPE0!!!!!="${CMAKE_BUILD_TYPE}") | ||
| message(CMAKE_CONFIGURATION_TYPES!!!!!="${CMAKE_CONFIGURATION_TYPES}") | ||
| message(CMAKE_CXX_FLAGS_RELEASE!!!!!="${CMAKE_CXX_FLAGS_RELEASE}") | ||
| set(CMAKE_CXX_STANDARD 17) |
There was a problem hiding this comment.
Did you mean to leave all these messages in?
pow2clk pushed a commit that referenced this pull request Oct 18, 2023
The previous PR #5881 didn't do what it meant to and had a lot of debug code due to a mistaken commit. This reverts that commit and restores the setting of the build type flag
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
A slightly complex series of events causes invocations of hctbuild after the first to enable asserts in release builds. This sets the build type even when it isn't needed to avoid it.
Because the build type was not being set when generating for visual studio in hctbuild, when generating the cmake files using CMakeLists, it would produce an alarming message saying it was defaulting to debug, but, in fact, since it didn't care for the purposes of sln file generation, that wasn't actually a problem, just confusing. It did set a variable to enable asserts, but didn't do this for release variant builds
The real problem occurs if hctbuild is invoked again, at that point, instead of defaulting to Debug, it leaves the build type empty because of another variable set in the cache.
Additionally, since the first invocation enabled asserts, that also comes from the cache and so code that strips NDEBUG from all build variants is invoked because the build type is, in fact, not debug, it's an empty string.