GNOME Bugzilla – Bug 793719
Various meson related improvements
Last modified: 2018-03-05 21:28:27 UTC
This bug includes various meson improvements.
Created attachment 368750 [details] [review] build: Remove default explicit installation directory Some meson targets that are installed in default directories are explicitly indicated. However, due to the fact that they are default installation directories, they are not necessary.
Created attachment 368753 [details] [review] build: Fix exec_prefix variable The `exec_prefix` variable inside the pkg-config file is pointing `libexecdir` but it should be pointing to prefix instead. This has been changed so `exec_prefix` points to the proper path. The `libexecdir` meson variable has also been removed because the only the `pkglibexecdir` variable is used.
Created attachment 368754 [details] [review] build: Remove unused variables and defines Although micro and macro versions are commonly used, they are not in Eye of GNOME, so there is no need to create them. Some PACKAGE_* definitions are also not used in Eye of GNOME. Finally, there is a variable pointing out if a debug build has been enabled or not. This is used only once, so it's not necessary to hold this information. All of these have been removed.
Created attachment 368755 [details] [review] build: Remove duplicated dependencies There are some dependencies which are also included on other dependencies, so there is no need to include both. Those dependencies that already included in other dependencies have been removed.
Created attachment 368756 [details] [review] build: Do not use options as auto There are some options that can be used to enable or disable options. However, if the conditions to use those components enabled by options are not fulfilled, they are disabled and this might confuse an user. Now, if the conditions of those enabled components are not fulfilled the build configuration will stop showing a message to the user. meson variables have been renamed from the `have_` pattern to the `enable_` pattern to better reflect this behaviour. The `zlib` detection behaviour has also been fixed, which will require both `inflate` and `crc32` to be available.
Created attachment 368757 [details] [review] build: Improve jpegutils handling Add explicitly jpegutils when libjpeg option is enabled, instead of using an empty string when the option is disabled.
Created attachment 368758 [details] [review] build: Improve dependency handling There are a set of dependencies which are used for building the shared libraries and another set for those using the library. This two sets has been split and a new set has been created with common dependencies. The introspection dependency from `gobject-introspection` has also been removed because it's already included in `libpeas` dependency.
Created attachment 368759 [details] [review] build: Remove --warn-all parameter meson already includes the `--warn-all` parameter on introspection generation. It has been removed to avoid the parameter duplication.
Created attachment 368760 [details] [review] build: Fix compiler flags The compiler flags used for the shared library and the executable are different and there is not common flag between them. They have been modified so now only those that are necessary are added to each targets.
Created attachment 368761 [details] [review] build: Reuse pkglibdir meson variable The installation directory used on the build of the shared library was used in the pkg-config file generation. Due to this behaviour when using the full path as shared library generation caused the libdir path to be wrong. However, this has been fixed in meson's 0.44 version so the pkglibdir directory is reused for this. The required meson version has also been updated accordingly.
Created attachment 368828 [details] [review] build: Create msgfmt command before loop A custom command is used for merging translations with `msgfmt` in loop that iterates over the available plugins. The command is used without any modifications on each loop iteration so it can be created before entering the loop to avoid creating it every iteration.
Review of attachment 368755 [details] [review]: No, although the risk may be low, we shouldn't do that. They might be included by other dependencies, but relying on other dependencies pulling them in makes them "indirect". Problems could occur if the direct dependencies change their own dependencies. Relying on indirect dependencies IIRC also caused problems when distros started linking with -Wl,--as-needed a few years ago. We might have the case where we need a more recent version of an indirect dependency than is pulled in by the direct dependency.
Review of attachment 368756 [details] [review]: Yes, that sounds like a good idea.
Review of attachment 368758 [details] [review]: ::: meson.build @@ -158,3 @@ -if enable_introspection - eog_deps += dependency('gobject-introspection-1.0', version: '>= 0.6.7') -endif Let's keep that one for the same reasons. ;-)
Review of attachment 368760 [details] [review]: ::: src/meson.build @@ -141,3 @@ '-DEOG_PLUGIN_DIR="@0@"'.format(eog_pluginsdir), - '-DEOG_PREFIX="@0@"'.format(eog_prefix), - '-DG_LOG_DOMAIN="@0@"'.format(meson.project_name().to_upper()), Hmm, I am not sure about this right now. Isn't this still needed for logging from the private so?
Pushed 368750 as 67f7135d - build: Remove default explicit installation directory Pushed 368753 as df246fcb - build: Fix exec_prefix variable Pushed 368754 as 5404a021 - build: Remove unused variables and defines Pushed 368756 as daf0193d - build: Do not use options as auto Pushed 368757 as 29fab0f8 - build: Improve jpegutils handling Pushed 368759 as 5b34ab7a - build: Remove --warn-all parameter Pushed 368761 as 96eec4ac - build: Reuse pkglibdir meson variable Pushed 368828 as 9e2073b1 - build: Create msgfmt command before loop
Created attachment 368992 [details] [review] build: Improve dependency handling Following your comments (comment 12 and comment 14), I have reworked this patch. It does check all dependencies without removing any of them. However, some of them seem to be unused: - gio-unix-2.0. - gsettings-desktop-schemas - gtk+-unix-print-3.0 - shared-mime-info Are these actually necessary? I've also noticed that even if `introspection` option is disabled, this is necessary for building `libeog`, which has sense due to the use of `libpeas`.
Created attachment 368994 [details] [review] build: Fix compiler flags (In reply to Felix Riemann from comment #15) > Review of attachment 368760 [details] [review] [review]: > > ::: src/meson.build > @@ -141,3 @@ > '-DEOG_PLUGIN_DIR="@0@"'.format(eog_pluginsdir), > - '-DEOG_PREFIX="@0@"'.format(eog_prefix), > - '-DG_LOG_DOMAIN="@0@"'.format(meson.project_name().to_upper()), > > Hmm, I am not sure about this right now. Isn't this still needed for logging > from the private so? You are right. I assumed that `G_LOG_DOMAIN` was only necessary in the executable. I have duplicated this flag in both compiler flag set, so it can be modified easily if different domains are necessary.
Review of attachment 368992 [details] [review]: (In reply to Iñigo Martínez from comment #17) > Created attachment 368992 [details] [review] [review] > build: Improve dependency handling > > Following your comments (comment 12 and comment 14), I have reworked this > patch. It does check all dependencies without removing any of them. However, > some of them seem to be unused: > > - gio-unix-2.0. > - gsettings-desktop-schemas > - gtk+-unix-print-3.0 > - shared-mime-info > > Are these actually necessary? - gio-unix is needed for GDesktopAppInfo. - gtk+-unix-print appears to be actually unneeded. It seems to have been needed for GtkPrintUnixDialog. However, I can't where we ever needed it. Maybe it was because of some other symbol but headers changed over the years and eog can live without it now. - gsettings-desktop-schemas is something of a runtime dependency to help ensuring it is installed on non-GNOME-Desktop systems. eog uses at least the desktop-wide dconf keys to change the background image. I think flags to globally disable printing/saving are defined here as well. - shared-mime-info is a "runtime dependency" as well. That version is needed to recognize all the mime types we list in eog.desktop. > I've also noticed that even if `introspection` option is disabled, this is > necessary for building `libeog`, which has sense due to the use of `libpeas`. Hmm, yes libpeas unconditionally pulls it in. Also it's not really optional in eog itself right now, due to this code missing the HAVE_INTROSPECTION ifdefs: https://git.gnome.org/browse/eog/tree/src/eog-plugin-engine.c?id=789afdc1647342ea905104db873fe86aba928708#n92 It doesn't show though because of libpeas pulling it in anyway. So we could either add the ifdefs to EogPluginEngine (allows an intrespection-free build if libpeas allows it) or call it a hard dependency. ::: src/meson.build @@ +139,3 @@ + zlib_dep, + cc.find_library('m') +] Aren't some of these actually depencies of libeog: gio_unix, gnome_desktop, libpeas_gtk? The eog executable only requires glib/gio/gtk and libeog, no?
Created attachment 369169 [details] [review] build: Improve dependency handling (In reply to Felix Riemann from comment #19) > Review of attachment 368992 [details] [review] [review]: > - gtk+-unix-print appears to be actually unneeded. It seems to have been > needed for GtkPrintUnixDialog. However, I can't where we ever needed it. > Maybe it was because of some other symbol but headers changed over the years > and eog can live without it now. This seems to be my mistake. `eog-print.c` uses symbols from `gtk+-unix-print` so this shouldn't be removed. > - gsettings-desktop-schemas is something of a runtime dependency to help > ensuring it is installed on non-GNOME-Desktop systems. eog uses at least the > desktop-wide dconf keys to change the background image. I think flags to > globally disable printing/saving are defined here as well. > - shared-mime-info is a "runtime dependency" as well. That version is needed > to recognize all the mime types we list in eog.desktop. I have modified the patch so the required versions of these dependencies are checked but they are not stored, so they are not used. > ::: src/meson.build > @@ +139,3 @@ > + zlib_dep, > + cc.find_library('m') > +] > > Aren't some of these actually depencies of libeog: gio_unix, gnome_desktop, > libpeas_gtk? > The eog executable only requires glib/gio/gtk and libeog, no? True. `common_deps` which holds dependencies for `gdk-pixbuf-2.0`, `gio-2.0`, `glib-2.0`, `gtk+-3.0` and `libpeas-1.0` is used in both targets, `libeog` and `eog`. On the other hand `libeog` also uses `gio-unix-2.0`, `gnome-desktop-3.0`, `gtk+-unix-print-3.0`, `libpeas-gtk-1.0` and `z` and `m` libraries.
(In reply to Iñigo Martínez from comment #20) > Created attachment 369169 [details] [review] [review] > build: Improve dependency handling > > (In reply to Felix Riemann from comment #19) > > Review of attachment 368992 [details] [review] [review] [review]: > > - gtk+-unix-print appears to be actually unneeded. It seems to have been > > needed for GtkPrintUnixDialog. However, I can't where we ever needed it. > > Maybe it was because of some other symbol but headers changed over the years > > and eog can live without it now. > > This seems to be my mistake. `eog-print.c` uses symbols from > `gtk+-unix-print` so this shouldn't be removed. No, builds and runs fine if we drop the unused gtkunixprint include in eog-print-image-setup.c. :) commit bfe8884881fbe9eab5576063fae8ecd681866b84 Author: Felix Riemann <> Date: Sun Mar 4 20:09:20 2018 +0100 build: Remove dependency on gtk-unix-print gtk-unix-print doesn't seem to be needed by eog anymore. https://bugzilla.gnome.org/show_bug.cgi?id=793719
Review of attachment 369169 [details] [review]: ::: src/meson.build @@ +197,3 @@ ) +deps = [libeog_dep] That would leave us with an indirect dependency on glib for the eog binary as it relies on libeog pulling in glib and gtk. But the main.c has calls to glib and gtk as well.
Created attachment 369279 [details] [review] build: Improve dependency handling (In reply to Felix Riemann from comment #21) > No, builds and runs fine if we drop the unused gtkunixprint include in > eog-print-image-setup.c. :) Thanks! Looking at the docs I thought that it was necessary for those calls to `gtk_print_*` functions. It also build properly here, so I have updated the patch. (In reply to Felix Riemann from comment #22) > +deps = [libeog_dep] > > That would leave us with an indirect dependency on glib for the eog binary > as it relies on libeog pulling in glib and gtk. But the main.c has calls to > glib and gtk as well. Regardless of the libraries required by `main. c`, the libraries in the `libeog_dep` internal dependency are those that are required by the exported headers. @@ -164,7 +179,7 @@ libeog = shared_library( libeog_dep = declare_dependency( link_with: libeog, include_directories: src_inc, - dependencies: eog_deps + dependencies: common_deps ) This `common_deps` includes both `glib` and `gtk` dependencies. Does this make sense to you?
Review of attachment 369279 [details] [review]: (In reply to Iñigo Martínez from comment #23) > (In reply to Felix Riemann from comment #22) > > +deps = [libeog_dep] > > > > That would leave us with an indirect dependency on glib for the eog binary > > as it relies on libeog pulling in glib and gtk. But the main.c has calls to > > glib and gtk as well. > > Regardless of the libraries required by `main. c`, the libraries in the > `libeog_dep` internal dependency are those that are required by the exported > headers. > > @@ -164,7 +179,7 @@ libeog = shared_library( > libeog_dep = declare_dependency( > link_with: libeog, > include_directories: src_inc, > - dependencies: eog_deps > + dependencies: common_deps > ) > > This `common_deps` includes both `glib` and `gtk` dependencies. > Oh okay, I thought this would mean that main.c would link to glib/gtk only because libeog does.
Pushed the remaining two patches for the 3.27.91 release. Thanks, for taking care! :) --- This problem has been fixed in the unstable development version. The fix will be available in the next major software release. You may need to upgrade your Linux distribution to obtain that newer version.