Implement CMake build script

[jcelerier]

Hello, here's a build script to allow using RubberBand with CMake.

It's not finished yet, but if you want to review it it should not change much.

[cannam]

Hello there - thanks for proposing this! I'm not a CMake user; can you give me a quick summary of how to test this?

[jcelerier]

Hi !

mkdir build && cd build
cmake path/to/rubberband 
make -jsomething

should do it. Add -DCMAKE_BUILD_TYPE=Debug or -DCMAKE_BUILD_TYPE=Release to get usual builds configurations. To install outside of the default (e.g. /usr/local/ on Linux / Mac) it's -DCMAKE_INSTALL_PREFIX=/install/root

It's possible to create .vcxproj (default on windows) with cmake -G "Visual Studio 16 2019" ... or XCode projects with cmake -G Xcode. Other options are available here : https://cmake.org/cmake/help/v3.16/manual/cmake-generators.7.html

I have only ported the features that I needed but I can add what's missing if you intend to merge :)

[cannam]

Hi again - sorry about the delay. The default build and the basic options you mention appear to be working, thanks!

I have pulled your changes to a branch called "cmake", and in principle am happy to merge. Because Rubber Band Library is not maintained in a Git repo (this one is just a mirror), I've done so by pulling the commits from your branch here to the upstream Mercurial repo, rather than use the Github pull request logic. (The upstream repo is at https://hg.sr.ht/~breakfastquay/rubberband and the changes have been mirrored back here. I've no problem with pulling any further commits from your repo also, no need for you to engage with that repo)

I do have a few questions:

• In CMakeLists.txt there is a version definition set to 2.1.1. This number is the version number of the dynamic library, not the version number of the Rubber Band Library release (which is most recently 1.8.2). Does this matter? Does CMake do anything with this value?

• The optional build of the command-line program appears to work OK, and I'd suggest dropping the tentative wording about an "example application" and just referring to it as the "command-line program" with an executable name of rubberband rather than rubberband_test

• Would you be prepared to add a formal licensing note, in such a way that makes it possible to include these files in the official distribution, i.e. to redistribute under GPL and also include in commercially-licensed copies? An MIT-style licence would be suitable. I imagine you don't want a licence text in CMakeLists.txt itself, but adding a note to the bottom of the README like the ones about e.g. rubberband-sharp would be sufficient.

• I see that a sibling PR (#22) is proposing to add a CMakeLists.txt for Android Studio use. The content is quite different because of course it's intended for building the JNI interface. Is there any obvious way to unify the two, or a sensible way to include both (in different directories possibly) that would seem natural to a practitioner of CMake? (I've put a note in #22 asking the contributor to chime in if they have any views as well.)

Thanks!

[Be-ing]

There will be bugs specific to one build system or another if you try to maintain multiple build systems. I highly recommend to remove the other build systems after merging this. There is no point maintaining build systems that CMake can generate. @cannam would you be okay with that?

[Be-ing]

@Holzhaus I don't think it would be very hard to build the Windows DLL. I think you'd need to unconditionally build the main library statically when using MSVC and build the files in the rubberband-dll folder if BUILD_SHARED_LIBS=ON. The Visual Studio project file also defines a few preprocessor definitions.

CI should build both statically and dynamically, at least with MSVC.

[jcelerier]

looks good, thanks @Holzhaus - and yes.

@Be Regarding the preprocessor definition iirc I grepped the source code to see which ones were actually being used - for instance _USRDLL is some random MFC thing which shouldn't be relevant at all to rubberband

[jcelerier]

sorry @cannam I had completely forgot to answer..

In CMakeLists.txt there is a version definition set to 2.1.1. This number is the version number of the dynamic library, not the version number of the Rubber Band Library release (which is most recently 1.8.2). Does this matter? Does CMake do anything with this value?

oops, okay, so the CMake version should be the project version, and then we should set the library version (SOVERSION property) so that on linux we get librubberband.so.2.1.1

dropping the tentative wording about an "example application" and just referring to it as the "command-line program"

ok will do

Would you be prepared to add a formal licensing note, in such a way that makes it possible to include these files in the official distribution, i.e. to redistribute under GPL and also include in commercially-licensed copies? An MIT-style licence would be suitable. I imagine you don't want a licence text in CMakeLists.txt itself, but adding a note to the bottom of the README like the ones about e.g. rubberband-sharp would be sufficient.

sure, no worries. May I encourage you into looking at CLA Assistant which is made to automate this process ? (https://cla-assistant.io/) otherwise I'll add some text in the CMakeLists.txt stating my contributions as being under MIT

Regarding the CMake JNI PR, the best way would be to merge both - from what I can see it's mainly about adding another rubberband-jni target on android (or even if one wants to have desktop Java bindings) which will add the JNI specific source files.

I don't particularly have time today to look at all that but doing my best to finish that PR quickly :-)

[jcelerier]

I granted you access to my repo if you want to push directly on it. Don't particularly care about the genexpr, with time I tend to prefer them as it's purely declarative (and they're sometimes the only way to achieve particular behaviours especially when working with various debug / release configs with e.g. vs generators), but feel free to refactor.

Why does this specify USE_KISSFFT on MSVC in any case? We may be using FFTW3...

iirc I followed how the current build system worked on that, see e.g. https://github.com/breakfastquay/rubberband/blob/d05806450a1598a1574f9e53b2db353c1ab2da3b/rubberband-library.vcxproj#L78

[Holzhaus]

Added some fixes and improvements. It now builds properly in my Windows VM. I packaged this for vcpkg.

[jcelerier]

re. the last commit, I think that it would be better to use rebase & push --force instead, makes for a much cleaner history

[cannam]

Re. KissFFT, my current view is that all builds should probably use it by default unless there is a system platform option (e.g. vDSP). It's not that much slower than FFTW, it produces the same results, and it avoids a class of build/deploy problems, e.g. PIC errors when linking against the default static build of FFTW. Users wanting FFTW probably know that they do and can select it as an option. I am thinking of changing this in the configure script as well, in the next release.

The situation with resamplers is thornier because the external option (libsamplerate) does actually produce "better" output. At the moment we default to the bundled option (speex) anyway in various places, but in most real-world cases users would be better off with libsamplerate.

Re. the possibility of dropping other build systems, I'll have to revisit that - it's certainly true that having all of autoconf, non-configure-based Makefiles, and Visual Studio projects is a bit messy. I don't actually use CMake anywhere else myself - I'm mostly using Meson now elsewhere, and I'm really just following this proposal because I know other people want to use CMake these days - but if the build script is straightforward enough and just produces Makefile-or-equivalents anyway, that might well be ok. I wouldn't anticipate any such change until after the next release though.

[Holzhaus]

Re. KissFFT, my current view is that all builds should probably use it by default unless there is a bundled platform option (e.g. vDSP). It's not that much slower than FFTW, it produces the same results, and it avoids a class of build/deploy problems, e.g. PIC errors when linking against the default static build of FFTW. Users wanting FFTW probably know that they do and can select it as an option. I am thinking of changing this in the configure script as well, in the next release.

Okay, but this cmakelists doesn't build the bundled kissfft yet. Therefore it leads to linkage errors.

The situation with resamplers is thornier because the external option (libsamplerate) does actually produce "better" output. At the moment we default to the bundled option (speex) anyway in various places, but in most real-world cases users would be better off with libsamplerate.

We can just add a configure option, e. g. LIBSAMPLERATE=OFF.

Re. the possibility of dropping other build systems, I'll have to revisit that - it's certainly true that having all of autoconf, non-configure-based Makefiles, and Visual Studio projects is a bit messy. I don't actually use CMake anywhere else myself - I'm mostly using Meson now elsewhere, and I'm really just following this proposal because I know other people want to use CMake these days - but if the build script is straightforward enough and just produces Makefile-or-equivalents anyway, that might well be ok. I wouldn't anticipate any such change until after the next release though.

CMake is actually not a build system, it's a build system generator. This means that it actually generates a Makefile on Unix and a vcxproj on Windows (or a ninja build, if you use -G Ninja).

FFTW is currently in the same situation, because they support both automake and CMake. That is very unfortunate, not just for testing but also for downstream CMake projects. Let me elaborate a bit:

CMake has something called "package configs". This allows downstream cmake projects to just invoke

find_package(rubberband REQUIRED)
target_link_libraries(myprogram PRIVATE rubberband::rubberband)

And CMake takes care of everything else. It's basically something similar to pkg-config that also works on Windows.

But since FFTW also supports automake, which doesn't support generating these configs and a lot of distributions still ship the automake build, downstream projects still need to take care of dependency detection themselves, because even though FFTW is be installed, the CMake package config is missing.

Therefore I can strongly advise against maintaining automake/vcxproj/CMake in parallel.

[cannam]

CMake is actually not a build system

I do get that - hence my reference to producing Makefiles etc.

It's basically something similar to pkg-config that also works on Windows. But since FFTW also supports automake, which doesn't support generating these configs and a lot of distributions still ship the automake build, downstream projects still need to take care of dependency detection themselves

This I don't quite get, I'm afraid. If the distinction between a CMake package config and a pkg-config file is that the former also works on Windows, then why does it matter which one (presumably Linux) distributions install? Can CMake on Linux not use pkg-config? Or are you using "distributions" to mean something that includes Windows in some way?

FFTW certainly builds and installs pkg-config files if you build it with autoconf - there can be other problems with the build, but I wasn't aware that discovery was one.

[Be-ing]

I'm afraid. If the distinction between a CMake package config and a pkg-config file is that the former also works on Windows, then why does it matter which one (presumably Linux) distributions install?

If Linux distributions rely on pkg-config but that isn't available on Windows, then CMake projects which use Rubberband need to check for both ways. If Linux distributions reliably install the CMake package configs, downstream projects only need to check for those.

[Holzhaus]

Okay, I reworked the handling of dependencies. Please have a look @cannam.

[Holzhaus]

I'm afraid. If the distinction between a CMake package config and a pkg-config file is that the former also works on Windows, then why does it matter which one (presumably Linux) distributions install?

If Linux distributions rely on pkg-config but that isn't available on Windows, then CMake projects which use Rubberband need to check for both ways. If Linux distributions reliably install the CMake package configs, downstream projects only need to check for those.

Yes. find_package(foo CONFIG REQUIRED) will throw an error if there is no CMake config for the package foo. And since that isn't always the case (because some linux distros still build with automake), a downstream CMake project cannot depend on that and needs to implement custom logic to find depedencies instead.

[cannam]

Ah, that's the missing bit I hadn't realised - that you have to provide separate detection logic depending on whether you're trying to detect a pkg-config or CMake config package. That seems unfortunate, but if it's just the way things are, then that does make a difference.

[Holzhaus]

I need some pointers regarding the DLL build. It looks to me like rubberband-dll.cpp is a plain C wrapper around rubberband that exports different symbols than those defined in RubberBandStretcher.h. Is that how it's supposed to work?

Because linking a C++ program on windows fails if the rubberband is a DLL due to undefined symbols: https://github.com/Holzhaus/mixxx/runs/1742546796?check_suite_focus=true#step:17:1237

Is it actually possible to link against a dynamic build of the rubberband library without putting ifdefs for windows in the consumer code?

[cannam]

rubberband-dll.cpp exposes its own naming convention specifically for use with the .NET FFI - it is the .NET analogue of RubberBandStretcherJNI.cpp found in src/jni. I don't think it has any application other than the .NET code in rubberband-sharp.

If a standard build of Rubber Band, that happens to have been compiled to a DLL, fails to link, then my first guess would be simply that the symbols are not exported (default symbol visibility on Windows being hidden). This is not a supported configuration at all with the existing Rubber Band build files, which are static library only on Windows.

[jcelerier]

On windows it's possible to set https://cmake.org/cmake/help/latest/prop_tgt/WINDOWS_EXPORT_ALL_SYMBOLS.html as a band-aid for that. Proper fix is to mark the API as dllexport with GenerateExportHeader but this changes the source code

[Be-ing]

I agree that WINDOWS_EXPORT_ALL_SYMBOLS would be a good idea.

I suggest moving the rubberband-dll folder into the rubberband-sharp folder to avoid confusion.

[Holzhaus]

I already did that in my own fork, but wasn't sure if that is actually the right thing to do here. @cannam so everything in the rubberband-dll is only needed for the rubberband .NET bindings, not regular builds?

[cannam]

so everything in the rubberband-dll is only needed for the rubberband .NET bindings, not regular builds?

That's right.

[Holzhaus]

Ok, I removed everything related to the rubberband-dll directory and set the WINDOWS_EXPORT_ALL_SYMBOLS property instead. (FYI, I'm currently working on prebuilt dependencies to make building @mixxxdj/mixxx easier on Windows, that why I noticed the linker errors).

[cannam]

-mmacosx-version-min should default to the oldest version that the code is compatible with, I think, since the more common failure mode is to accidentally build it for too recent a target compared with the rest of the application it is to be linked against. That is currently 10.7.

(I don't see any performance difference between versions compiled with -mmacosx-version-min=10.7 and -mmacosx-version-min=11)

[Be-ing]

We could set the CMAKE_OSX_DEPLOYMENT_TARGET to the minimum by default if the user hasn't set it, but I don't think we should override it if the user has manually specified it.

I mention optimization because I ran into an issue with Mixxx where I had to disable an optimization to get it to build with an older macOS deployment target: https://github.com/mixxxdj/mixxx/pull/3305/commits/e7cf360d04ce2ecb0b41d4774f5ae52f8d076d70

[Be-ing]

@cannam how about making one last release without CMake then merge this and remove the other build systems?

[Be-ing]

@Holzhaus can you add a GH Actions workflow for CMake?

[Holzhaus]

@Holzhaus can you add a GH Actions workflow for CMake?

Is this even desired? Apparently this is just a mirror and the actual development happens on hg, not git.

[Be-ing]

There is already a GH Actions workflow in this repo. It doesn't hurt to have one in a repo on another service anyway.

[Be-ing]

The CMake config file is broken: https://github.com/jcelerier/rubberband/pull/3

[Be-ing]

The Windows DLL build is working: https://github.com/mixxxdj/mixxx/pull/3594 vcpkg port is here: https://github.com/mixxxdj/vcpkg/tree/2.3/overlay/ports/rubberband I will submit this upstream to vcpkg when this PR is merged and released.

[Be-ing]

There will be bugs specific to one build system or another if you try to maintain multiple build systems. I highly recommend to remove the other build systems after merging this. There is no point maintaining build systems that CMake can generate. @cannam would you be okay with that?

Case in point: https://github.com/libsndfile/libsamplerate/issues/140

[Be-ing]

Some minor improvements, then I think this is ready to merge: https://github.com/jcelerier/rubberband/pull/4

[Be-ing]

I got CI working with GitHub Actions on Linux, macOS, and Windows in https://github.com/jcelerier/rubberband/pull/4

[Be-ing]

@cannam I think this is ready to merge.

[cannam]

I've pulled these changes to the cmake branch upstream, but I am increasingly unconvinced about the wisdom of taking this patch.

I was happy (and in principle still am) to include a CMake build script simply because there is apparently demand for it. But as I've mentioned, I've no immediate use for it myself, it isn't the build system I would pick if I were starting now, and increasingly the arguments proposed in its favour are suggesting that it isn't a great idea unless it's the primary build system and it might also be more difficult to switch away in the future. Even as I can see the logic for some consolidation, I'm not all that happy with that proposition.

I've set up an alternative branch with a Meson build (the branch is called meson) which I'm going to trial as well - it is also a little incomplete, but I'd appreciate views on that also. Meson has some of the same problems as CMake, but I do find it reasonably transparent.

[Holzhaus]

Personally I don't care if you pick cmake or meson, as long as it's a cross platform build system that is reasonably easy to integrate into vcpkg. Building software on windows is extremely painful, and vcpkg at least makes it a bit easier.

Does meson support symbol exporting like cmake does? Otherwise you'll need to put these ugly declspec(dllexport) stuff everywhere.

[cannam]

Building software on windows is extremely painful

I can agree with that

Does meson support symbol exporting like cmake does? Otherwise you'll need to put these ugly declspec(dllexport) stuff everywhere.

I don't think it does have a built-in mechanism for this. At the moment my Meson script builds only a static library on Windows. I take it that, for integration with vcpkg or whatever, shared libraries are essential? (My own limited experience has been that static libraries are so far preferable on Windows that there wasn't much reason to care about this)

I'll have a look at what the options are - I certainly don't intend to be adding export declarations so this may be significant.

[Holzhaus]

I don't think it does have a built-in mechanism for this.

You're right: https://github.com/mesonbuild/meson/issues/2132

I take it that, for integration with vcpkg or whatever, shared libraries are essential?

For vcpkg it's possible set a lib to "static only". Unfortunately, it's kind of inconvenient to mix static and dynamic libs and we'd like to use dynamic libs for our dependencies.

The alternative is to use Def files. Someone suggested to take the Def file generated by cmake, strip out the unneeded symbols and use that for meson dll builds. That won't for you to clutter your source code with this Microsoft-specific stuff.

[jcelerier]

I'll have a look at what the options are - I certainly don't intend to be adding export declarations so this may be significant.

that wouldn't be too bad though... if I'm not mistaken just adding one for

 class RubberBandStretcher

would be enough, no ? and would likely reduce the size of the .dll. (I personnally only use the static library so I don't really care though. Even if it has a very important implication if using fftw on windows - it means that the host app does not need to call fftw_enable_threads() to make things thread safe unlike in the static library case, as if I'm not mistaken the FFTW statics won't go "out" of the rubberband dll if fftw is linked statically to it)

[cannam]

would be enough, no ?

Huh:

https://docs.microsoft.com/en-us/cpp/cpp/using-dllimport-and-dllexport-in-cpp-classes

You learn something new every day. Thanks!

I'd have to export the rubberband-c.h symbols as well, but they're a compatibility interface already so I don't really mind that.

[Be-ing]

Unfortunately, it's kind of inconvenient to mix static and dynamic libs and we'd like to use dynamic libs for our dependencies.

IIRC an issue with static libraries on Windows with Mixxx was that linking required an absurd amount of RAM, though I'm not sure if that was because of a compiler flag we used or if that's just how it works with MSVC.

[Be-ing]

the arguments proposed in its favour are suggesting that it isn't a great idea unless it's the primary build system

It's not a great idea to support multiple build systems regardless of what they are. It would also be a bad idea to support Meson and autotools, so I don't understand what advantage Meson would bring. However, as long it is easy to build on Windows and Unix and supports static and dynamic linking on all platforms, Meson would be fine. But we've already done the work for you here with CMake and it is verified to be working on Linux, macOS, and Ubuntu with CI.

[cannam]

I'm trying to test the CMake build on Windows, specifically with regard to shared libraries. I've tried it with both msbuild and nmake back-ends, but in both cases it only seems to produce a static library target. Does the CMake script in this PR have support for building a DLL and if so, how do I enable it?

I was specifically interested in what it did about library naming in the case where both static and dynamic libraries are built - on Windows this is problematic because of the import library associated with a DLL, which is usually given a .lib extension just as a naive static library build would. There are various ways to deal with this and I was curious which one CMake would use.

[Holzhaus]

'm trying to test the CMake build on Windows, specifically with regard to shared libraries. I've tried it with both msbuild and nmake back-ends, but in both cases it only seems to produce a static library target. Does the CMake script in this PR have support for building a DLL and if so, how do I enable it?

Pass -DBUILD_SHARED_LIBS=ON to cmake during configuration. https://cmake.org/cmake/help/latest/variable/BUILD_SHARED_LIBS.html

[cannam]

'm trying to test the CMake build on Windows, specifically with regard to shared libraries. I've tried it with both msbuild and nmake back-ends, but in both cases it only seems to produce a static library target. Does the CMake script in this PR have support for building a DLL and if so, how do I enable it?

Pass -DBUILD_SHARED_LIBS=ON to cmake during configuration. https://cmake.org/cmake/help/latest/variable/BUILD_SHARED_LIBS.html

Thanks. That builds only the shared libraries - is it possible to tell it to build both?

(Edit: oh, or does it? I was assuming rubberband.dll and rubberband.lib were the shared library and import library respectively, when built with that option - but I realise I'm assuming the conclusion there, since I don't know how it manages import library naming)

[Holzhaus]

'm trying to test the CMake build on Windows, specifically with regard to shared libraries. I've tried it with both msbuild and nmake back-ends, but in both cases it only seems to produce a static library target. Does the CMake script in this PR have support for building a DLL and if so, how do I enable it?

Pass -DBUILD_SHARED_LIBS=ON to cmake during configuration. https://cmake.org/cmake/help/latest/variable/BUILD_SHARED_LIBS.html

Thanks. That builds only the shared libraries - is it possible to tell it to build both?

(Edit: oh, or does it? I was assuming rubberband.dll and rubberband.lib were the shared library and import library respectively, when built with that option - but I realise I'm assuming the conclusion there, since I don't know how it manages import library naming)

Your assumption is correct.

You can build both by using 2 separate builds dirs, otherwise the lib files would conflict I guess.

[Be-ing]

I don't understand why anyone would want to build the same library both statically and dynamically simultaneously.

[cannam]

I don't understand what advantage Meson would bring

I think it's really a question of perspective. If it is just "another build system", added because some people asked for it, then I don't really mind what it's like, so long as it works ok. But if it's the primary or only one, then I will be using it as well, and so I am likely to have stronger views about what it consists of and to want it to have more in common with systems I'm using elsewhere.