GNOME Bugzilla – Bug 793627
Various meson related improvements
Last modified: 2018-05-24 11:30:50 UTC
This bug includes various meson improvements.
Created attachment 368594 [details] [review] build: Remove unnecessary default options `warning_level` is already 1 by default in meson, so it's not necessary to set this value as default option. The C99 standard was also incorrectly introduced in the meson port.
Created attachment 368595 [details] [review] build: Remove unnecessary variables and defines There are a number of variables and defines that were available in autotools and were ported to meson that are actually not necessary. Anything unnecessary has been removed.
Created attachment 368596 [details] [review] build: Migrate to new meson porting guidelines Following the new meson porting guidelines, this patch renames the build options. The list of changes is as follows: - Remove the enable prefix from boolean options. - Remove the with prefix from string options. - The character separator from multi-word options has been changed to underscore. Automatic feature detection has also been removed. This has involved reworking some build aspects particularly when adding plguins to the build. The old `with-plugins` option has been renamed to `plugins` and now it's a meson array option, that handles comma separated values. This option is used now to handle fine grained plugin sets. meson version has also been bumped to 0.44 to be able to use the array type. Another meson option has also been added to help with this. This new option called 'plugins_set` allows setting a predefined set of plugins. This both options can be combined to enable a custom set of plugins.
Created attachment 368597 [details] [review] build: Use LINGUAS file on help generation The `linguas` parameter is deprecated when generating help with yelp. The languages to be used along with `linguas` parameter have been moved to the `LINGUAS` file.
Created attachment 368598 [details] [review] build: Improve debug handling totem is considered a debug build if the build type is not `release`. However this is wrong, because `plain` type should not be considered a debug build. The way compiler flags are handled on debug build has also been changed by taking advantage of the `get_supported_arguments` helper function.
Created attachment 368599 [details] [review] build: Remove build file from docs directory The meson build file in `docs` directory is used only to include the `reference` directory. The file has been removed and the full `docs/reference` path is used from the root source directory.
Created attachment 368600 [details] [review] build: Improve gst helpers build Totem builds three static helper libraries that are related to gstreamer. The build commands create different variables for holding their library dependencies which are not necessary. The meson internal dependencies created to use those static libraries in other targets have also been improved by including those libraries which are needed.
Created attachment 368601 [details] [review] build: Improve backend build The backend build is creating different variables that are used only once, so the common variables can be reused. The `libbaconvideowidget` dependency also needs to reference the created enum header and the `clutter-gtk` library so they both have been added. The compiler flags and dependencies of `bvw-test` have also been revised.
Created attachment 368602 [details] [review] build: Reformat properties build file Changed `properties` build file to fit to better meson formatting. The library name has also been fixed by removing the `lib` prefix. The `gtk+` library dependency has also been added to the properties internal dependency.
Created attachment 368603 [details] [review] build: Reformat src build file Changed `src` build file to fit to better meson formatting. All dependencies has also been revised. Redundant dependencies have been removed. A comment has also been added regarding introspection support because it is not optional in its current state. The `exec_prefix` pkg-config variable has also been fixed. Finally the introspection GIR file generation has also been improved by using recent meson build parameters.
Created attachment 368604 [details] [review] build: Improve plugins build files The build files involved on plugins building and installation have been improved. The description `plugin` file of each plugin is created now at the `src/plugin` directory that simplifies the build commands. All dependencies has also been revised. Redundant dependencies have been removed. Common dependencies, compiler arguments and included directories have been now grouped under the same dependency. Following the `subdir` -> `plugin name ` pattern, the `properties` directory has been renamed to `movie-properties`. The `im-status` plugin files have also been renamed to remove the `totem-` prefix. Now, only the plugins passed from the command line are checked because those are the ones that can only be error prone.
Created attachment 368605 [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 368606 [details] [review] plugins: Migrate plugins from Intltool to Gettext This patch migrates plugins files from Intltool to Gettext by using meson's i18n features. However, the migration has workarounds for two issues. The first issue lies in the fact that, even though the `plugin` uses the Desktop entry specification[0], it is not recongnized as such by gettext because it uses the file extension to detect the supported files. The second issue resides in the translatable entries used by the `plugin` files: `Name` and `Description`. Although `Name` is part of the default keyworks, `Description` is not. As a workaround for these problems, the files has been renamed to include the `desktop` extension, which helps gettext detecting the `plugin` files. After the translation merge process they are renamed to their right name without the `desktop` extension. The default procedure to extract the translatable strings and merge them has also been changed. New targets have been included to generate the `pot` file and merge the translations. [0] https://specifications.freedesktop.org/desktop-entry-spec/desktop-entry-spec-latest.html
Created attachment 368607 [details] [review] build: Check D-Bus session bus services directory Totem install a session bus service file. This file is installed in the `dbus-1/services` directory under `datadir`. However, the real directory can be checked from the `dbus-1.pc` pkg-config file. A new option has also been added to set as a different session bus services directory, so the user can install the file in it.
(In reply to Iñigo Martínez from comment #13) > Created attachment 368606 [details] [review] [review] > plugins: Migrate plugins from Intltool to Gettext > > The default procedure to extract the translatable strings and merge them has > also been changed. New targets have been included to generate the `pot` file > and merge the translations. > This sounds like something Damned Lies can’t do. Claude, could you confirm? This affects also other apps with .plugin files that moved to Meson (https://bugzilla.gnome.org/show_bug.cgi?id=793658).
I confirm that GNOME modules should not require running the build system to produce pot files. We may talk with the gettext folks, to know if it's possible to trick gettext considering those .plugin files as .desktop files.
Iñigo tried to add proper support to gettext (http://lists.gnu.org/archive/html/bug-gettext/2017-06/msg00001.html), but it seems to have stalled (and gettext development slowed down significantly in the past couple of years.)
GNOME Clocks is another app with this problem: https://bugzilla.gnome.org/show_bug.cgi?id=793677
Created attachment 368696 [details] [review] plugins: Migrate plugins from Intltool to Gettext I've updated the patch using the `args` parameter in `gettext` as eog does[0], which is cleaner than using the entire command. [0] https://git.gnome.org/browse/eog/commit/?id=956e8239723fc8bc4a043dc268e5a2a64a7a9817
Created attachment 368697 [details] [review] build: Check D-Bus session bus services directory Fixed a missing comma.
(In reply to Piotr Drąg from comment #17) > Iñigo tried to add proper support to gettext > (http://lists.gnu.org/archive/html/bug-gettext/2017-06/msg00001.html), but > it seems to have stalled (and gettext development slowed down significantly > in the past couple of years.) I should get back to this at some point.
Created attachment 368701 [details] [review] build: Do not use a variable for python3 module The `python3` module is used in meson only to check that a recent version is available, so there is no need to store the module in a variable for any other use.
Created attachment 368702 [details] [review] build: Remove Makefile.plugins file The Makefile.plugins belongs to autotools which was already removed. It also contained a procedure to check python files. This has also been added to meson with `check_python` target name.
Created attachment 368706 [details] [review] build: Improve documentation generation Some of the parameters used in documentation generation are duplicated because many of them are already added by meson. The `src` directory is the only one that has been left as there is no need for the rest. The references has also been fixed by using the proper path.
(In reply to Iñigo Martínez from comment #19) > Created attachment 368696 [details] [review] [review] > plugins: Migrate plugins from Intltool to Gettext > > I've updated the patch using the `args` parameter in `gettext` as eog > does[0], which is cleaner than using the entire command. > So Claude has improved Damned Lies (thank you!) in https://git.gnome.org/browse/damned-lies/commit/?id=720a991b728d35c91c2ea8d54dc91572d0677afc, and this works for us now.
Review of attachment 368594 [details] [review]: ::: meson.build @@ -5,3 @@ - default_options: [ - 'buildtype=debugoptimized', - 'c_std=gnu99', I'll re-add it in a follow-up commit.
Review of attachment 368595 [details] [review]: Looks good otherwise. ::: meson.build @@ -83,3 @@ common_flags = [ '-DHAVE_CONFIG_H', - '-D_REENTRANT', Are you sure about this? I'd rather not remove it unless we can be certain it doesn't cause problems (probably on X11 and threading, those libs are weird).
Review of attachment 368596 [details] [review]: This probably needs work. Splitting up the plugins changes from the option renaming would have been nice, and made this more readable as well. I'll set this as needs-work, and we'll figure out whether those changes are acceptable for a stable release. ::: src/plugins/meson.build @@ +61,3 @@ +enable_plugins += get_option('plugins') + +# Sanity check: Make sure enabled extensions are valid and also remove duplicates Extensions? Do you mean plugins? In the autotools days, passing "all" as the plugins to enable would error out if any of the plugins wasn't buildable, because of missing dependencies for example. I don't think we need a "base" option either, the default should enable the plugins for which the dependencies are correctly set.
Review of attachment 368597 [details] [review]: Looks good
Review of attachment 368598 [details] [review]: Sure.
Review of attachment 368599 [details] [review]: Sure.
Review of attachment 368600 [details] [review]: Why would I want to merge those? I don't understand.
Review of attachment 368601 [details] [review]: This patch really needs splitting up. The enum generation changes can be done separately of everything else, for example. ::: meson.build @@ +121,3 @@ totem_plparser_req_version = '>= 3.10.1' +clutter_gst_dep = dependency('clutter-gst-3.0', version: '>= 2.99.2') Why declare it here if it's only used in the src/backend directory? ::: src/backend/meson.build @@ +93,3 @@ + cflags = warn_flags + [ + '-DG_LOG_DOMAIN="@0@"'.format(backend_test), + '-DLOGO_PATH="@0@"'.format(join_paths(totem_pkgdatadir, meson.project_name(), 'totem_logo.png')) The logo path isn't used at all, and can be removed in a separate commit. ::: src/meson.build @@ +57,3 @@ glib_dep, gio_dep, + gst_tag_dep, I don't think I want that. This is an internal dependency of the video widget, so linking against the video widget static library should bring that in.
Review of attachment 368602 [details] [review]: > Changed `properties` build file to fit to better meson formatting. > The library name has also been fixed by removing the `lib` prefix. > > The `gtk+` library dependency has also been added to the properties > internal dependency. Those really need to be separate commits.
Review of attachment 368603 [details] [review]: Again, those need to be separated, this is unreviewable.
Review of attachment 368604 [details] [review]: Ditto.
Review of attachment 368605 [details] [review]: That's good. Does it require a particular version of glib?
Review of attachment 368696 [details] [review]: > recongnized Typo. ::: po/POTFILES.in @@ -18,3 @@ -src/properties/bacon-video-widget-properties.c -[type: gettext/glade]src/grilo.ui -src/totem.c Why reorder the filenames? This just makes it more difficult to review those patches.
Review of attachment 368697 [details] [review]: I'm pretty sure we don't want to do that, as 1) it adds a compile-time dependency on dbus 2) it would try to install system-wide unless the user had built said dbus in their alternate location (say, jhbuild)
Review of attachment 368701 [details] [review]: Sure.
Review of attachment 368702 [details] [review]: > The Makefile.plugins belongs to autotools which was already removed. > > It also contained a procedure to check python files. This has also > been added to meson with `check_python` target name. Please make those changes separately.
Review of attachment 368706 [details] [review]: If you verified that the generated docs match the old ones, looks good.
attachment 368594 [details] [review] pushed as 522531e7 - build: Remove unnecessary default options attachment 368595 [details] [review] pushed as 3c076201 - build: Remove unnecessary variables and defines attachment 368597 [details] [review] pushed as 300349c0 - build: Use LINGUAS file on help generation attachment 368598 [details] [review] pushed as 0dbed5a4 - build: Improve debug handling attachment 368599 [details] [review] pushed as c2986ff7 - build: Remove meson build file from docs directory attachment 368605 [details] [review] pushed as 54bce40a - build: Use template files for enums generation attachment 368706 [details] [review] pushed as c84daa27 - build: Improve documentation generation
(In reply to Bastien Nocera from comment #26) > Review of attachment 368594 [details] [review] [review]: > - 'c_std=gnu99', > > I'll re-add it in a follow-up commit. Why not use the compiler's default standard? Otherwise you would be limited to an fixed standard even if the compiler uses a modern standard by default. (In reply to Bastien Nocera from comment #27) > Review of attachment 368595 [details] [review] [review]: > - '-D_REENTRANT', > > Are you sure about this? I'd rather not remove it unless we can be certain > it doesn't cause problems (probably on X11 and threading, those libs are > weird). I've added it back. However, shouldn't this be added by any particular dependency? Let's say any X11 library if it might cause any problem. (In reply to Bastien Nocera from comment #37) > Review of attachment 368605 [details] [review] [review]: > > That's good. Does it require a particular version of glib? AFAIK, meson is not limited by any specific version and `glib-mkenum` was available in version `2.35.0`, which is the one used by totem.
(In reply to Iñigo Martínez from comment #44) > (In reply to Bastien Nocera from comment #26) > > Review of attachment 368594 [details] [review] [review] [review]: > > - 'c_std=gnu99', > > > > I'll re-add it in a follow-up commit. > > Why not use the compiler's default standard? Otherwise you would be limited > to an fixed standard even if the compiler uses a modern standard by default. We used to use C89, now C99. I don't think we'd want to support newer standards if there was one, as this is the base-level functionality. > (In reply to Bastien Nocera from comment #27) > > Review of attachment 368595 [details] [review] [review] [review]: > > - '-D_REENTRANT', > > > > Are you sure about this? I'd rather not remove it unless we can be certain > > it doesn't cause problems (probably on X11 and threading, those libs are > > weird). > > I've added it back. However, shouldn't this be added by any particular > dependency? Let's say any X11 library if it might cause any problem. Not remembering which library might need this, I'd rather keep it here. > (In reply to Bastien Nocera from comment #37) > > Review of attachment 368605 [details] [review] [review] [review]: > > > > That's good. Does it require a particular version of glib? > > AFAIK, meson is not limited by any specific version and `glib-mkenum` was > available in version `2.35.0`, which is the one used by totem. Sounds good.
(In reply to Bastien Nocera from comment #28) > Review of attachment 368596 [details] [review] [review]: > ::: src/plugins/meson.build > @@ +61,3 @@ > +enable_plugins += get_option('plugins') > + > +# Sanity check: Make sure enabled extensions are valid and also remove > duplicates > > Extensions? Do you mean plugins? Yes, my fault :(. This was based on the work done on gnome-shell-extensions, and thats why I used extensions instead of plugins. > In the autotools days, passing "all" as the plugins to enable would error > out if any of the plugins wasn't buildable, because of missing dependencies > for example. > I don't think we need a "base" option either, the default should enable the > plugins for which the dependencies are correctly set. Following the meson porting guidelines, I've renamed the options but I've also removed the automatic feature detection. This means that if all the plugins are going to be built, all requirements must be met or the user must disable all those plugins, which can be very disturbing for the user. My approach to avoid this issue has been to create groups based on their requirements. `base` group would consist on those basic requirements, which must be fully complied. I would like to know if this approach can fit in totem, or if we should avoid that guideline, because much of the work done on the following patches is built over this premise.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/totem/issues/237.