Update CMake File #952

Merged
merged 15 commits into from Sep 18, 2016

Projects

None yet

3 participants

@Ghabry
Member
Ghabry commented Jul 25, 2016 edited

This synchronizes the CMake file with the features of the autoconf file and adds a shiny summary table :)

I also tried to simplify the code (400 lines cough) and removed some features I don't see a use for. Wine and Shinonome are gone, Wine test makes no sense and Shinonome can be manually triggered if you really need a recompile.

@carstene1ns carstene1ns commented on an outdated diff Jul 25, 2016
CMakeLists.txt
"${CMAKE_CURRENT_SOURCE_DIR}/builds/cmake/Modules")
+if(CMAKE_BUILD_TYPE MATCHES "Debug")
+ add_definitions(-D _DEBUG=1)
@carstene1ns
carstene1ns Jul 25, 2016 Member

style: remove space before definition

@carstene1ns carstene1ns commented on an outdated diff Jul 25, 2016
CMakeLists.txt
add_definitions(
${PNG_DEFINITIONS}
-D USE_SDL=1)
-
+
@carstene1ns
carstene1ns Jul 25, 2016 Member

style: space

@carstene1ns carstene1ns commented on an outdated diff Jul 25, 2016
CMakeLists.txt
-include_directories("${CMAKE_CURRENT_SOURCE_DIR}/src")
-
-foreach(i Expat Freetype Harfbuzz Pixman ZLIB PNG SDL2 Iconv)
- find_package(${i} REQUIRED)
-
- string(TOUPPER ${i} i)
+# Sound system to use
+set(PLAYER_AUDIO_BACKEND "SDL2_mixer" CACHE STRING "Audio system to use. The SDL2_mixer audio system provides advanced features required by RPG Maker. Options: SDL2_mixer OpenAL OFF")
+set_property(CACHE PLAYER_AUDIO_BACKEND PROPERTY STRINGS SDL2 OpenAL)
+
+# Configure Audio backends
+
+if(${PLAYER_AUDIO_BACKEND} MATCHES "SDL2_mixer")
+ # SDL2_mixer Audio
+ find_lib_req(SDL2_mixer ON)
+ add_definitions(-D HAVE_SDL_MIXER=1)
@carstene1ns
carstene1ns Jul 25, 2016 Member

style: remove space before definition

@carstene1ns carstene1ns commented on an outdated diff Jul 25, 2016
CMakeLists.txt
+ endif()
+
+ # wildmidi (optional, OFF)
+ option(PLAYER_WITH_WILDMIDI "Play MIDI audio with wildmidi. (optional)" OFF)
+ find_lib("wildmidi" PLAYER_WITH_WILDMIDI)
+ if(${RETVAL})
+ add_definitions(-DHAVE_WILDMIDI)
+ endif()
+
+ # Provide fmmidi options
+ set(PLAYER_ENABLE_FMMIDI "Fallback" CACHE STRING "Enable internal MIDI sequencer. Fallback (default) mode will use it when the external MIDI library fails. Options: Fallback ON OFF")
+ set_property(CACHE PLAYER_ENABLE_FMMIDI PROPERTY STRINGS Fallback ON OFF)
+ if(${PLAYER_ENABLE_FMMIDI} MATCHES "Fallback")
+ add_definitions(-DWANT_FMMIDI=2)
+ elseif(${PLAYER_ENABLE_FMMIDI} MATCHES "ON")
+ add_definitions(-DWAND_FMMIDI=1)
@carstene1ns carstene1ns commented on an outdated diff Jul 25, 2016
CMakeLists.txt
+ # mpg123
+ option(PLAYER_WITH_MPG123 "Play MP3 audio with libmpg123." ON)
+ find_lib("mpg123" PLAYER_WITH_MPG123)
+ if(${RETVAL})
+ add_definitions(-DHAVE_MPG123)
+ endif()
+
+ # libsndfile
+ option(PLAYER_WITH_LIBSNDFILE "Play WAV audio with libsndfile." ON)
+ find_lib("SndFile" PLAYER_WITH_LIBSNDFILE)
+ if(${RETVAL})
+ add_definitions(-DHAVE_LIBSNDFILE)
+ endif()
+
+ # libogg
+ option(PLAYER_WITH_LIBOGG "Play OGG audio with libogg or tremor." ON)
@carstene1ns
carstene1ns Jul 25, 2016 Member

This does only care for Ogg/Vorbis for now, so removing libtremor makes sense IMHO

@carstene1ns carstene1ns and 1 other commented on an outdated diff Jul 25, 2016
CMakeLists.txt
+ option(PLAYER_WITH_LIBSNDFILE "Play WAV audio with libsndfile." ON)
+ find_lib("SndFile" PLAYER_WITH_LIBSNDFILE)
+ if(${RETVAL})
+ add_definitions(-DHAVE_LIBSNDFILE)
+ endif()
+
+ # libogg
+ option(PLAYER_WITH_LIBOGG "Play OGG audio with libogg or tremor." ON)
+ find_lib("OggVorbis" PLAYER_WITH_SPEEXDSP)
+ if(${RETVAL})
+ add_definitions(-DHAVE_OGGVORBIS)
+ endif()
+
+ # wildmidi (optional, OFF)
+ option(PLAYER_WITH_WILDMIDI "Play MIDI audio with wildmidi. (optional)" OFF)
+ find_lib("wildmidi" PLAYER_WITH_WILDMIDI)
@carstene1ns
carstene1ns Jul 25, 2016 Member

this will likely not work, as the library name is mixed case (tested?)

@Ghabry
Ghabry Jul 25, 2016 Member

This works because find_library uses that argument to open "Findwildmidi.cmake" but I can rename both to be consistent with the lib name (WildMidi)

@carstene1ns carstene1ns commented on an outdated diff Jul 25, 2016
CMakeLists.txt
+ elseif(${PLAYER_ENABLE_FMMIDI} MATCHES "ON")
+ add_definitions(-DWAND_FMMIDI=1)
+ elseif(${PLAYER_ENABLE_FMMIDI} MATCHES "OFF")
+ # noop
+ else()
+ message(FATAL_ERROR "Bad FmMidi setting")
+ endif()
+elseif(${PLAYER_AUDIO_BACKEND} MATCHES "OpenAL")
+ # OpenAL Audio
+ add_definitions(-DNO_SDL_MIXER)
+ find_package(OpenAL REQUIRED)
+ find_package(SndFile REQUIRED)
+ find_package(FluidSynth REQUIRED)
+ include_directories(${OPENAL_INCLUDE_DIR} ${SNDFILE_INCLUDE_DIR} ${FLUIDSYNTH_INCLUDE_DIR})
+ list(APPEND EASYRPG_PLAYER_LIBRARIES ${OPENAL_LIBRARY} ${SNDFILE_LIBRARIES} ${FLUIDSYNTH_LIBRARY})
+ add_definitions(-D HAVE_OPENAL=1)
@carstene1ns
carstene1ns Jul 25, 2016 Member

style: remove space before definition

@carstene1ns carstene1ns commented on an outdated diff Jul 25, 2016
builds/cmake/Modules/FindOggVorbis.cmake
@@ -0,0 +1,10 @@
+find_path(OGGVORBIS_INCLUDE_DIR ogg/ogg.h)
+find_library(OGGVORBIS_LIBRARY NAMES ogg libogg)
@carstene1ns
carstene1ns Jul 25, 2016 Member

the required lib is vorbisfile (which provides the same high-level api as tremor), so this will not work correctly

@carstene1ns carstene1ns commented on an outdated diff Jul 25, 2016
CMakeLists.txt
endif()
# CPack
set(CPACK_GENERATOR "ZIP" "TGZ")
if(${CMAKE_SYSTEM_NAME} MATCHES "Windows")
- list(APPEND CPACK_GENERATOR "NSIS")
+ list(APPEND CPACK_GENERATOR "NSIS")
@carstene1ns
carstene1ns Jul 25, 2016 Member

generating a windows installer for the portable player was always a bit off IMO, so maybe we should remove that option now

@carstene1ns carstene1ns commented on an outdated diff Jul 25, 2016
CMakeLists.txt
+ # noop
+ else()
+ message(FATAL_ERROR "Bad FmMidi setting")
+ endif()
+elseif(${PLAYER_AUDIO_BACKEND} MATCHES "OpenAL")
+ # OpenAL Audio
+ add_definitions(-DNO_SDL_MIXER)
+ find_package(OpenAL REQUIRED)
+ find_package(SndFile REQUIRED)
+ find_package(FluidSynth REQUIRED)
+ include_directories(${OPENAL_INCLUDE_DIR} ${SNDFILE_INCLUDE_DIR} ${FLUIDSYNTH_INCLUDE_DIR})
+ list(APPEND EASYRPG_PLAYER_LIBRARIES ${OPENAL_LIBRARY} ${SNDFILE_LIBRARIES} ${FLUIDSYNTH_LIBRARY})
+ add_definitions(-D HAVE_OPENAL=1)
+elseif(${PLAYER_AUDIO_BACKEND} MATCHES "OFF")
+ add_definitions(-DNO_SDL_MIXER)
+elseif()
@carstene1ns
carstene1ns Jul 25, 2016 Member

style: should this not be else() ?

@Ghabry
Member
Ghabry commented Aug 6, 2016 edited

@carstene1ns
This is again open for review. But before merging I think I will squash some commits together or you think it's fine?

@fdelapena fdelapena commented on the diff Aug 8, 2016
tests/filefinder.cpp
void CheckIsDirectory() {
assert(FileFinder::IsDirectory("."));
}
void CheckEnglishFilename() {
- assert(!FileFinder::FindImage("Backdrop", "castle").empty());
@fdelapena
fdelapena Aug 8, 2016 edited Member

The old test was doing a "touch" command for a RTP castle filename written in Japanese, maybe TestGame-2000 should have some filename in Japanese or vice-versa (use chara1 in Japanese in this string), to make this function name still meaningful. Or just rename the function 😜.

@Ghabry
Ghabry Aug 10, 2016 Member

Well it searches for a file that has a English name :D. But I could change it to a japanese -> english lookup

@carstene1ns carstene1ns was assigned by Ghabry Aug 12, 2016
@carstene1ns
Member

Unrelated minor visual issue: the sdl2_mixer version stuff does not work, these definitions need to be changed as the header still uses the old version macros (without 2).

@carstene1ns carstene1ns commented on an outdated diff Aug 19, 2016
CMakeLists.txt
if(CMAKE_GENERATOR MATCHES "Makefile")
- add_definitions(-fno-rtti)
+ add_definitions(-fno-rtti)
@carstene1ns
carstene1ns Aug 19, 2016 edited Member

This should actually be added to CXXFLAGS.

set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fno-rtti")
@carstene1ns carstene1ns commented on an outdated diff Aug 19, 2016
CMakeLists.txt
endif()
-if(CMAKE_BUILD_TYPE MATCHES "Debug")
- add_definitions(-D _DEBUG=1)
+# Detect all required libraries
+foreach(i Pixman ZLIB PNG SDL2)
+ find_lib_req(${i} ON)
+endforeach()
+
+# Always enable Wine registry support on non-Windows
+if(CMAKE_SYSTEM_NAME MATCHES "Windows")
@carstene1ns
carstene1ns Aug 19, 2016 edited Member

Why not if(NOT <...>)?

@carstene1ns
Member

IMO the "line" status messages are not needed and look bad as the summary is separated by empty lines anyway and the length of them is arbitrary, but this may be personal preference.

@carstene1ns carstene1ns and 1 other commented on an outdated diff Aug 19, 2016
CMakeLists.txt
+function(find_lib_req LIBNAME COND)
+ find_lib_int(${LIBNAME} ${COND} ON)
+
+ # Export _FOUND variable to parent scope which is global scope
+ set(${LIBNAME}_FOUND ${${LIBNAME}_FOUND} PARENT_SCOPE)
+ set(EASYRPG_PLAYER_LIBRARIES ${EASYRPG_PLAYER_LIBRARIES} ${NEW_LIBS} PARENT_SCOPE)
+ set(RETVAL ${RETVAL} PARENT_SCOPE)
+endfunction()
+
+function(find_lib LIBNAME COND)
+ find_lib_int(${LIBNAME} ${COND} OFF)
+
+ # Export _FOUND variable to parent scope which is global scope
+ set(${LIBNAME}_FOUND ${${LIBNAME}_FOUND} PARENT_SCOPE)
+ set(EASYRPG_PLAYER_LIBRARIES ${EASYRPG_PLAYER_LIBRARIES} ${NEW_LIBS} PARENT_SCOPE)
+ set(RETVAL ${RETVAL} PARENT_SCOPE)
@carstene1ns
carstene1ns Aug 19, 2016 Member

Why is RETVAL needed? Seems like just for convenience, because the status could be queried by using ${LIBNAME}_FOUND at higher level too.

@Ghabry
Ghabry Aug 19, 2016 Member

Yes is just my pseudo-return variable because looks like that you can't really return something from Cmake functions...

@carstene1ns carstene1ns commented on an outdated diff Aug 19, 2016
CMakeLists.txt
-
- add_test(
- NAME test_${name} WORKING_DIRECTORY ${TEST_GAME_PATH}
- COMMAND ${EXECUTABLE_OUTPUT_PATH}/test_${name})
- set_tests_properties(test_${name}
- PROPERTIES ENVIRONMENT "${TEST_ENVS}")
-endforeach()
+option(PLAYER_ENABLE_TESTS "Execute unit tests after compilation finishes" ON)
+
+if(PLAYER_ENABLE_TESTS)
+ enable_testing()
+
+ set(TEST_GAME_REPOSITORY_PATH "${CMAKE_CURRENT_SOURCE_DIR}/lib/TestGame")
+ if(NOT EXISTS ${TEST_GAME_REPOSITORY_PATH})
+ execute_process(COMMAND ${GIT_EXECUTABLE} clone
+ "https://github.com/EasyRPG/TestGame.git"
@carstene1ns
carstene1ns Aug 19, 2016 Member

btw. we could just use --depth=1 here too as the history is not needed either.

@carstene1ns carstene1ns commented on an outdated diff Aug 19, 2016
CMakeLists.txt
endif()
-# mpg123
-find_package(libmpg123)
-if (libmpg123_FOUND)
- include_directories(${MPG123_INCLUDE_DIR})
- list(APPEND EASYRPG_PLAYER_LIBRARIES ${MPG123_LIBRARY})
- add_definitions(-DHAVE_MPG123)
+# freetype and harfbuzz
+option(PLAYER_WITH_FREETYPE "Support FreeType font rendering" ON)
+find_lib("Freetype" PLAYER_WITH_FREETYPE)
+if(${RETVAL})
+ add_definitions(-DHAVE_FREETYPE)
+ option(PLAYER_WITH_HARFBUZZ "Enable HarfBuzz text shaping (Requires FreeType)" ON)
@carstene1ns
carstene1ns Aug 19, 2016 Member

CMAKE_DEPENDENT_OPTION could be used instead for harfbuzz.

@carstene1ns carstene1ns commented on an outdated diff Aug 19, 2016
CMakeLists.txt
list(APPEND CMAKE_MODULE_PATH
- "${CMAKE_CURRENT_SOURCE_DIR}/lib/liblcf/builds/cmake/Modules"
"${CMAKE_CURRENT_SOURCE_DIR}/builds/cmake/Modules")
@carstene1ns
carstene1ns Aug 19, 2016 Member

Some boilerplate I would like to propose to be added here:

# Set a default build type if none was specified
if(NOT CMAKE_BUILD_TYPE)
    message(STATUS "Setting build type to 'Debug' as none was specified.")
    set(CMAKE_BUILD_TYPE Debug CACHE STRING "Choose the type of build." FORCE)
    set_property(CACHE CMAKE_BUILD_TYPE PROPERTY STRINGS "Debug" "Release" "MinSizeRel" "RelWithDebInfo")
endif()

This makes sure a build type is defined, so different flags can be used.
Cmake will for example use -g for "Debug" builds and optimize -O3 for "Release" build by default.

@Ghabry
Member
Ghabry commented Sep 11, 2016

I hope this was the last change now :D

@Ghabry
Member
Ghabry commented Sep 11, 2016 edited

No it wasnt. Found a problem in combination with PLAYER_BUILD_LIBLCF which only happens without auxing... trying to fix this tomorrow >.>

@carstene1ns carstene1ns and 1 other commented on an outdated diff Sep 11, 2016
CMakeLists.txt
+
+ # Export _FOUND variable to parent scope which is global scope
+ set(${LIBNAME}_FOUND ${${LIBNAME}_FOUND} PARENT_SCOPE)
+ set(EASYRPG_PLAYER_LIBRARIES ${EASYRPG_PLAYER_LIBRARIES} ${NEW_LIBS} PARENT_SCOPE)
+endfunction()
+
+# liblcf
+option(PLAYER_BUILD_LIBLCF "Instead of detecting liblcf the liblcf repository is cloned into lib/liblcf and built together with the Player. This is convenient for development" OFF)
+set(PLAYER_BUILD_LIBLCF_GIT "https://github.com/EasyRPG/liblcf.git" CACHE STRING "Git repository of liblcf to clone when building liblcf. Requires PLAYER_BUILD_LIBLCF=ON.")
+
+if(PLAYER_BUILD_LIBLCF)
+ # liblcf is built as part of this cmake file
+ set(LIBLCF_PATH "${CMAKE_CURRENT_SOURCE_DIR}/lib/liblcf")
+ find_package(Git REQUIRED)
+ if(NOT EXISTS ${LIBLCF_PATH})
+ execute_process(COMMAND ${GIT_EXECUTABLE} clone
@carstene1ns
carstene1ns Sep 11, 2016 Member

reason why this does not use --depth=1 anymore?

@Ghabry
Ghabry Sep 11, 2016 Member

Because you can't really work with a unshallow clone when you want to edit code but I figured out in the meanwhile that there is an unshallow command for me usecase :D

CMakeLists.txt
+
+ if(${REQ})
+ find_package(${LIBNAME} REQUIRED)
+ else()
@Ghabry
Ghabry Sep 15, 2016 Member

Just testing the new review feature.

CMakeLists.txt
+
+ string(TOUPPER ${LIBNAME} LIBNAMEUP)
+ include_directories(${${LIBNAMEUP}_INCLUDE_DIR} ${${LIBNAMEUP}_INCLUDE_DIRS})
+ set(NEW_LIBS ${${LIBNAMEUP}_LIBRARY} ${${LIBNAMEUP}_LIBRARIES} PARENT_SCOPE)
@Ghabry
Ghabry Sep 15, 2016 Member

This stuff should only be set when FOUND 👎

+set(FLUIDSYNTH_INCLUDE_DIRS ${FLUIDSYNTH_INCLUDE_DIR})
+set(FLUIDSYNTH_LIBRARIES ${FLUIDSYNTH_LIBRARY})
+
+mark_as_advanced(FLUIDSYNTH_INCLUDE_DIR FLUIDSYNTH_LIBRARY)
@carstene1ns
carstene1ns Sep 18, 2016 Member

This is needed for XMP, too 💃.

@Ghabry Ghabry CMake: Fix build error when an optional lib was not found. Fix proble…
…ms in CMake Module files.
93c0955
@carstene1ns carstene1ns merged commit 4930c4f into EasyRPG:master Sep 18, 2016

6 checks passed

Android (armeabi-v7a) Build finished.
Details
GNU/Linux Build finished.
Details
OSX Build finished.
Details
Windows (x64) Build finished.
Details
Windows (x86) Build finished.
Details
web Build finished.
Details
@Ghabry Ghabry deleted the Ghabry:cmake branch Jan 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment