widberg / bgfx.cmake

https://github.com/bkaradzic/bgfx.cmake. Independently maintained CMake build scripts for bgfx. Released under public domain.
https://github.com/bkaradzic/bgfx.cmake
Creative Commons Zero v1.0 Universal
286 stars 252 forks source link

Add set(CMAKE_DEBUG_POSTFIX "d") to root CMakeLists.txt #97

Closed pr0g closed 3 years ago

pr0g commented 3 years ago

There was a previous discussion a little while ago about adding this (see here https://github.com/widberg/bgfx.cmake/issues/75 and here https://github.com/widberg/bgfx.cmake/pull/77)

At the time, this conflicted with BGFX_USE_DEBUG_SUFFIX which has now been removed (https://github.com/widberg/bgfx.cmake/pull/81)

I propose adding:

if(NOT DEFINED CMAKE_DEBUG_POSTFIX)
    set(CMAKE_DEBUG_POSTFIX "d")
endif()

To the root CMakeLists.txt file to ensure a sensible default behaviour with the ability to override it from the command line if required.

I use this (awesome!) bgfx CMake build wrapper in another project https://github.com/pr0g/sdl-bgfx-imgui-starter with ExternalProject_Add - the library can be built in both Debug and Release and all built libraries are installed to the same install location (this makes the -DCMAKE_PREFIX_PATH setup simpler for find_package in the sample application. This requires having CMAKE_DEBUG_POSTFIX set and I'd rather not have to build that into the instructions if I can help it (which is why I think adding it as 'on' by default is a good idea).

If people strongly disagree, this PR can be closed but I'd find it incredibly helpful and it would mean I'd no longer need to maintain my fork either 🙂.

Thanks!

Tom

(Note: There's also a couple of other new line character adds at the end of files in the PR but they're just editor hygiene changes with no other impact)

bwrsandman commented 3 years ago

sensible default behaviour

This would differ from general and default cmake behaviour. I don't see what benefit it brings to change this for every user. It sounds to me that appending 'd' is your personal preference.

pr0g commented 3 years ago

I guess to me the more important question is when would you ever not want this behaviour? It is so incredibly useful.

I personally don't see a problem with being opinionated about certain options like this just because it isn't the CMake default. I honestly can't think of a good reason why it isn't the default so don't see why it shouldn't be used and recommended.

Is there a specific scenario where this would break someone or significantly impact them?

The other good reason in my mind to add it is if you build bgfx normally using make following the regular build instructions (which I did recently), libs and exes are distinguished by configuration in the name (e.g. shadercDebug.exe). So it feels this behaviour is closer to what the library expects itself rather than that of CMake.

hartcw commented 3 years ago

I originally added the "BGFX_USE_DEBUG_SUFFIX" because [at the time] the "d" suffix was always being added, and I needed a way to disable it.

Now that we use the CMAKE_DEBUG_POSTFIX option instead, my preference is still to avoid having different names for the debug/release versions of the lib (instead I use different output directories for debug/release libs).

With this patch I would now need to specify -DCMAKE_DEBUG_POSTFIX="" when running cmake. This makes it different to all the other 3rdparty libs that I build with cmake.

pr0g commented 3 years ago

Fair enough... I can from my calling code pass CMAKE_DEBUG_POSTFIX="d", I just haven't come across a good reason why you wouldn't want to do this. I am genuinely curious what this breaks? I just want to understand the rationale of why and not just have it be a principled thing.

I again think appending Debug might actually be the most 'correct' thing to do in some sense as that's what bgfx does when building it using GENie/make

hartcw commented 3 years ago

In my case, I'm building about 10 3rdparty libs with cmake, and then linking them into an application using qmake. It's much easier if library names are the same for release/debug versions; then you just need to point qmake to different release/debug directory to find the libs, intead of individually specifying different library names in the link list.

Plus, I find it safer if debug/release builds are in separate directories, because there is less risk that you ship some debug libs in your final package.

Just to be clear, I can achieve this by explicitly using -DCMAKE_DEBUG_POSTFIX="", so I don't have a strong objection if others want "d" to be the default.

pr0g commented 3 years ago

Thank you for the explanation, I do see exactly what you're saying.

In my use cases, I wind up not running into this issue as I find my 3rd party dependencies with find_package and never explicitly refer to the library name directly (e.g. I just use the imported target in target_link_libraries, bgfx::bgfx - please see this repo's CMakeLists.txt for an example).

If you were specifying the .lib file directly I can see how the naming could cause problems.

It's cool I'll close this and just update my ExternalProject_Add command to pass -DCMAKE_DEBUG_POSTFIX="d" instead. I'm just happy to know why it's important to not add it.

Thanks!

Tom