GNOME Bugzilla – Bug 790837
Meson: missing many configure options
Last modified: 2017-12-19 19:57:09 UTC
Compared to configure.ac, meson_options.txt is missing many options.
Created attachment 364406 [details] m4 parser to generate complete meson_options.txt This script is of course a quick and dirty way to extract all configure options. Manual edit of the resulting meson_options.txt is required. Probably we don't want to keep then all. Also they require implementation in meson.build. But that gives an idea of what's missing. I've run this script against glib and gst components.
Created attachment 364407 [details] Generated meson_options.txt for glib
Created attachment 364527 [details] meson_options.txt generator Updated my generator to also parse AG_GST_CHECK_FEATURE and AG_GST_CHECK_PLUGIN. It's useless for glib, but let's keep it here for the record.
Created attachment 364529 [details] meson_options.txt generator And now with AG_GST_CHECK_SUBSYSTEM_DISABLE too.
I don't think we want a mechanical port of the Autotools options. Some of those are ridiculous; others are superseded by Meson itself; others are just leftovers from ages past. A quick review of each * option('enable-debug', type : 'combo', choices : ['no', 'minimum', 'yes'], value : '', description : 'turn on debugging @<:@default=glib_debug_default@:>@') Unnecessary; Meson has the `--buildtype` option for this already. * option('enable-gc-friendly', type : 'boolean', value : false, description : 'turn on garbage collector friendliness [default=no]') Let's drop this. * option('enable-mem-pools', type : 'boolean', value : true, description : 'disable all glib memory pools') Same as above. * option('with-runtime-libdir', type : 'string', value : '', description : 'install runtime libraries relative to libdir') What is this I don't even. * option('with-python', type : 'string', value : '', description : 'Path to Python interpreter; searches $PATH if only a program name is given; if not given, searches for a few standard names such as "python3" or "python2"') Meson already depends on Python 3, and already has tools to discover the Python interpreter. Even for the Python 2.x interpreter, I'd still use PATH. If your build environment has Python in a non-standard path then you're going to break something else entirely. * option('with-libiconv', type : 'combo', choices : ['no', 'gnu', 'native'], value : '', description : 'use the libiconv library') This is reasonable; should probably be called `libiconv` and drop the "with". * option('enable-included-printf', type : 'boolean', value : false, description : 'use included printf [default=auto]') Do we still care about broken platforms? Can we use platform detection instead? * option('with-gio-module-dir', type : 'string', value : '', description : 'load gio modules from this directory [LIBDIR/gio/modules]') Urgh, do I really want to know what's this used for? Maybe macOS? * option('enable-selinux', type : 'boolean', value : true, description : 'build without selinux support') Find it hilarious that we have SELinux support but no AppArmor; anyway, yes, we'll need to keep it, but probably rename it as `selinux`. * option('enable-fam', type : 'boolean', value : true, description : 'don't use fam for file system monitoring') Haha, hilarious. This should go on the chopping board. * option('enable-xattr', type : 'boolean', value : true, description : 'build without xattr support') See above, re: SELinux. Rename to `xattr` or `extended-attributes`. * option('enable-libelf', type : 'boolean', value : true, description : 'build without libelf support') Ugh; this should probably under on a platform check. If you're building for Linux, you need this; if you're not building on Linux, you don't. * option('enable-libmount', type : 'boolean', value : false, description : 'build with libmount support [default for Linux]') Same as above: if you're building on Linux, you need this. * option('with-threads', type : 'combo', choices : ['posix', 'win32'], value : '', description : 'specify a thread implementation to use') This should probably be renamed to `threads_implementation` or `threads_impl`. * option('with-pcre', type : 'combo', choices : ['internal', 'system'], value : 'system', description : 'whether to use system PCRE [default=system]') This should probably be turned into a boolean, renamed `internal_pcre`, and default to`false`. * option('enable-man', type : 'boolean', value : false, description : 'generate man pages [default=auto]') This should be renamed to `man` or `man_pages`, and turned into a boolean. It should depend on `xsltproc` being available, and docs enabled, since the man pages are also included in the API reference. * option('enable-dtrace', type : 'boolean', value : false, description : 'include tracing support for dtrace') If at all possible, I'd actually use a platform check for this. * option('enable-systemtap', type : 'boolean', value : false, description : 'include tracing support for systemtap') Same as above. * option('with-tapset-install-dir', type : 'string', value : '', description : 'path where systemtap tapsets are installed [DATADIR/systemtap/tapset]') Should be renamed to `tapset_dir`. * option('enable-coverage', type : 'boolean', value : false, description : 'enable coverage testing with gcov') Meson already has coverage support. * option('enable-bsymbolic', type : 'boolean', value : true, description : 'avoid linking with -Bsymbolic') This is adorable, but should be dropped; we always want to build with -Bsymbolic, if it's available on our platform. * option('enable-znodelete', type : 'boolean', value : true, description : 'avoid linking with -z,nodelete') Considering that things will break horribly in our libraries if we don't link with `-z,nodelete`, this should just be dropped. * option('enable-compile-warnings', type : 'boolean', value : true, description : 'Don't use builtin compiler warnings') No; we need to add *more* compiler warnings, and we can use `--werror` in Meson if we want to turn them into errors on our automated builders. +++ The rest of the options come from m4 files that projects using Autotools will end up including, and do not apply to GLib itself; or they refer to the tools we depend on — like gtk-doc; or they apply to tools we don't use when building with Meson — like libtool.
(In reply to Emmanuele Bassi (:ebassi) from comment #5) > I don't think we want a mechanical port of the Autotools options. Some of > those are ridiculous; others are superseded by Meson itself; others are just > leftovers from ages past. Of course, not suggesting to add them all, but they need to be reviewed one by one as you did. Thanks.
We should also think about naming convention, currently glib has a few "with-" and "enable-" options and here you suggest to not have that prefix.
(In reply to Xavier Claessens from comment #7) > We should also think about naming convention, currently glib has a few > "with-" and "enable-" options and here you suggest to not have that prefix. See this thread: https://mail.gnome.org/archives/desktop-devel-list/2017-November/msg00076.html and this wiki page: https://wiki.gnome.org/Initiatives/GnomeGoals/MesonPorting about the best practices for porting projects to Meson, including naming options.
Created attachment 364533 [details] meson_options.txt generator Updated with naming convention
Created attachment 364574 [details] [review] WIP: Meson: Add missing options and conform to naming guidelines
(In reply to Emmanuele Bassi (:ebassi) from comment #5) > * option('with-runtime-libdir', > type : 'string', > value : '', > description : 'install runtime libraries relative to libdir') > > What is this I don't even. It seems to be used on Makefiles, so we'll have to figure it out. > * option('with-gio-module-dir', > type : 'string', > value : '', > description : 'load gio modules from this directory > [LIBDIR/gio/modules]') > > Urgh, do I really want to know what's this used for? Maybe macOS? We'll have to figure it out > * option('enable-libelf', > type : 'boolean', > value : true, > description : 'build without libelf support') > > Ugh; this should probably under on a platform check. If you're building for > Linux, you need this; if you're not building on Linux, you don't. Currently meson.build simply check for non required dependency. I think that's the right thing to do. Maybe it could error if not found target is linux? > * option('enable-libmount', > type : 'boolean', > value : false, > description : 'build with libmount support [default for Linux]') > > Same as above: if you're building on Linux, you need this. ATM it's not required even on linux. meson_options.txt already has a yes/no/auto option. Do you think it should be boolean default to true? > * option('with-threads', > type : 'combo', > choices : ['posix', 'win32'], > value : '', > description : 'specify a thread implementation to use') > > This should probably be renamed to `threads_implementation` or > `threads_impl`. Currently meson.build has code to autodetect this. Maybe we don't need an option. > * option('enable-dtrace', > type : 'boolean', > value : false, > description : 'include tracing support for dtrace') > > If at all possible, I'd actually use a platform check for this. > > * option('enable-systemtap', > type : 'boolean', > value : false, > description : 'include tracing support for systemtap') > > Same as above. You think they should be automatically enabled on platforms that support them, with no option? Those options are already used in meson build, I think it's reasonable to keep them.
(In reply to Xavier Claessens from comment #11) > (In reply to Emmanuele Bassi (:ebassi) from comment #5) > > * option('with-runtime-libdir', > > type : 'string', > > value : '', > > description : 'install runtime libraries relative to libdir') > > > > What is this I don't even. > > It seems to be used on Makefiles, so we'll have to figure it out. See commit 0ccd18bc83c5e6eff77940a61cc9b31a88dd1851. Looks like something legit, it's for when glib is used before /usr gets mounted. But it's really ugly in the Makefile.am...
Created attachment 364600 [details] [review] WIP: Meson: Add missing options and conform to naming guidelines
Note that I'm still new to meson, so please review carefully, I'm not sure it's correctly implemented. Especially: - I simplified selinux by just depending on the pc file, is that OK? - XATTR_NOFOLLOW: that code does not compile, it has too many args passed to getxarttr (could be a bug in autotools too?). And how is HAVE_SYS_XATTR_H supposed to be defined?
Created attachment 364626 [details] [review] WIP: Meson: Add missing options and conform to naming guidelines
(In reply to Xavier Claessens from comment #14) > - XATTR_NOFOLLOW: that code does not compile, it has too many args passed to > getxarttr (could be a bug in autotools too?). And how is HAVE_SYS_XATTR_H > supposed to be defined? Solved that in lastest patched. Didn't know that getxattr has different signature on macos.
(In reply to Xavier Claessens from comment #16) > (In reply to Xavier Claessens from comment #14) > > - XATTR_NOFOLLOW: that code does not compile, it has too many args passed to > > getxarttr (could be a bug in autotools too?). And how is HAVE_SYS_XATTR_H > > supposed to be defined? > > Solved that in lastest patched. Didn't know that getxattr has different > signature on macos. ⇒ Why it is important that an autotools → Meson port doesn’t regress on any of this configuration stuff, since all these horrible corner cases have been solved already.
what do you mean they have been solved already? I have ~0 knowledge on the macos plateform, but I guess glocalfileinfo.c won't build on macos without this patch, because it uses getxattr and it has a different signature there. Or do you mean macos has same getxattr than linux now?
Well, it builds atm because we don't define HAVE_XATTR so the whole feature is disabled when building with meson.
Created attachment 365371 [details] [review] Meson: Add missing options and conform to naming guidelines
Re-attached with the "WIP:" removed because I think it's ready to get merged.
Review of attachment 365371 [details] [review]: ::: config.h.meson @@ +352,2 @@ /* Define to 1 if libselinux is available */ #mesondefine HAVE_SELINUX Could we get rid of config.h.meson wholesale? ::: meson.build @@ +1408,3 @@ + found_iconv = true + endif + elif libiconv_opt == 'no' This is a bit weird: 'no' leads to libiconv detection? Either this is wrong, or the option value needs to be changed. ::: meson_options.txt @@ +8,3 @@ + choices : ['no', 'gnu', 'native', 'maybe'], + value : 'maybe', + description : 'use the libiconv library') Care to (briefly) describe each option in the `description` field? @@ +13,3 @@ + type : 'string', + value : '', + description : 'directory for charset.alias file (libdir by default)') (default to 'libdir' if unset) @@ +18,3 @@ + type : 'string', + value : '', + description : 'load gio modules from this directory [LIBDIR/gio/modules]') (default to 'libdir/gio/modules' if unset) @@ +33,3 @@ + type : 'boolean', + value : false, + description : 'build with libmount support [default for Linux]') default for Linux, except the default value is "false". @@ +38,3 @@ + type : 'boolean', + value : false, + description : 'whether to use internal PCRE [default=system]') The "default" bit here is confusing, just drop it. @@ +43,3 @@ + type : 'boolean', + value : false, + description : 'generate man pages') Add "(requires xsltproc)".
(In reply to Emmanuele Bassi (:ebassi) from comment #22) > Review of attachment 365371 [details] [review] [review]: > > ::: config.h.meson > @@ +352,2 @@ > /* Define to 1 if libselinux is available */ > #mesondefine HAVE_SELINUX > > Could we get rid of config.h.meson wholesale? It won't build out of the box, so extra work is required. Since it's not related to this patch, I opened bug #791486. > @@ +33,3 @@ > + type : 'boolean', > + value : false, > + description : 'build with libmount support [default for Linux]') > > default for Linux, except the default value is "false". Changed the default to true, so on plateforms that doesn't have libmount they should specify libmount=false explicitly. Wondering if nowadays we could drop the cc.find_library('mount') check and always require that libmount comes with a pc file?
Created attachment 365378 [details] [review] Meson: Add missing options and conform to naming guidelines
Review of attachment 365378 [details] [review]: ACK-by: me
Thanks, merged.