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 `CMAKE_DEBUG_POSTFIX` to distinguish debug versions of built libraries #77

Closed pr0g closed 4 years ago

pr0g commented 4 years ago

In reference to: https://github.com/widberg/bgfx.cmake/issues/75#issue-615213441

This will ensure when building in config Debug, all built libraries will have d appended as a postfix. This is incredibly useful when installing libraries as it means both Debug and Release versions of the library can be installed to the same location.

bwrsandman commented 4 years ago

This seems non-standard... Will this require projects using as a fetch content to manually specify 'd' suffix?

pr0g commented 4 years ago

I do not believe so, FetchContent just uses the exported target name, so you don't have to worry about the actual .lib files (SDL uses this approach and I've just tested this locally with bgfx and it works like a charm :)).

I could go through every target and use set_target_properties but in this instance I'm not sure it's worth it.

I could test out FetchContent if you like, or feel free to try it out with my fork.

bwrsandman commented 4 years ago

For example, if I were to make a Linux package on the AUR and a user decides to install in debug for development purposes, the generated .so would have a different name. This would be inconvenient as installed apps depending on this package would have no library and newly built packages will be looking for a a suffix.

Maybe add this as a default-off option?

bwrsandman commented 4 years ago

I just tested as a fetch content and there is no issue as far as I can see.

pr0g commented 4 years ago

But as I understand it the CMakeLists.txt for the project would look the same:

find_package(bgfx REQUIRED)

add_executable(${PROJECT_NAME})

target_link_libraries(
    ${PROJECT_NAME} PRIVATE
    bgfx::bgfx)
pr0g commented 4 years ago

That's great to hear @bwrsandman! :D Thanks for testing!

bwrsandman commented 4 years ago

I just ran across this option which seems to exist already, maybe there's a way to reconcile both? https://github.com/widberg/bgfx.cmake/blob/master/CMakeLists.txt#L33

pr0g commented 4 years ago

Oh awesome good spot! Hmm that looks like it's on by default right? Maybe my change isn't needed at all? Or would I need to pass that option to CMake when running configure?

bwrsandman commented 4 years ago

Testing some more and it looks like the d suffix is being added twice if I set the library to SHARED on this PR

Linking CXX shared library libbgfxdd.so

$ ldd openblack | grep bgfx
    libbgfxdd.so => ~/code/openblack/cmake-build-debug/_deps/bgfx-build/libbgfxdd.so (0x00007f836963f000)

Even without setting it to shared, it's double applied on this PR

Linking CXX static library libbgfxdd.a

On upstream master it's applied by default on my system:

Linking CXX static library libbgfxd.a

My worry here is if I install bgfx as a shared library system wide without the d suffix and build openblack, it won't be able to use the system wide library due to the name difference.

pr0g commented 4 years ago

Testing some more and it looks like the d suffix is being added twice if I set the library to SHARED

Linking CXX shared library libbgfxdd.so

$ ldd openblack | grep bgfx
  libbgfxdd.so => ~/code/openblack/cmake-build-debug/_deps/bgfx-build/libbgfxdd.so (0x00007f836963f000)

Even without setting it to shared, it's double applied.

Linking CXX static library libbgfxdd.a

My worry here is if I install bgfx as a shared library system wide without the d suffix and build openblack, it won't be able to use the system wide library due to the name difference.

Hmm I think I can just delete my addition, maybe the postfix was already being applied and I just missed it 😬 (my bad). I'm just going to test again locally and can close this PR if I can confirm it's already working.

pr0g commented 4 years ago

Yes it seems bx, bgfx and bimg all already have that 'd' postfix thanks to the option you pointed out 🤦‍♂️ so my update is surplus to requirements 😛 Thanks for reviewing, I'll close the PR now.