GNOME Bugzilla – Bug 793087
Port to meson build system
Last modified: 2018-02-05 18:46:02 UTC
meson is a build system focused on speed an ease of use, which helps speeding up the software development.
Created attachment 367742 [details] [review] build: Move gvc module to subprojects In order to share the gvc modules between autotools and meson, this patch moves its directory to subprojects directory and updates autotools.
Created attachment 367743 [details] [review] build: Add support for command line arguments The `gsd-power-constants-update.pl` has the files it uses hardcoded. This patch modifies those hardcoded files so they can be specified from the command line.
Created attachment 367744 [details] [review] build: Port to meson build system meson is a build system focused on speed an ease of use, which helps speeding up the software development. This patch adds meson support along autotools.
Created attachment 367745 [details] [review] build: Add meson build files to distributable files Although it is possible to generate distributable files on meson since version 0.41 by using the `ninja dist` command, autotools does different things that end up creating a different distributable file. meson build files have been added to autotools build files as distributable files, so the whole meson port would also be distributed.
Created attachment 367746 [details] [review] build: Remove autotools Just in case it is considered, this patch removes autotools support to avoid the burden of maintaining multiple build systems.
Finally, I have created a wip branch[0] to ease it's testing. Any suggestion would be very much appreciated. [0] https://git.gnome.org/browse/gnome-settings-daemon/log/?h=wip/inigomartinez/meson
Review of attachment 367742 [details] [review]: Looks fine.
Review of attachment 367743 [details] [review]: It might be nice to convert this to another language, though I see that gtk+ still depends on Perl as well, so maybe not so important.
Created attachment 367748 [details] [review] build: Port to meson build system I have rebased these changes with master that has at the moment, two commits more. The patch has been updated accordingly due to these changes.
Created attachment 367749 [details] [review] build: Add meson build files to distributable files This patch has also been updated.
Created attachment 367750 [details] [review] build: Remove autotools Finally, this patch has also been updated.
Created attachment 367752 [details] [review] build: Port to meson build system Options has been thoroughly tested by enabling and disabling them. A couple of fixes has been included. The minimum meson version has also been bumped due to an issue with version comparison.
attachment 367742 [details] [review] pushed as f3273fc5 - build: Move gvc module to subprojects attachment 367743 [details] [review] pushed as 8ed1fb66 - build: Add support for command line arguments
Created attachment 367775 [details] [review] build: Remove autotools I forgot to also remove the `Makefile.am.gresources` file. This patch update removes that file.
Created attachment 367776 [details] [review] build: Use template files for enums generations The data contents for the enum related files are stored in the build files. This patch moves this information to template files to be used along with glib-mkenums.
Created attachment 367790 [details] [review] i18n: Fix gvc submodule file in POTFILES.in Recently the gvc submodule was moved into a new location to ease the transition to meson. However, some strings are extracted from one of the gvc's files. This patch fixes the path to that file in the POTFILES.in file, so strings are extracted properly.
Created attachment 367797 [details] [review] build: Migrate from Intltool to Gettext This patch migrates `gnome-settings-daemon`'s meson port from intltool to gettext. Let me know if this patch should go in another specific bug.
Review of attachment 367749 [details] [review]: I'd rather this wasn't in the tarball at all.
Review of attachment 367752 [details] [review]: Are gvc submodule changes necessary? A couple of nits, but looks good overall. ::: data/meson.build @@ +64,3 @@ + filebase: meson.project_name(), + subdirs: gsd_api_name, + requires: 'glib-2.0' There are actually no dependencies for this, it's header only, and doesn't load up glib.h ::: meson.build @@ +1,3 @@ +project( + 'gnome-settings-daemon', 'c', + version: '3.26.2', I've bumped the version on master, so you'll need to change that. @@ +2,3 @@ + 'gnome-settings-daemon', 'c', + version: '3.26.2', + license: 'GPL2+', It's GPL2+ and LGPL2+ as well though the combined work is GPL2+. @@ +83,3 @@ +pulse_req_version = '>= 2.0' + +colord_dep = dependency('colord', version: '>= 1.0.2') Does this take care of reducing duplicates? Eg. geocode-glib requires gio, upower requires gio, if I link to both, does it link it only once? @@ +128,3 @@ + gudev_dep = dependency('gudev-1.0') +endif +config_h.set('HAVE_GUDEV', enable_gudev) set10 here? @@ +178,3 @@ + +# Rfkill +enable_rfkill = get_option('rfkill') We want to enable this on all Linux systems, it's a requirement for the non-optional Bluetooth and Network panels, as well as gnome-shell. @@ +189,3 @@ + +# Sharing plugin +enable_network_manager = get_option('network_manager') Not optional on Linux, it's required in g-c-c's Network panel as well. @@ +225,3 @@ +) + +output = '\n ' + meson.project_name() + ' ' + meson.project_version() +'\n' The \n at the beginning of lines is a bit ugly, can you move those up to the preceding line instead? ::: meson_options.txt @@ +2,3 @@ +option('udev_dir', type: 'string', value: '', description: 'Absolute path to the udev base directory.') + +option('alsa', type: 'boolean', value: true, description: 'build with ALSA support (not optional on Linux platforms)') Do we really need to make those options? Seeing as they're required on a number of platforms, and unusable on the others? ::: plugins/clipboard/meson.build @@ +1,1 @@ +desktop = 'org.gnome.SettingsDaemon.Clipboard.desktop' Is there any way to make this whole thing into more of a macro? I'd imagine you would need the plugin name in CamelCase, whether it has a GTK+ dep or not, the list of sources and the deps. Might not help a lot for the more complicated plugins with tests... ::: plugins/media-keys/meson.build @@ +50,3 @@ + +cflags += [ + '-DBINDIR="@0@"'.format(gsd_bindir), This is only used by the color plugin, and we really shouldn't need to use it. Remove it everywhere else, and we can file a bug to remove it from the color plugin. @@ +51,3 @@ +cflags += [ + '-DBINDIR="@0@"'.format(gsd_bindir), + '-DGTKBUILDERDIR="@0@"'.format(gsd_pkgdatadir), You can remove this... @@ +52,3 @@ + '-DBINDIR="@0@"'.format(gsd_bindir), + '-DGTKBUILDERDIR="@0@"'.format(gsd_pkgdatadir), + '-DPIXMAPDIR="@0@"'.format(gsd_pkgdatadir) ...and this. Doesn't seem to be used anywhere. ::: plugins/power/meson.build @@ +143,3 @@ +test_py = find_program('test.py') + +# FIXME: the test doesn't work due to issues in the python code The tests require using Python 2 for now. I've got some patches that bring in more Python 3 support, but it's not done yet. ::: plugins/screensaver-proxy/meson.build @@ +16,3 @@ +deps = plugins_deps + [gio_dep,] + +cflags += ['-DLIBEXECDIR="@0@"'.format(gsd_libexecdir)] This isn't used here (and in a number of other places).
Review of attachment 367775 [details] [review]: Let's say yes, after meson has been merged.
Review of attachment 367776 [details] [review]: Nicer, yes.
Review of attachment 367790 [details] [review]: ::: po/POTFILES.in @@ -18,3 @@ plugins/media-keys/gsd-screenshot-utils.c -# Please do not remove this file from POTFILES.in. Run "git submodule init && git submodule update" to get it. -plugins/media-keys/gvc/gvc-mixer-control.c Pretty sure you can remove this altogether, there aren't any user-visible strings to translate.
Review of attachment 367797 [details] [review]: Looks good.
(In reply to Bastien Nocera from comment #22) > Review of attachment 367790 [details] [review] [review]: > > ::: po/POTFILES.in > @@ -18,3 @@ > plugins/media-keys/gsd-screenshot-utils.c > -# Please do not remove this file from POTFILES.in. Run "git submodule init > && git submodule update" to get it. > -plugins/media-keys/gvc/gvc-mixer-control.c > > Pretty sure you can remove this altogether, there aren't any user-visible > strings to translate. $ cat subprojects/gvc/gvc-mixer-control.c |grep '_("' return g_strdup (_("Disabled")); gvc_mixer_stream_set_name (stream, _("System Sounds")); $ cat subprojects/gvc/gvc-mixer-control.c |grep ngettext sinks_str = g_strdup_printf (ngettext ("%u Output", sources_str = g_strdup_printf (ngettext ("%u Input", Unless you mean that these are not shown anywhere by gnome-settings-daemon.
D’uh, I missed “user-visible” in your comment. Sorry!
Created attachment 367843 [details] [review] build: Port to meson build system Almost all the comments have been addressed. Some comments below. (In reply to Bastien Nocera from comment #19) > Review of attachment 367752 [details] [review] [review]: > > Are gvc submodule changes necessary? gvc has to be moved to the commit where meson was merged[0]. The last commits include 3 fixes in the code, and the rest are build improvements. None of them are critical changes and should not impact gnome-settings-daemon in any way. I also suppose that these recent changes are nice for g-s-d. > ::: meson.build > @@ +1,3 @@ > +project( > + 'gnome-settings-daemon', 'c', > + version: '3.26.2', > > I've bumped the version on master, so you'll need to change that. At the time I've checked it[1], the change wasn't present yet and it's still at 3.26.2 > @@ +2,3 @@ > + 'gnome-settings-daemon', 'c', > + version: '3.26.2', > + license: 'GPL2+', > > It's GPL2+ and LGPL2+ as well though the combined work is GPL2+. I suppose that the combined work license is the best option here. > @@ +83,3 @@ > +pulse_req_version = '>= 2.0' > + > +colord_dep = dependency('colord', version: '>= 1.0.2') > > Does this take care of reducing duplicates? Eg. geocode-glib requires gio, > upower requires gio, if I link to both, does it link it only once? Although `geocode-glib`/`upower` and `gio` are not used at the same time, I have checked the rest of the dependencies and I've ended removing a couple of them :) For example, `libpulse-mainloop-glib` includes `libpulse` and they were both used at the same time, so I removed the latter. The other one is `glib`, which is included by other dependencies. > @@ +128,3 @@ > + gudev_dep = dependency('gudev-1.0') > +endif > +config_h.set('HAVE_GUDEV', enable_gudev) > > set10 here? It's not necessary because only `ifndef` is used as guard. > @@ +225,3 @@ > +) > + > +output = '\n ' + meson.project_name() + ' ' + > meson.project_version() +'\n' > > The \n at the beginning of lines is a bit ugly, can you move those up to the > preceding line instead? I've removed all of them except the first one because otherwise it will not fit properly in the screen. > ::: meson_options.txt > @@ +2,3 @@ > +option('udev_dir', type: 'string', value: '', description: 'Absolute path > to the udev base directory.') > + > +option('alsa', type: 'boolean', value: true, description: 'build with ALSA > support (not optional on Linux platforms)') > > Do we really need to make those options? Seeing as they're required on a > number of platforms, and unusable on the others? The `udev_dir` option has been added because by default, it will check the `udevdir` variable in the `udev.pc` file that will point to the udev's directory in the system. However, being a system directory, a user might not be able to install any files, so this option allows installing the file in a directory under prefix or at any other writable directory. Similar approaches have been implemented in a number of packages (`gvfs`, `NetworkManager`, `glib-networking`, etc...). Regarding the `alsa` option, wouldn't it be interesting to have the chance to disable the option on those platforms that are not able to use it? > ::: plugins/clipboard/meson.build > @@ +1,1 @@ > +desktop = 'org.gnome.SettingsDaemon.Clipboard.desktop' > > Is there any way to make this whole thing into more of a macro? I'd imagine > you would need the plugin name in CamelCase, whether it has a GTK+ dep or > not, the list of sources and the deps. > > Might not help a lot for the more complicated plugins with tests... It might help, as all the plugins have their own `desktop` file. This would be easier to handle in the outer loop that iterates over enabled plugins with something like: foreach plugin: plugins configure_file( input: join_paths(plugin, desktop + '.in'), output: join_paths(plugin, desktop), configuration: plugins_conf, install: true, install_dir: gsd_xdg_autostart ) subdir(plugin) endforeach However, meson is still not able to output files in a different directory. We faced the same issue also with `gnome-shell-extensions`[2]. The support for generating files in different directories is already being worked on in meson[3], but it hasn't been merged yet. In this specific case, as all the desktop files are named with different names, as a workaround, we can generate them all in the `plugins` build directory. Removing all the redundant build commands is nice, but I don't know if it makes much sense, and I'm afraid that it might create more issues. I'll create a different patch, so it can be applied optionally. > ::: plugins/power/meson.build > @@ +143,3 @@ > +test_py = find_program('test.py') > + > +# FIXME: the test doesn't work due to issues in the python code > > The tests require using Python 2 for now. I've got some patches that bring > in more Python 3 support, but it's not done yet. Ok. I will let it disabled here so, once it's updated, it will be just a matter of enabling it. Finally, I have also checked all macros defined using the compiler options and removed a lot of them. Please, let me know if I've made any mistake. [0] https://git.gnome.org/browse/libgnome-volume-control/commit/?id=01e1fde6 [1] https://git.gnome.org/browse/gnome-settings-daemon/commit/?id=2159c1ca [2] https://gitlab.gnome.org/GNOME/gnome-shell-extensions/commit/531c6efbab85f94e4d3b7a553203a25c28e695ef#note_11151 [3] https://github.com/mesonbuild/meson/pull/2617
Created attachment 367844 [details] [review] build: Remove autotools An updated patch after the changes in the meson port patch.
Created attachment 367845 [details] [review] i18n: Remove gvc files from POTFILES.in (In reply to Bastien Nocera from comment #22) > Review of attachment 367790 [details] [review] [review]: > > ::: po/POTFILES.in > @@ -18,3 @@ > plugins/media-keys/gsd-screenshot-utils.c > -# Please do not remove this file from POTFILES.in. Run "git submodule init > && git submodule update" to get it. > -plugins/media-keys/gvc/gvc-mixer-control.c > > Pretty sure you can remove this altogether, there aren't any user-visible > strings to translate. I didn't notice that Piotr alread pushed this change[0]. Here goes a patch removing the gvc file. Please change/reword it as you think it's more appropiate. [0] https://git.gnome.org/browse/gnome-settings-daemon/commit/?id=2159c1ca55a35295e530bf66ceca41b0c7c6c1a2
I've put all the commits together in the wip branch[0], so take a look there if you have any doubt. Thank you for your kind support, [0] https://git.gnome.org/browse/gnome-settings-daemon/log/?h=wip/inigomartinez/meson
(In reply to Iñigo Martínez from comment #28) > Created attachment 367845 [details] [review] [review] > i18n: Remove gvc files from POTFILES.in > > (In reply to Bastien Nocera from comment #22) > > Review of attachment 367790 [details] [review] [review] [review]: > > > > ::: po/POTFILES.in > > @@ -18,3 @@ > > plugins/media-keys/gsd-screenshot-utils.c > > -# Please do not remove this file from POTFILES.in. Run "git submodule init > > && git submodule update" to get it. > > -plugins/media-keys/gvc/gvc-mixer-control.c > > > > Pretty sure you can remove this altogether, there aren't any user-visible > > strings to translate. > > I didn't notice that Piotr alread pushed this change[0]. > > Here goes a patch removing the gvc file. Please change/reword it as you > think it's more appropiate. > You should put it in po/POTFILES.skip then, as it technically does include translatable strings.
(In reply to Iñigo Martínez from comment #26) > Created attachment 367843 [details] [review] [review] > build: Port to meson build system > > Almost all the comments have been addressed. Some comments below. > > (In reply to Bastien Nocera from comment #19) > > Review of attachment 367752 [details] [review] [review] [review]: > > > > Are gvc submodule changes necessary? > > gvc has to be moved to the commit where meson was merged[0]. The last > commits include 3 fixes in the code, and the rest are build improvements. > None of them are critical changes and should not impact > gnome-settings-daemon in any way. I also suppose that these recent changes > are nice for g-s-d. Could you please make the rebase in a separate patch? > > ::: meson.build > > @@ +1,3 @@ > > +project( > > + 'gnome-settings-daemon', 'c', > > + version: '3.26.2', > > > > I've bumped the version on master, so you'll need to change that. > > At the time I've checked it[1], the change wasn't present yet and it's still > at 3.26.2 I thought I would do it before clicking "Submit" on the review, but it was long, and I forgot :) It's done now. > > @@ +2,3 @@ > > + 'gnome-settings-daemon', 'c', > > + version: '3.26.2', > > + license: 'GPL2+', > > > > It's GPL2+ and LGPL2+ as well though the combined work is GPL2+. > > I suppose that the combined work license is the best option here. > > > @@ +83,3 @@ > > +pulse_req_version = '>= 2.0' > > + > > +colord_dep = dependency('colord', version: '>= 1.0.2') > > > > Does this take care of reducing duplicates? Eg. geocode-glib requires gio, > > upower requires gio, if I link to both, does it link it only once? > > Although `geocode-glib`/`upower` and `gio` are not used at the same time, I > have checked the rest of the dependencies and I've ended removing a couple > of them :) geocode-glib/upower was an example. > For example, `libpulse-mainloop-glib` includes `libpulse` and they were both > used at the same time, so I removed the latter. > > The other one is `glib`, which is included by other dependencies. OK. > Regarding the `alsa` option, wouldn't it be interesting to have the chance > to disable the option on those platforms that are not able to use it? It should be automatically disabled on non-Linux. But I also meant "Do we need all those options" for the ones below it. > > ::: plugins/clipboard/meson.build > > @@ +1,1 @@ > > +desktop = 'org.gnome.SettingsDaemon.Clipboard.desktop' > > > > Is there any way to make this whole thing into more of a macro? I'd imagine > > you would need the plugin name in CamelCase, whether it has a GTK+ dep or > > not, the list of sources and the deps. > > > > Might not help a lot for the more complicated plugins with tests... > > It might help, as all the plugins have their own `desktop` file. This would > be easier to handle in the outer loop that iterates over enabled plugins > with something like: > > foreach plugin: plugins > configure_file( > input: join_paths(plugin, desktop + '.in'), > output: join_paths(plugin, desktop), > configuration: plugins_conf, > install: true, > install_dir: gsd_xdg_autostart > ) > > subdir(plugin) > endforeach > > However, meson is still not able to output files in a different directory. > We faced the same issue also with `gnome-shell-extensions`[2]. > > The support for generating files in different directories is already being > worked on in meson[3], but it hasn't been merged yet. > > In this specific case, as all the desktop files are named with different > names, as a workaround, we can generate them all in the `plugins` build > directory. > > Removing all the redundant build commands is nice, but I don't know if it > makes much sense, and I'm afraid that it might create more issues. > > I'll create a different patch, so it can be applied optionally. OK, I'll review that separately, thanks for trying it out. > > ::: plugins/power/meson.build > > @@ +143,3 @@ > > +test_py = find_program('test.py') > > + > > +# FIXME: the test doesn't work due to issues in the python code > > > > The tests require using Python 2 for now. I've got some patches that bring > > in more Python 3 support, but it's not done yet. > > Ok. I will let it disabled here so, once it's updated, it will be just a > matter of enabling it. I'd rather this didn't regress and worked in this patch. I'm guessing ${PYTHON} is Python 3, not Python 2. It needs Python 2 for now, and, as it's our regression test, it cannot be disabled. > Finally, I have also checked all macros defined using the compiler options > and removed a lot of them. Please, let me know if I've made any mistake. Thanks. (In reply to Piotr Drąg from comment #30) > (In reply to Iñigo Martínez from comment #28) > > Created attachment 367845 [details] [review] [review] [review] > > i18n: Remove gvc files from POTFILES.in > > > > (In reply to Bastien Nocera from comment #22) > > > Review of attachment 367790 [details] [review] [review] [review] [review]: > > > > > > ::: po/POTFILES.in > > > @@ -18,3 @@ > > > plugins/media-keys/gsd-screenshot-utils.c > > > -# Please do not remove this file from POTFILES.in. Run "git submodule init > > > && git submodule update" to get it. > > > -plugins/media-keys/gvc/gvc-mixer-control.c > > > > > > Pretty sure you can remove this altogether, there aren't any user-visible > > > strings to translate. > > > > I didn't notice that Piotr alread pushed this change[0]. > > > > Here goes a patch removing the gvc file. Please change/reword it as you > > think it's more appropiate. > > > > You should put it in po/POTFILES.skip then, as it technically does include > translatable strings. I've done that now.
Created attachment 367847 [details] [review] i18n: Remove gvc files from POTFILES.in There aren't any user-visible strings to translate, so let's remove this.
Comment on attachment 367847 [details] [review] i18n: Remove gvc files from POTFILES.in Attachment 367847 [details] pushed as ea6725f - i18n: Remove gvc files from POTFILES.in
I still can't figure out why the tests aren't running, and I'm going to give it another try this week-end.
Created attachment 367852 [details] [review] build: Port to meson build system (In reply to Bastien Nocera from comment #31) > It should be automatically disabled on non-Linux. But I also meant "Do we > need all those options" for the ones below it. I'm not sure what options might actually be needed. IMHO, I would remove the cups option, since it is not optional in g-c-c. I have left it for the moment since I may be missing something. I'll happy to remove cups option (or any other) if you agree.
Created attachment 367853 [details] [review] build: Remove autotools Updated after recent changes.
Created attachment 367854 [details] [review] build: Rebase recent libgnome-volume-control I have made the rebase in a different commit. Here it goes.
Created attachment 367855 [details] [review] build: Remove desktop generation duplicated commands Here goes the optional patch that removes the duplicated build commands.
Created attachment 367920 [details] [review] build: Port to meson build system meson is a build system focused on speed an ease of use, which helps speeding up the software development. This patch adds meson support along autotools.
Attachment 367797 [details] pushed as f69e9d8 - build: Migrate from Intltool to Gettext Attachment 367853 [details] pushed as acdd75f - build: Remove autotools Attachment 367854 [details] pushed as f06b1c3 - build: Rebase recent libgnome-volume-control Attachment 367855 [details] pushed as d539772 - build: Remove desktop generation duplicated commands Attachment 367920 [details] pushed as f180fc7 - build: Port to meson build system
Created attachment 367925 [details] [review] build: Apply a workaround for D-Bus code generation A quick and dirty workaround, that should be easily reversible, that using a newly created script tries to simulate a separated generation of D-Bus source code and header.
Comment on attachment 367925 [details] [review] build: Apply a workaround for D-Bus code generation Attachment 367925 [details] pushed as 5924d72 - build: Apply a workaround for D-Bus code generation