GNOME Bugzilla – Bug 794350
meson: fix ladspa dependencies
Last modified: 2018-03-27 13:55:33 UTC
I want to use meson, since it enables me to build and install only the few plugins I need. However, in reality to build the libgstladspa.so target I must first build libxml2, raptor2 and lrdf. This is not consistent with the autotools implementation which treats lrdf as an optional dependency. The bottom line is, requiring lrdf defeats the purpose of saving time if you really don't need lrdf. There is also an issue with not checking for ladspa.h which could cause an unexpected build failure if the ladspa sdk is not installed.
Created attachment 369723 [details] [review] Patch that fixes meson dependency issues with ladspa
Comment on attachment 369723 [details] [review] Patch that fixes meson dependency issues with ladspa Thanks for the patch. Please amend the patch author with your actual name, thanks! (git commit --amend --author='Name <foo@bar.com>')
Created attachment 369725 [details] [review] Patch file with fixed author name You are so fast! I was just about to fix this :)
Looks good to me and we should probably get this in before the release. Tim?
I think it can wait until .1, sorry :)
commit e61e840d099b6624cde54993780378564649a23d (HEAD -> master) Author: Patrik Nilsson <asavartzeth@gmail.com> Date: Mon Mar 5 17:43:26 2018 +0100 meson: fix ladspa dependencies There are two issues, both related to dependency checking with the meson support for the ladspa plugin. With autotools, lrdf is handled like an optional dependency. But with meson it is required. This makes the meson support less flexible and inconsistent with autotools. When autotools is used it properly checks if ladspa.h is available. But with meson it does not, instead it treats lrdf as the main dependency. This could cause a build failure if lrdf is installed, but the ladspa sdk is not. https://bugzilla.gnome.org/show_bug.cgi?id=794350
Review of attachment 369725 [details] [review]: ::: ext/ladspa/meson.build @@ +20,3 @@ link_args : noseh_link_args, include_directories : [configinc, libsinc], + dependencies : [gstaudio_dep, gstbase_dep, gmodule_dep, mathlib], You forgot lrdf_dep here, which now causes: /usr/include/lrdf.h:8:10: erreur fatale: raptor.h : No such file or directory
Created attachment 370172 [details] [review] Fix optional lrdf_dep dependency You're right. Although re-adding lrdf_dep to the dependencies list would defeat the point of making it optional. The patch has been fixed and tested thoroughly against 1.14.0, with and without lrdf.
Review of attachment 370172 [details] [review]: You mention in the commit message that `ladspa.h` is not checked in the Meson build, but your commit doesn't add that? Shouldn't there also be a cc.has_header() check for it? See: http://mesonbuild.com/Reference-manual.html#compiler-object
Another thing you can do is: lrdf_dep = dependency('lrdf', required : false) if not lrdf_dep.found() lrdf_dep = [] else ... endif Meson flattens dependency lists automatically.
(In reply to Nirbheek Chauhan from comment #10) > Another thing you can do is: > > lrdf_dep = dependency('lrdf', required : false) > if not lrdf_dep.found() > lrdf_dep = [] > else > ... > endif > > Meson flattens dependency lists automatically. Another thing is that you don't need to do that even. Not-found not-required dependencies are just ignored by Meson, so you don't need to assign [] to it.
Created attachment 370177 [details] [review] meson: Add missing optional lrdf dep to ladspa build My comments were wrong, I am blind. This change is enough to fix things.
Attachment 370177 [details] pushed as 5d2674f - meson: Add missing optional lrdf dep to ladspa build
Review of attachment 370172 [details] [review]: I thought meson was smart, you if a optional dep was not found, you didn't have to do this if dance ?
I've just tested locally with what I pushed to fix the breakage. It does not break if lrdf is not found. the lrdf_dep is always added to the list, but it seems to resolve to nothing, hence having no side effect, no warning either. What is problem do you have in git master that I don't see ?
Ah, sorry, I see, I didn't push that patch, was still hanging in my local branch. Same as Nirkeek patch.