Cmake cleanup #10163
Cmake cleanup #10163
Conversation
CMakeLists.txt
Outdated
| # get build type | ||
| if(CMAKE_BUILD_TYPE MATCHES "Release") | ||
| message(STATUS "Release build seleceted.") | ||
| set(CMAKE_BUILD_TYPE "Release") |
There was a problem hiding this comment.
This doesn't make any sense. The var is already "Release".
There was a problem hiding this comment.
if you try and set an invalid build type as a build option, itll break the cmake build. this change means that if the build type is invalid it will default to release
There was a problem hiding this comment.
Need RelWithDebInfo type, and MinSizeRel for completeness. Otherwise they will default to Release too.
CMakeLists.txt
Outdated
| set(CMAKE_BUILD_TYPE "Release") | ||
| elseif(CMAKE_BUILD_TYPE MATCHES "Debug") | ||
| message(STATUS "Debug build seleceted. Debug builds can be unstable!!") | ||
| set(CMAKE_BUILD_TYPE "Debug") |
|
cc @hcorion |
|
This is really weird. |
|
some of the cmake files used tab indents, others used space indents. either is fine, but you shouldn't really swap between them (same with case type). in cmake, it's most common is to use spaces instead of tabs. |
There was a problem hiding this comment.
Other than the mentioned issue I think it looks good.
In the future when combining formatting and code changes, if you could do all formatting (in this case tabs->spaces) in one, and then code changes in commits after that, it would be much easier to review. For example in your simplified msvc compile flags in rpcs3/Emu commit it's really hard to tell what is formatting changes and what is actually changing functionality. In addition it looks like you switch from spaces->tabs->spaces in the different commits which makes it even trickier to review.
CMakeLists.txt
Outdated
| if(NOT CMAKE_BUILD_TYPE) | ||
| message(STATUS "No build type selected, default to Release") | ||
| set(CMAKE_BUILD_TYPE "Release") | ||
| # get build type |
There was a problem hiding this comment.
Yeah this chunk of code is unnecessary clutter.
You don't need to warn the user that debug builds are unstable, if they've decided to compile RPCS3 with debug options we can assume they know what they're doing. In addition, you don't handle RelWithDebInfo.
Best to just leave it as it was.
There was a problem hiding this comment.
removed it from the commit, but i do still think its messy and can lead to undefined behavior. ill revisit it again some other time :)
CMakeLists.txt
Outdated
| # get build type | ||
| if(CMAKE_BUILD_TYPE MATCHES "Release") | ||
| message(STATUS "Release build seleceted.") | ||
| set(CMAKE_BUILD_TYPE "Release") |
There was a problem hiding this comment.
Need RelWithDebInfo type, and MinSizeRel for completeness. Otherwise they will default to Release too.
this is generally why i keep to one commit per 'theme of changes' so to speak, instead of bundling them up. its far easier to make weird history errors |
p01arst0rm
force-pushed
the
cmake-cleanup
branch
from
28051a8
to
5ee69d5
Compare
|
@Nekotekina "Need RelWithDebInfo type, and MinSizeRel for completeness. Otherwise they will default to Release too." i've removed that commit from this push. It's refactoring, not cleanup and should really have its own commit. This PR is just for housekeeping type stuff :) |
|
Oookay, I see |
if an invalid build type is passed into cmake, the build
will instead default to Release.