Created attachment 370698 [details]
Script that generate a meson_options.txt

I had this script for a while, it generates a meson_options.txt based on all options it finds by parsing m4 files.

Created attachment 370699 [details]
Generated options for gst-plugins-good

To get some context, here is the output of my script for gst-plugins-good.

I think we should use this opportunity to re-imagine the available options configuration system. We don't have to copy all the existing options or map them 1:1.

The reason I haven't added options for plugins yet is because I want this done properly with support in Meson for auto-options and skipping subdirs, and with a standardised global override for packagers (see below).

We will ignore the guidance in that gnome wiki page, it doesn't match our needs.

I'm just going to write down some initial thoughts/requirements which will hopefully help us decide what to propose on the Meson side.

I would suggest we focus on the plugin selection problematique first, the other bits are less interesting or can already be handled satisfactorily with the existing functionality in Meson (boolean options, disabler object etc.).

In terms of auto functionality, what I think we want is:

1) a tristate enum of some sort (enable/disable/auto) or equivalent

2) in "enabled" mode, checks should fail if the dependency requirements are not met

3) in "disabled" mode, checks for dependencies should not be done at all (!), at least there should be no visible user output. It is not acceptable that the user sees checks for libvorbis in the output or log if they passed -Dvorbis=disable.

4) a global configure switch to use "disable" as default value for all "auto" defaults, so that only things that have been requested explicitly get enabled.

5) a global configure switch to use "enable" as default value for all "auto" defaults, so that meson configure fails if any plugin dependency is not met for plugins that haven't been disabled explicitly. This is useful for distro packagers that want to make sure that all plugins get shipped (or consciously disabled) and don't accidentally disappear from the package just because a dep requirement was added or bumped.

6) plugin-specific dependency() etc. checks should stay in ext/foo/meson.build


For plugins we could probably do something with existing Meson features by doing something like -Drequired-plugins=a,b,c and -Ddisable-plugins=d,e,f and supporting 'all' and 'none' as options. This seems more like an emergency fallback though, and not very nice. Surely we can do something nicer with Meson support?

Something else we want (less important though IMHO): enable/disable a group of plugins. Currently in autotools we have --disable-external to disable plugins with external dependencies, but we may also want e.g. audio plugins, video plugins, subtitle plugins, GPL plugins, exotic plugins.

This use case seems to be supported by the Meson "dolphin" PR already:

 https://github.com/mesonbuild/meson/pull/2411

 https://github.com/mesonbuild/meson/commit/904fe4745d8fb285c28f33dff2afbf3754e875fd#diff-ba7afafbb566a046436c614eb21b8968

I proposed a new type of option, called 'dependency', it can have 3 values: "required" (i.e. enable), "not-required" (i.e. auto) or "disabled" (i.e. disable).

The intended usage in GStreamer would be to have something like this on the top of ext/cairo/meson.build:

cairo_dep = dependency('cairo-gobject', version : '>=1.10.0', required : get_option('cairo'))
if not cairo_dep.found()
  subdir_done()
endif

And in meson_options.txt:
option('cairo', type: 'dependency', value: 'not-required')

About global option to switch all "not-required" to either "required" or "disabled", if I'm not mistaken it could work like this:

meson_options.txt:
option('group_switch', type: 'dependency', value: 'not-required')

ext/cairo/meson.build:
opt = get_option('cairo') or get_option('group_switch')

I think the "or" operator works like this:
required, required => required
required, not-required => required
required, disabled => disabled

no-required, required => required
no-required, not-required => not-required
no-required, disabled => disabled

disabled, required => disabled
disabled, not-required => disabled
disabled, disabled => disabled

The global switch option was supposed to be something built into Meson and should be transparent to the user, not something in meson_options.txt.

Actually the 'or' operator doesn't give what we want, the code should be:
opt = get_option('cairo')
if not opt
  opt = get_option('group_switch')
endif

That's for grouping. About getting a global switch built-in I guess it can be done with the same logic internally in meson.

Forget what I said above, I updated my proposal:
https://github.com/mesonbuild/meson/pull/3376#issuecomment-380239323

Created attachment 370762 [details] [review]
Add option to enable/disable each plugin in good

(In reply to Xavier Claessens from comment #10)
> Created attachment 370762 [details] [review] [review]
> Add option to enable/disable each plugin in good

Wrote a script to generate this patch, could need a few manual edits.

Comment on attachment 370762 [details] [review]
Add option to enable/disable each plugin in good

Thanks for working on this!

Just two comments from a user-point-of-view:

>+if not get_option('audioparsers')
>+  subdir_done()
>+endif

I think this we could probably do nicer if we added something to subdir() ?

But what I think will be confusing is that users now have to know what type a plugin is to enable it or disable it, if I understand correctly. Because the dependencyless options are now booleans so you'd have to do

  meson -Daudioparsers=false

but for the ones with dependencies you'd have to do

  meson -Dpng=required (or whatever name we settle on later)

That seems unfortunate, but easy to fix by just using tristate options there too I guess. Which would still make sense because it will allow for the "disable everything auto by default, and let me opt-in by enabling things explicitly", for example. Which won't be supported with boolean options.

Created attachment 370812 [details] [review]
Add option to enable/disable each plugin in good

Created attachment 370813 [details]
Script to gen option for each plugin

This is quick and dirty, but does the job.

Review of attachment 370812 [details] [review]:

::: ext/cairo/meson.build
@@ -1,1 +1,1 @@
-cairo_dep = dependency('cairo-gobject', version : '>=1.10.0', required : false)
+cairo_dep = dependency('cairo-gobject', version : '>=1.10.0', required : required_opt)

I can't not think that these meson scripts would be cleaner is meson let us create functions.

(In reply to Olivier Crête from comment #15)
> I can't not think that these meson scripts would be cleaner is meson let us
> create functions.

Every time you think a function would help you, check gstreamer/common/m4/, and you'll see why you certainly don't want functions but a proper build system.

(In reply to Xavier Claessens from comment #16)
> (In reply to Olivier Crête from comment #15)
> > I can't not think that these meson scripts would be cleaner is meson let us
> > create functions.
> 
> Every time you think a function would help you, check gstreamer/common/m4/,
> and you'll see why you certainly don't want functions but a proper build
> system.

Because copy-pasting is better?!?!

I don't think there's much point debating this in this bug report. It's not something that's up for debate in Meson. I don't think it will be a problem for us.

Created attachment 372676 [details] [review]
Meson: Add feature option each ext plugin in good

Created attachment 372677 [details] [review]
Meson: Add feature option each plugin in good

Comment on attachment 372677 [details] [review]
Meson: Add feature option each plugin in good

Tri-state options have landed in Meson master, so we can merge this once there is a new Meson release (0.47.0), as we don't want to depend on Meson from git.

>--- a/ext/meson.build
>+++ b/ext/meson.build
>@@ -1,30 +1,28 @@
>-subdir('aalib')
>-subdir('cairo')
>-subdir('flac')
>-subdir('gdk_pixbuf')
>-subdir('gtk')
>-subdir('jack')
>-subdir('jpeg')
>-subdir('lame')
>-subdir('libcaca')
>-# FIXME: dv plugin fails to link with msvc, wants pthread.lib
>-if cc.get_id() != 'msvc'
>-  subdir('dv')
>-endif
>-subdir('libpng')
>-subdir('mpg123')
>-subdir('raw1394')
>-subdir('qt')
>-subdir('pulse')
>-subdir('shout2')
>-subdir('soup')
>-subdir('speex')
>-# FIXME: taglib fails to compile and link with msvc
>-if cc.get_id() == 'msvc'
>-  message('Building with MSVC, disabling taglib support. Patches welcome!')
>-else
>-  subdir('taglib')
>-endif
>-subdir('twolame')
>-subdir('vpx')
>-subdir('wavpack')
>+plugins = [
>+  'aalib',
>+  'cairo',
>+  'dv',
>+  'flac',
>+  'gdk_pixbuf',
>+  'gtk',
>+  'jack',
>+  'jpeg',
>+  'lame',
>+  'libcaca',
>+  'libpng',
>+  'mpg123',
>+  'pulse',
>+  'qt',
>+  'raw1394',
>+  'shout2',
>+  'soup',
>+  'speex',
>+  'taglib',
>+  'twolame',
>+  'vpx',
>+  'wavpack',
>+]
>+
>+foreach plugin : plugins
>+  subdir(plugin)
>+endforeach

Why are you changing this? This is not an equivalent change, and it's not clear to me this is really nicer or cleaner or shorter. Can we just keep it as it is for now? :) (same for gst/meson.build)

> Can we just keep it as it is for now? :)

For maximum correctness we should probably make it so that it errors out if compiler is 'msvc' and the feature was enabled, but that seems like a minor issue.

oh that's leftover from the previous patch where I was skipping subdir in the foreach loop because we didn't have subdir_done() back then.

Checking in gst-plugins-bad, it seems common check for a lib and its header:

dep = cc.find_library('foo', required : false)
if dep.found() and cc.has_header_symbol('foo.h', 'something')
  ...
endif

Converting that pattern to the tristate option is annoying, because if the option is "enabled" we want to abort if header is not found but there is no "required" kwarg for that function.

(In reply to Xavier Claessens from comment #24)
> Checking in gst-plugins-bad, it seems common check for a lib and its header:
> 
> dep = cc.find_library('foo', required : false)
> if dep.found() and cc.has_header_symbol('foo.h', 'something')
>   ...
> endif
> 
> Converting that pattern to the tristate option is annoying, because if the
> option is "enabled" we want to abort if header is not found but there is no
> "required" kwarg for that function.

We probably want cc.find_dependency back: https://github.com/thiblahute/meson/commits/compiler.find_dependency - not 100% sure it is the latest code, I will make sure it is.

You can have a look at how I was using it in the -bad meson component branch: https://github.com/thiblahute/gst-plugins-bad/commit/8bc030477b0f1eed638838e6842fff85efdaae37#diff-f208d0156984b3c606dccdc01ac83d08R9

Doing

  gst-plugins-bad $ git grep -B5 -e 'found.*has_header_symbol'

at least some of the checks are a bit weird, e.g. what's the point/reason of doing a check with dependency() and then also doing cc.has_header_symbol()? That is most likely not only not required, but probably also wrong in most cases because the dep is not actually passed in the check so it won't work for non-standard prefixes. (I will make a patch for this.)

For cc.find_library() I can see the point in the header check.

I think we probably want what Thibault proposed (was looking for that).

Let's start with -base, the non straighforward plugins are:
- gl, no need of a "feature" option, that one has more complicated options already. Right?

- vorbis has 3 deps: 'vorbis', 'vorbisenc' and 'vorbisidec'. If I understand correctly it needs 2 features: 'vorbis' and 'ivorbis'. We should check for vorbisenc_dep only if vorbis_dep is found.

- theora: Do we need 2 features for env/dec or should it be all-or-nothing?

- cdparanoia: There is a comment saying 10.2 has a pkg-config, but Ubuntu 18.04 has libcdparanoia-dev 3.10.2 and there is no .pc file. The check for header is wrong because cdparanoia_found is true even if no header can be found.

- gl: let's skip/ignore it for now

- vorbis: agreed

- theora: I think there should be only one "theora" feature for now and both dec/enc should be mandatory for it. Which doesn't match autotools usage, but I think that's a fringe use case and mostly accidental. Later we may want to add extra features for disabling decoders or encoders in general, but let's cross that bridge when we get there, that's a feature we don't have in the autotools build either yet.

- cdparanoia: the comment says the pkg-config file was added after 10.2, and I don't thin there has been any release since, so we'll have to keep the manual checks I think :/

Created attachment 373152 [details] [review]
Feature options for gstreamer core.

meson: Add feature options for optional deps

The only automagic deps are now those used by tests and examples.

Created attachment 373153 [details] [review]
Feature options for gst-plugins-base.

meson: Add feature options for most external dependencies

GL dependency detection is still automagic.

Created attachment 373154 [details] [review]
Feature options for gst-plugins-good

meson: Add feature options for most external dependencies

Checking for GL, Qt5, and C++ are still automagic. FIXMEs have been
added for these so they can be fixed later.

Created attachment 373155 [details] [review]
Feature options for gst-plugins-bad

meson: Convert common options to feature options

The rest will be converted later, these are necessary for gst-build to
set options correctly.

Created attachment 373156 [details] [review]
Feature options for gst-plugins-ugly

meson: Convert common options to feature options

The rest will be converted later, these are necessary for gst-build to
set options correctly.

Created attachment 373157 [details] [review]
Feature options for gst-editing-services

meson: Convert common options to feature options

The remaining automagic options are in tests and examples.

Created attachment 373158 [details] [review]
Feature options for gst-rtsp-server

meson: Convert common options to feature options

These are necessary for gst-build to set options correctly. The
remaining automagic option is cgroup support in examples.

Created attachment 373159 [details] [review]
Feature options for gst-devtools

meson: Convert common options to feature options

The rest will be converted later, these are necessary for gst-build to
set options correctly.

Created attachment 373160 [details] [review]
Feature options for gst-build

Convert common meson options to feature options

These changes have been mirrored in all subproject repositories.

I stopped porting -bad because there's a lot of plugins in there, and decided to do the minimal work first that's needed to be synchronized across all repos (i.e., convert the "common" yield: true options to feature options). Once these are in, the remaining ones can be added at their own pace.

Review of attachment 373153 [details] [review]:

::: gst/meson.build
@@ +6,3 @@
+    subdir(plugin)
+  endif
+endforeach

As per earlier discussion in this bug, we prefer to keep all subdir() calls and inside the plugin's meson.build do

if get_option('foo').disabled()
  subdir_done()
endif

Created attachment 373161 [details]
Script to gen option for each plugin

I had this updated script for a while, but didn't attach it. It will use subdir_done() instead of the foreach loop in parent directory. Could be useful to Nirbheek if he wants to resume work on this.

Review of attachment 373160 [details] [review]:

::: meson_options.txt
@@ +13,3 @@
 # Common options, automatically inherited by subprojects
+option('gtk_doc', type : 'feature', value : 'auto',
+       description : 'Generate API documentation with gtk-doc')

That's going to be inconsistent with all other GNOME modules that has boolean value. There has been effort to consolidate on the "gtk_doc" naming and now we'll divert again... Not saying it's a bad idea, but I think we should try to coordinate this change for all modules.

(In reply to Xavier Claessens from comment #39)
> Review of attachment 373153 [details] [review] [review]:
> 
> ::: gst/meson.build
> @@ +6,3 @@
> +    subdir(plugin)
> +  endif
> +endforeach
> 
> As per earlier discussion in this bug, we prefer to keep all subdir() calls
> and inside the plugin's meson.build do
> 
> if get_option('foo').disabled()
>   subdir_done()
> endif

The `foreach` mechanism in that patch seems much more elegant and self-contained to me than adding `subdir_done()` separately for each plugin. It's also much less churn.

Adding a new plugin subdir will automatically associate it with an option, and the build system will immediately complain if you forget to define it.

(In reply to Xavier Claessens from comment #41)
> Review of attachment 373160 [details] [review] [review]:
> 
> ::: meson_options.txt
> @@ +13,3 @@
>  # Common options, automatically inherited by subprojects
> +option('gtk_doc', type : 'feature', value : 'auto',
> +       description : 'Generate API documentation with gtk-doc')
> 
> That's going to be inconsistent with all other GNOME modules that has
> boolean value. There has been effort to consolidate on the "gtk_doc" naming
> and now we'll divert again... Not saying it's a bad idea, but I think we
> should try to coordinate this change for all modules.

Well, this sort of thing is actually why I was advocating for -Dfoo={true,false,auto}, because it's backwards-compatible w.r.t. user interface when you switch options over from boolean to feature. I'm sure we can fix that with aliases in the future, so that yes/no|true/false|enabled/disabled are all the same.

Also the name is not set in stone; our build system API is not fixed till we make it the default.

(In reply to Nirbheek Chauhan from comment #42)
> Adding a new plugin subdir will automatically associate it with an option,
> and the build system will immediately complain if you forget to define it.

Good point.

We should prepare an MR for gnome-continuous before the gtk_doc change lands and let ebassi know in advance: https://gitlab.gnome.org/GNOME/gnome-continuous/blob/master/manifest.json#L689

Comment on attachment 373153 [details] [review]
Feature options for gst-plugins-base.

>+# Examples-only feature options
>+option('gdk-pixbuf', type : 'feature', value : 'auto', description : 'GDK-PixBuf support in examples')
>+option('gtk3', type : 'feature', value : 'auto', description : 'GTK+ 3 support in examples')
>+option('qt5', type : 'feature', value : 'auto', description : 'Qt5 support in examples')
>+option('sdl1', type : 'feature', value : 'auto', description : 'SDL 1.2 support in examples')

Not sure if this is really needed, it's certainly more than we provide now and no one ever asked for this as far as  I recall. Wouldn't a simple "examples" option be sufficient?


>+option('x11', type : 'feature', value : 'auto', description : 'X11 ximagesink plugin, and X11 support in libraries, plugins, examples')

Arguably the x11 plugin selection should be independent of x11 support selection for Gst-GL or the examples, but we can always change that later I guess.

Comment on attachment 373154 [details] [review]
Feature options for gst-plugins-good

> # Common options
>+option('examples', type : 'boolean', value : true, yield : true)

Wonder if this should be a feature as well and default to auto?

(Even if we don't have a way to pass arrays of features to required:)

(In reply to Tim-Philipp Müller from comment #45)
> We should prepare an MR for gnome-continuous before the gtk_doc change lands
> and let ebassi know in advance:
> https://gitlab.gnome.org/GNOME/gnome-continuous/blob/master/manifest.
> json#L689

It looks like continuous is already broken, and is chugging along because Meson ignores unknown option arguments. We should fix it, but it shouldn't block these patches.

(In reply to Tim-Philipp Müller from comment #46)
> Comment on attachment 373153 [details] [review] [review]
> Feature options for gst-plugins-base.
> 
> >+# Examples-only feature options
> >+option('gdk-pixbuf', type : 'feature', value : 'auto', description : 'GDK-PixBuf support in examples')
> >+option('gtk3', type : 'feature', value : 'auto', description : 'GTK+ 3 support in examples')
> >+option('qt5', type : 'feature', value : 'auto', description : 'Qt5 support in examples')
> >+option('sdl1', type : 'feature', value : 'auto', description : 'SDL 1.2 support in examples')
> 
> Not sure if this is really needed, it's certainly more than we provide now
> and no one ever asked for this as far as  I recall. Wouldn't a simple
> "examples" option be sufficient?
> 

Yes, I realized when I looked at -bad that this doesn't scale nor does it make sense. I will put it behind the examples option instead.

(In reply to Tim-Philipp Müller from comment #47)
> Comment on attachment 373154 [details] [review] [review]
> Feature options for gst-plugins-good
> 
> > # Common options
> >+option('examples', type : 'boolean', value : true, yield : true)
> 
> Wonder if this should be a feature as well and default to auto?
> 
> (Even if we don't have a way to pass arrays of features to required:)

Let me have a crack at that and see how it looks, but yes it does make sense to do this.

Created attachment 373180 [details] [review]
Feature options for gstreamer core

meson: Add feature options for optional deps

Everything should be behind an option now.

Created attachment 373181 [details] [review]
Feature options for gst-plugins-base

meson: Add feature options for all plugins

GL dependency detection is still automagic.

Created attachment 373182 [details] [review]
Feature options for gst-plugins-good

meson: Add feature options for all plugins

Checks for GL, Qt5, and C++ are still automagic. FIXMEs have been
added for these so they can be fixed later.

Created attachment 373183 [details] [review]
Feature options for gst-plugins-bad

meson: Add feature options for many plugins

The rest will be converted later, these are necessary for gst-build to
set options correctly.

Created attachment 373184 [details] [review]
Feature options for gst-rtsp-server

meson: Convert common options to feature options

These are necessary for gst-build to set options correctly. The
remaining automagic option is cgroup support in examples.

Created attachment 373188 [details] [review]
More feature options for gst-plugins-bad

Add feature options for almost all plugins

The only plugins remaining are those that haven't been ported to Meson
yet, and msdk. Also, the tests are still automagic.

Created attachment 373190 [details] [review]
More feature options for gst-plugins-ugly

Add feature options for all plugins

The only automagic dependency left is C++ availability detection.

Everything committed. Some minor automagic left, but that can be dealt with on a case-by-case basis.