breakfastquay / rubberband

Official mirror of Rubber Band Library, an audio time-stretching and pitch-shifting library.
http://breakfastquay.com/rubberband/
GNU General Public License v2.0
566 stars 91 forks source link

static and dynamic libraries both built and installed #51

Closed Be-ing closed 2 years ago

Be-ing commented 2 years ago

This is a hassle for packaging and downstream users. Please use Meson's standard mechanism for controlling whether the library is built statically or dynamically. Per Meson documentation:

It is generally preferred to use the library command instead of shared_library and static_library and then configure which libraries (static or shared or both of them) will be built at the build configuration time using the default_library built-in option.

cannam commented 2 years ago

I did look at this at the time I think, but the static library is used when building the other targets such as the command-line tool and plugins, so we have to build it regardless of whether the dynamic one is requested or not.

Of course it's possible I've missed a better way to do this.

My goals for build scripts in general are

It sounds as if my first goal there (which is not really negotiable in my mind) may be in conflict with what you're asking for, but it could be helpful if you can explain why you think the current system is a hassle - I certainly don't want the system to be actively annoying!

eli-schwartz commented 2 years ago

In another project of mine, I've built the library like this:

libfoo_a = static_library('foo_objlib', all_src, install: false)
libfoo = library('foo', link_with: libfoo_a, install: true)

All sources are built exactly once. Any targets that must be linked statically get a dependent link_with: libfoo_a. Any targets that don't care how they are linked, but wish to follow the default user configured link model, will link_with: libfoo instead.

If the user configures with -Ddefault_library=static then libfoo_objlib.a and libfoo.a are both created with the same objects, but you only pay the cost of two ar commands, not the cost of recompiling.

The current meson.build is using a method compatible with older versions of meson (< 0.52, but this project requires 0.53) that involves extract_all_objects(recursive: true), which accomplishes something similar for building the shared library from the static library, while introducing a bit of weirdness due to a custom option -Dno_shared=<bool> -- most people building projects using meson are likely expecting -Ddefault_library to work out of the box.

  • The default build command should build (and then install) as many of the standard targets as possible in the current environment.

FWIW, you can override meson's own defaults by adding to project() the kwarg default_options: ['-Ddefault_library=both'].

This would not prevent users from manually setting it back to =shared if they only want the shared library.

Aside: if you pass your installable library to pkgconfig.generate() it will include your pkg-config dependencies and found libraries and suchlike automatically. It will also correctly define the -l flag, which currently seems like it could be intended as -lrubberband-static (but is instead hardcoded to -lrubberband) for unclear reasons.

cannam commented 2 years ago

This all looks very helpful, thanks!

Be-ing commented 2 years ago

@eli-schwartz that seems like a decent solution, thanks. The important point for me is using the build system's standard mechanism for controlling whether the library is built statically or dynamically, not creating an ad-hoc system idiosyncratic to this particular library.

cannam commented 2 years ago

I've had a go at this and pushed the results - let me know how this looks!

There were a couple of other complications, mostly from the MSVC build for which I have been trying to retain naming compatibility with prior build systems, but the main thing is that it respects the default_library option and the wacky extra option has gone. Seems to check out OK in my tests so far.

eli-schwartz commented 2 years ago

From a quick glance, that seems to make sense. I'm surprised that link_with was unhappy on MSVC but tbh I don't know anything about that toolchain anyway.

IIRC the rationale for meson using the libfoo.a name on Windows is because foo.lib is the name for both an import library for the shared dll, and the common name for the static lib, and when building both they would have a name clash. I guess you traditionally added "-static" to the name instead... I guess there is not much you can do there, other than your workaround in the name of compat with previous versions. At least now it is scoped to windows :D

What do you think about adding another declare_dependency for the default library, e.g. rubberband_dep = declare_dependency(...)?

This would allow people to add rubberband as a wrap dependency fallback: https://mesonbuild.com/Wrap-dependency-system-manual.html#provide-section

cannam commented 2 years ago

IIRC the rationale for meson using the libfoo.a name on Windows is because foo.lib is the name for both an import library for the shared dll, and the common name for the static lib, and when building both they would have a name clash.

I do kind of like this logic, and I would probably go along with it for a new library.

What do you think about adding another declare_dependency for the default library, e.g. rubberband_dep = declare_dependency(...)?

This would allow people to add rubberband as a wrap dependency fallback: https://mesonbuild.com/Wrap-dependency-system-manual.html#provide-section

I've added what I think you mean (i.e. a two-liner after the definition of rubberband_library). Do take a look. I haven't actually looked into Wrap at all myself yet.

Thanks for your help here.

eli-schwartz commented 2 years ago

Almost. It should also set include_directories: 'rubberband' so that the superproject both:

when using rubberband_dep as a build dependency.

cannam commented 2 years ago

OK, I've added that too - though I am flying blind here as I don't know how to test it, but the idea seems benign enough! Let me know if I've bungled this.

eli-schwartz commented 2 years ago

I made a dumb mistake and suggested the wrong thing. Includes are supposed to be done as

#include <rubberband/RubberBandStretcher.h>

So, the include_directories object should be '.', not 'rubberband'.

Now I am actually at my computer and can test it. :)

You can test it by making an example meson project:

project('rubberband-test', 'cpp')

rubberband_dep = dependency('rubberband')

executable('rubberband-example', 'main.cpp', dependencies: [rubberband_dep])

And then creating this file:

$ cat subprojects/rubberband.wrap 
[wrap-hg]
url = https://hg.sr.ht/~breakfastquay/rubberband
revision = tip

[provide]
rubberband = rubberband_dep

Then configure meson using:

$ meson setup builddir --wrap-mode=forcefallback
[...]
Looking for a fallback subproject for the dependency rubberband because:
Use of fallback dependencies is forced.
requesting all changes
adding changesets
adding manifests
adding file changes
added 494 changesets with 1801 changes to 238 files (+3 heads)
new changesets 9996a45cda46:fa0323bacc9e
updating to branch default
129 files updated, 0 files merged, 0 files removed, 0 files unresolved

Executing subproject rubberband 

rubberband| Project name: Rubber Band Library
rubberband| Project version: 2.0.0
[...]

And verify that everything builds correctly (make sure rubberband isn't installed globally to the system, or /usr/include may leak).

Again, it will not build correctly, because I gave a bad suggestion:

adds subprojects/rubberband-2.0.0/rubberband/ as an -I directory

I lied, you don't want this, the files to include are not subprojects/rubberband-2.0.0/rubberband/rubberband/RubberBandStretcher.h (too many directories there)

So, the following fix makes it actually work:

diff -r fa0323bacc9e meson.build
--- a/meson.build   Wed Jan 05 12:55:01 2022 +0000
+++ b/meson.build   Wed Jan 05 19:56:59 2022 -0500
@@ -488,7 +488,7 @@
 #
 rubberband_dep = declare_dependency(
   link_with: rubberband_library,
-  include_directories: 'rubberband',
+  include_directories: '.',
 )

 if get_option('default_library') != 'shared' and rubberband_additional_static_lib != ''
cannam commented 2 years ago

I probably should have spotted that myself, even without the test run - sorry! The further explanation you've given is great though, really helpful.

When trying it myself, I was also led to discover that meson.source_root(), which I used in a couple of places to pick up the LADSPA and Vamp plugin linker scripts, is a bad idea because it gives the parent project's root instead of the subproject's. So I have fixed that as well as making the change you suggest.

Perhaps I should break out some of the more esoteric build info in the Rubber Band README into a separate file and add this information about subprojects to it, with a link to the Wrap docs. If nothing else it would be a useful reminder for me.

eli-schwartz commented 2 years ago

Great, happy to help.

Once this is tagged in a release I can add rubberband to the Meson WrapDB, and then people who depend on rubberband can download that wrap file using meson wrap install rubberband.

(It will download the tarball rather than mercurial tip, hence waiting for the tagged release.)

Be-ing commented 2 years ago

I will make a PR to finally get Rubberband in upstream vcpkg when a new release is tagged.

cannam commented 2 years ago

I will make a PR to finally get Rubberband in upstream vcpkg when a new release is tagged.

I had never previously tried vcpkg, but I just followed the getting-started instructions in this post and pulled down a couple of libraries to take a look. I was curious whether it would build static or dynamic libraries, since from the context here you did not want the Rubber Band build system to default to both. It went for static - is this the norm for vcpkg across platforms or is this a configurable thing?

I am curious how vcpkg handles libraries with differing or incompatible licence requirements. The examples in their intro are all BSD-ish, but I installed an LGPL library (libsndfile) and a GPL one (wildmidi) and the vcpkg command gave me no feedback that indicated there was any licence to consider. Since it does dependency tracking (so may install many libraries from a single command), is intended for vendoring in development (hence pre-built static libraries etc without any developer involvement), and comes from a company that typically supports proprietary software, I can easily imagine a user assuming everything there was redistributable in a proprietary application, which seems problematic. Does it have a standard or conventional way to query licence status?

Be-ing commented 2 years ago

Whether vcpkg builds static or dynamic libraries is controlled by the target triplet.

If a vcpkg user violates the license of a library they are using, that's their responsibility, but I agree that vcpkg should make it easier to identify the licenses of the whole dependency graph. There is a way for vcpkg manifests to specify an SPDX license identifier but it is not widely used. There is a pending PR to check for that on vcpkg's CI.

cannam commented 2 years ago

v2.0.1 is out now with this fix in it!

Be-ing commented 2 years ago

I made a PR to add it to vcpkg.

Be-ing commented 2 years ago

This bug is fixed now, closing.

Be-ing commented 2 years ago

There is a way for vcpkg manifests to specify an SPDX license identifier but it is not widely used. There is a pending PR to check for that on vcpkg's CI.

https://github.com/microsoft/vcpkg/pull/20790 was merged recently, so this situation of making the license info more easily accessible in vcpkg should improve gradually.

eli-schwartz commented 2 years ago

Once this is tagged in a release I can add rubberband to the Meson WrapDB, and then people who depend on rubberband can download that wrap file using meson wrap install rubberband.

I forgot to do this, originally, but it is now done: https://github.com/mesonbuild/wrapdb/commit/c029beee6d7696956df42bb6c2b1fcf3eb9b8f74