Cleanup CMake

[Flamefire]

The new project structure looks great and the CMake is also mostly fine. However may I suggest the following changes (maybe missing some) to make it cleaner:

• Put targets into subfolders -> if(OPTIONAL_FEATURE) add_subdirectory(optional_feature) endif()
• Use ONLY target_* directives (as far as possible)
• Remove CMake version checks (some [wrong] checks check for <=3.2 but this is already enforced)
• Remove add_definitions(-DNDEBUG) -> CMake does that where required
• OPENBSD_LOCALBASE partially ignored -> use it
• c++11 or c++98? You have differing settings in the CMakeLists. I suggest to remove it and use the compilers default which allows using this in either
• remove message on end -> Not "the CMake way". You have set those manually anyway or you have ccmake to easily check those. Better: Rename option to have common prefix, then ccmake/cmake-gui groups them nicely and you don't clash names with super projects
• configure a config header for things like version (

  libADLMIDI/include/adlmidi.h

  Line 3 in d89084d

  *

  ) or options (defines currently passed on command line)

[Wohlstand]

Put targets into subfolders

Yeah, the thing I must to do long time ago, but didn't yet...

Remove CMake version checks

Reason why version checks are added to allow support of older CMake builds, but, as required CMake version is higher than in condition, yeah, nuke it.

OPENBSD_LOCALBASE partially ignored -> use it

Yeah, that needs some more depther research, I did the only small test from off VirtualBox environment...

c++11 or c++98? You have differing settings in the CMakeLists. I suggest to remove it and use the compilers default which allows using this in either

Most of parts must be enforced with C++98 to be sure code will be built on older compilers (as example, older Android NDKs, OpenWatcom where I have done some experiments, ancient GCCs, and some other clunky compilers I didn't tested yet, but they will more work than not). There are two cases are using C++11:

• adlmidi2 is enforced to be C++11 app as it's the reincornation of original ADLMIDI player in it's original form where the C++11 was used widely.
• gen_adldata where C++11 is used for convenience. It's a middleware which is not needed for everything, mainly on developer machine to bootstrap banks cache and calculate sounding delays. For now it runs an emulator to analyze the output PCM sound for sounding delays, but @jpcima on his side works on the alternative solution that is able to calculate those aproximal delays by the formula from off the operators data. This will give the instant getting of sounding delay values, and, possibly, they are will no more needed to be stored in the bank(s) and can be calculated on the fly from any bank.

As the "Put targets into subfolders" task, the C++98/C++11 mixing between different targets will be no more needed.

remove message on end -> Not "the CMake way". You have set those manually anyway or you have ccmake to easily check those. Better: Rename option to have common prefix, then ccmake/cmake-gui groups them nicely and you don't clash names with super projects

The reason why I made the stats of options is ability to easier see the available stuff with no necessary to dig docs. But yeah, looks like the better option would be needed. Also, libSDL's CMake build does similiar printing of it's options are set.

configure a config header for things like version

The reason I keeping "ready-to-use" header with no any pre-configures is giving an ability to use it as-is with no any change requirement (Optionally, everyone can import code of library into the project in the form of sources set like was done in a case of GZDoom [but, maybe I'll change this, and will make those libraries to be built via CMake and be linked which will be much easier to maintain and update.. like the case of GME and ZLib]). But, maybe I'll put some another info that will tell about version the "unknown" when header was used out of library's build.

[Flamefire]

Most of parts must be enforced with C++98 to be sure code will be built on older compilers

Then this is a bug:

libADLMIDI/CMakeLists.txt

Line 118 in d89084d

set(CMAKE_CXX_STANDARD 11)

Also, libSDL's CMake build does similiar printing of it's options are set.

SDL is a autotools/configure thing. CMake is not really used there although it may work. (Had a conversation with them lately) In autotools there is no cache that you can easily check so this output is required. For CMake you can simply look in the file.

The reason I keeping "ready-to-use" header with no any pre-configures is giving an ability to use it as-is with no any change requirement

Good point, but even then: The cmake-input file look very easy (Example: https://github.com/Return-To-The-Roots/s25client/blob/9e0de7282866bc8cedf3a779151ae023ebc582c3/libs/rttrConfig/build_paths.h.cmake) so if one chooses to not use the build system "configuring" the header themselves is trivial and you have a list of options which you can turn on/off without digging into the CMake or sources. So the configure header is even better for that.

[Wohlstand]

Remove add_definitions(-DNDEBUG) -> CMake does that where required

As I remember, in some cases it doesn't appears properly, therefore I defining it manually to avoud possible ussue for a case of lack of it.

[Flamefire]

As I remember, in some cases it doesn't appears properly, therefore I defining it manually to avoud possible ussue for a case of lack of it.

AFAIK only when misusing something. E.g. when overwriting CMAKE_CXX_FLAGS_* without appending. I just grepped the CMake code and it gets added for all release builds (release, minsizerel, relwithdebinfo) Can you remember where it didn't? Again I suspect an error in the CMake (user) code or a very old CMake version

I may help if you need some

[Wohlstand]

After finishing this, I'll backport the full stuff into libOPNMIDI also...

[jpcima]

I reviewed some of the CMake changes made and observed this.

1. Linking to stdc++ under the condition NOT MSVC.
   Bad, as there exist different STL choices than GNU. Darwin prefers the libc++ of LLVM.
   In my experience, it's unnecessary to link explicitly a STL library, it's sufficient to detect or to set the linker language.

2. I'd make a similar remark about linking pthread. Use FindThreads and link that.
   Or let C++ STL link threads libs automatically when applicable.

[Flamefire]

@jpcima I highly support this. AFAIK my changes did not introduce those and I didn't want to change something for which there might be a reason. But yes linking to a STL lib should not be required anywhere and probably pthread is also not even required (what was the reason? SDL should include that itself if required)

[jpcima]

The platforms which need special support are DOS and Android, so it might relate to either of these. I am not able to tell you a lot more about these cases.

[Wohlstand]

Manual linking of libc++ / libstdc++ is required from off the pure C application that statically links libADLMIDI, otherwise you'll meet with lot of undefined referrence on STL calls.

[Flamefire]

I'm pretty sure CMake is able to handle this. For example for imported libraries you can set https://cmake.org/cmake/help/v3.0/prop_tgt/IMPORTED_LINK_INTERFACE_LANGUAGES.html to explicitly tell which linker (link language) to use. For CMake targets CMake already knows this so I'm sure it sets this correctly.

[jpcima]

The configuration of adlmidiplay is broken on my SDL library 2.0.9.

libADLMIDI/utils/midiplay/CMakeLists.txt

Lines 7 to 8 in 25c48a9

	find_package(SDL2 REQUIRED)
	string(STRIP ${SDL2_LIBRARIES} SDL2_LIBRARIES)

The variable SDL2_LIBRARIES is empty, and the error is: string sub-command STRIP requires two arguments. (on the line 2 the variable surely needs quoting but it's not the point)

On my setup, SDLTargets.cmake provides SDL2 libs in the modern cmake fashion, and the old SDL2_* variables are not defined.

add_library(SDL2::SDL2 SHARED IMPORTED)
add_library(SDL2::SDL2main STATIC IMPORTED)

I remember it's an annoying problem because SDL2 find-modules act different on more or less recent versions of the library. Like OPNMIDI does right now, I would use a manual search method for the library.

[Flamefire]

The variable SDL2_LIBRARIES is empty, and the error is: string sub-command STRIP requires two arguments. (on the line 2 the variable surely needs quoting but it's not the point)

After find_package(SDL2 REQUIRED) you can assume SDL2_LIBRARIES is set so no quotes are fine and are even a good way this error here is detected.

On my setup, SDLTargets.cmake provides SDL2 libs in the modern cmake fashion, and the old SDL2_* variables are not defined.

Could you post "your setup"? Yes the SDLTargets.cmake sets targets but that's not what the find_package uses first. It looks for sdl2-config.cmake which for e.g. the official MinGW build looks like this:

...
set(SDL2_LIBDIR "${exec_prefix}/lib")
set(SDL2_INCLUDE_DIRS "${prefix}/include/SDL2")
set(SDL2_LIBRARIES "-L${SDL2_LIBDIR}  -lmingw32 -lSDL2main -lSDL2 -mwindows")
string(STRIP "${SDL2_LIBRARIES}" SDL2_LIBRARIES)

So the variable is set an all should be fine.

Can you append your sdl2-config and SDLTargets.cmake files? From the above add_library one can't see how the actual libraries are set.

[jpcima]

You can have a look at these attached SDL2 files, as found in sdl2 2.0.9-1 of Arch-Linux.
It's from /usr/lib/cmake/SDL2. From examination of the build recipe, they are just a sed substitution away from original, almost unchanged.
cmake-SDL2.tar.gz

[Flamefire]

Wow, nice. Didn't know SDL2 actually installs the exported targets now and that it even builds with CMake.

Well simple solution then: Change

libADLMIDI/utils/midiplay/CMakeLists.txt

Lines 7 to 8 in 25c48a9

	find_package(SDL2 REQUIRED)
	string(STRIP ${SDL2_LIBRARIES} SDL2_LIBRARIES)

to:

if(TARGET SDL2::SDL2)
    target_link_libraries(adlmidiplay PRIVATE SDL2::SDL2)
else()
    string(STRIP ${SDL2_LIBRARIES} SDL2_LIBRARIES)
    target_include_directories(adlmidiplay PRIVATE ${SDL2_INCLUDE_DIRS})
    target_link_libraries(adlmidiplay PRIVATE ${SDL2_LIBRARIES})
endif()

[jpcima]

Seems a nice solution @Flamefire. I will try.