Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement CMake build script #18

Closed
wants to merge 29 commits into from

Conversation

jcelerier
Copy link

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
Copy link
Member

cannam commented Dec 21, 2019

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
Copy link
Author

jcelerier commented Dec 21, 2019

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
Copy link
Member

cannam commented Jan 16, 2020

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 (add CMakeLists.txt for building in Android Studio #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 add CMakeLists.txt for building in Android Studio #22 asking the contributor to chime in if they have any views as well.)

Thanks!

Copy link

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this also work for building the DLL on Windows?

CMakeLists.txt Outdated Show resolved Hide resolved
@Be-ing
Copy link

Be-ing commented Jan 21, 2021

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
Copy link

Be-ing commented Jan 21, 2021

@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.

Co-authored-by: Jan Holthuis <[email protected]>
@jcelerier
Copy link
Author

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
Copy link
Author

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 :-)

Copy link

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm working on some fixes.

CMakeLists.txt Outdated
Comment on lines 42 to 45
target_compile_options(rubberband
PRIVATE
$<$<BOOL:${APPLE}>:-mmacosx-version-min=10.11>
$<$<AND:$<NOT:$<CXX_COMPILER_ID:MSVC>>,$<CONFIG:Release>>:-ffast-math ${MARCH_NATIVE_FLAG} -O3 -ftree-vectorize>
Copy link

@Holzhaus Holzhaus Jan 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally find these generator expressiong very hard to read. This would be much more straightforward IMHO:

Suggested change
target_compile_options(rubberband
PRIVATE
$<$<BOOL:${APPLE}>:-mmacosx-version-min=10.11>
$<$<AND:$<NOT:$<CXX_COMPILER_ID:MSVC>>,$<CONFIG:Release>>:-ffast-math ${MARCH_NATIVE_FLAG} -O3 -ftree-vectorize>
if(APPLE)
target_compile_options(rubberband PRIVATE -mmacosx-version-min=10.11)
endif()
if(NOT MSVC)
target_compile_options(rubberband PRIVATE $<CONFIG:Release:-ffast-math ${MARCH_NATIVE_FLAG} -O3 -ftree-vectorize>)
endif()

I'll open a PR.

CMakeLists.txt Outdated
PRIVATE
$<$<BOOL:${UNIX}>:USE_PTHREADS>
$<$<BOOL:${APPLE}>:HAVE_VDSP>
$<$<BOOL:$<CXX_COMPILER_ID:MSVC>>:__MSVC__ WIN32 _LIB NOMINMAX _USE_MATH_DEFINES USE_KISSFFT>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Also, if I understand that weird Microsoft XML thingy correctly, WIN32 is only specified for 32 bit builds, not all windows builds.

ANd the same applies here, I think an if/else ladder would be much more readable than all these genexprs.

@jcelerier
Copy link
Author

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.

<PreprocessorDefinitions>__MSVC__;WIN32;_DEBUG;_LIB;NOMINMAX;_USE_MATH_DEFINES;USE_KISSFFT;USE_SPEEX;%(PreprocessorDefinitions)</PreprocessorDefinitions>

@Be-ing
Copy link

Be-ing commented Feb 3, 2021

@cannam I think this is ready to merge.

@cannam
Copy link
Member

cannam commented Feb 5, 2021

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
Copy link

Holzhaus commented Feb 5, 2021

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
Copy link
Member

cannam commented Feb 5, 2021

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
Copy link

Holzhaus commented Feb 5, 2021

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

You're right: mesonbuild/meson#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
Copy link
Author

jcelerier commented Feb 5, 2021

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
Copy link
Member

cannam commented Feb 5, 2021

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
Copy link

Be-ing commented Feb 6, 2021

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
Copy link

Be-ing commented Feb 6, 2021

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
Copy link
Member

cannam commented Feb 8, 2021

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
Copy link

Holzhaus commented Feb 8, 2021

'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
Copy link
Member

cannam commented Feb 8, 2021

'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
Copy link

Holzhaus commented Feb 8, 2021

'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
Copy link

Be-ing commented Feb 8, 2021

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

@cannam
Copy link
Member

cannam commented Feb 8, 2021

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.

@Be-ing
Copy link

Be-ing commented Feb 8, 2021

have more in common with systems I'm using elsewhere

This is the big reason for using CMake over Meson. The C/C++ ecosystem is generally converging on CMake. Qt6 now uses CMake. Microsoft Visual Studio includes CMake now. Microsoft's vcpkg is built with CMake too and using CMake makes it trivially easy to package libraries in vcpkg (which is the motivation for @Holzhaus and me working on this).

@Holzhaus
Copy link

Holzhaus commented Feb 8, 2021

Of course you can build both static and shared libs simultaneously when renaming one of them: https://stackoverflow.com/a/29824424

However, this is pretty bad from the user's standpoint IMHO:

  1. It breaks the standard BUILD_SHARED_LIBS option
  2. If you add both to the ALL target, both are built by default even though most users only need one of them
  3. Projects that use rubberband now need able to detect multiple different names or drop support for one of them.

@matthiasbeyer
Copy link

From a packagers perspective (packing on CentOS/Debian as my dayjob, and Nix by night) I can say: Don't use meson. It's the one build system we (dayjob) have the biggest problems with (except maybe autotools), because it pulls in python and with that a whole zoo of stuff.

Just my 2ct, tho.

Base automatically changed from master to default February 11, 2021 14:06
@eszlari
Copy link

eszlari commented Feb 20, 2021

One thing to consider: CMake's add_subdirectory() vs. Meson's subproject(). Normally, they each can only include other projects into their tree, that use the same buildsystem. But Meson has (some) support for CMake, which ironically would make CMake the better choice, because this way, other projects could add rubberband to their trees, whether they use Meson or CMake.

@cannam
Copy link
Member

cannam commented Mar 5, 2021

The next release will definitely use Meson. I'll keep an eye on feedback (including from commercial users) and may review later. I am actively working on Rubber Band at the moment, so (a) I am glad to have a build system I like working with, but also (b) there should be more releases to follow, giving options to review the situation.

I'm also going to retain some of the previous build files in a subdirectory (otherbuilds) in case anyone directly needs a Makefile or Visual Studio project file. These will be modified to build only the static library, so no versioning issues, and will also be tested in CI.

Thank you everyone for providing the impulse to review the build situation, and for all the work put into the proposed CMake scripts. If this decision turns out to be a bad one, then switching will be much easier with this work to start from.

@cannam cannam closed this Mar 5, 2021
@eszlari
Copy link

eszlari commented Mar 5, 2021

@cannam Any estimate when you will merge the meson branch into master?

I guess when rubberband is available through vcpkg, it will not make much of a difference for consumers on Windows, wether you use Meson or CMake. And vcpkg improved their Meson support too.

@Be-ing
Copy link

Be-ing commented Mar 6, 2021

Actually it will make a difference because Meson has nothing comparable to WINDOWS_EXPORT_ALL_SYMBOLS for dynamic linking on Windows. Dynamic linking on Windows could be implemented on Windows with Meson, but this requires some more work than simply setting the WINDOWS_EXPORT_ALL_SYMBOLS property on a CMake target.

@eszlari
Copy link

eszlari commented Mar 21, 2021

You could add a custom_target() in Meson that uses dumpbin to generate the .def file:

https://stackoverflow.com/questions/9946322/how-to-generate-an-import-library-lib-file-from-a-dll

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants