ZDoom / ZMusic

GZDoom's music system as a standalone library
https://forum.zdoom.org/index.php
60 stars 32 forks source link

Overhaul CMakeLists to conform with modern CMake #20

Closed Blzut3 closed 3 years ago

Blzut3 commented 3 years ago

Apologies for the unreadable diff, but this is one of those moments where the band aid just needs to be ripped off.

Blzut3 commented 3 years ago

CMake put up a bit of a fight on getting the static library build working, but I think I managed it. Had to turn all the deps into OBJECT libraries since we ultimately want libzmusic.a to contain everything, and more importantly we want to avoid leaking the intermediate targets to the end user. Not something CMake currently natively supports but there's a work around, so that was fun.

alexey-lysiuk commented 3 years ago

Merging of static libraries is a pretty cool thing. It has a minor issue with the lite version though. Several dependencies from the full version are pulled, and their object files are put into the lite library.

coelckers commented 3 years ago

I haven't fully checked this yet so I do not know if this is already possible - but if this is designed for being used as a submodule it should have a checkable option to link the parent project to either full or lite configuration of the static library.

Blzut3 commented 3 years ago

@alexey-lysiuk Should be fixed. Got confused since zmusiclite was still depending on the header files.

@coelckers Keeping in mind that there may still be a few adjustments needed in a future commit to get the integration perfect, there isn't exactly a need for an option. Usage would look something like this:

option(FORCE_INTERNAL_ZMUSIC "Use internal ZMusic" OFF)
find_package(ZMusic QUIET COMPONENTS Full)
if(ZMusic_FOUND AND NOT FORCE_INTERNAL_ZMUSIC)
    message(STATUS "Using system ZMusic")
elseif(EXISTS ZMusic/CMakeLists.txt)
    message(STATUS "Using internal ZMusic")
    set(BUILD_SHARED_LIBS OFF)
    add_subdirectory(ZMusic EXCLUDE_FROM_ALL)
    # Restore BUILD_SHARED_LIBS here if necessary
else()
    message(SEND_ERROR "Could not find system ZMusic and submodule not checked out.")
endif()

target_link_libraries(game-engine PRIVATE ZMusic::zmusic)

The key is the EXCLUDE_FROM_ALL on add_subdirectory which means that the install() rules won't be run and it will only build the targets that are depended upon. So while the other zmusic target may exist, it doesn't contribute to bloated build times or anything like that.

This would mean that the other zmusic target will appear in the list in Visual Studio or Xcode, but if one wants to get rid of visual clutter then compiling separately will get rid of even more. I think I might be able to use the FOLDER property to group all the ZMusic targets together in Visual Studio though. Ultimately I don't think cluttering the ZMusic CMakeLists with branches just to remove a few targets that don't get built is worthwhile.

For this PR though I want to focus on regressions on the separate compile model. The pieces are there for working as a subdirectory, but like I said might need a few tweaks.

alexey-lysiuk commented 3 years ago

@Blzut3 It works perfectly now.

ericonr commented 3 years ago

Hi! Can I close #10 now?

Blzut3 commented 3 years ago

@ericonr I would say so.

@coelckers I commented out the ability to use system GME per your request. I believe this PR is just waiting for you to verify that it doesn't regress your workflow. Keep in mind that I don't have write access to this repository (which is fine) so I can't merge this myself. I'm not able to move onto the next stage of this effort until this stage is cleared.

coelckers commented 3 years ago

This does not seem to fully work yet. I am getting linker errors with MSVC and Release config where it reports a lot of linker symbols missing.

Compiler setting is "multithreaded DLL" but we want to compile with statically linked CRT. Looks like this got mixed up somewhere.

alexey-lysiuk commented 3 years ago

VS 2019 Release x64 was built without an issue for me. Because of Visual Studio update I needed to clear CMake cache though.

Blzut3 commented 3 years ago

Yeah, make sure CMake is up to date and that you cleared your cache. If done you should see that /MD is no longer part of the CMAKE_*_C(XX)_FLAGS variables since CMake now has native support for setting the runtime.

coelckers commented 3 years ago

After clearing the cache it works as intended.