After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 787346 - [Patch] Port to meson
[Patch] Port to meson
Status: RESOLVED FIXED
Product: gnome-chess
Classification: Applications
Component: General
git master
Other Linux
: Normal normal
: ---
Assigned To: gnome-chess-maint
gnome-chess-maint
Depends on:
Blocks: 782980
 
 
Reported: 2017-09-06 07:53 UTC by Niels De Graef
Modified: 2017-09-09 08:14 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[Patch] Port GNOME Chess to Meson (9.46 KB, patch)
2017-09-06 07:53 UTC, Niels De Graef
none Details | Review
[Patch] Port GNOME Chess to Meson (v2) (34.46 KB, patch)
2017-09-06 16:28 UTC, Niels De Graef
committed Details | Review

Description Niels De Graef 2017-09-06 07:53:35 UTC
Created attachment 359252 [details] [review]
[Patch] Port GNOME Chess to Meson

Meson [1] is a new but powerful build system that is quickly being embraced in the GNOME community [2]. The attached patch port GNOME Chess to Meson as well.


[1] https://mesonbuild.com/
[2] https://wiki.gnome.org/Initiatives/GnomeGoals/MesonPorting
Comment 1 Michael Catanzaro 2017-09-06 14:27:47 UTC
Review of attachment 359252 [details] [review]:

Awesome, thanks very much!

::: data/icons/meson.build
@@ +1,3 @@
+# The app icon, in different sizes
+iconsdir = join_paths(datadir, 'icons', 'hicolor')
+foreach size: ['16x16', '22x22', '24x24', '32x32', '48x48', '256x256', '512x512', 'scalable']

Let's take this opportunity to delete the legacy 22x22 icon:

https://developer.gnome.org/hig/stable/icons-and-artwork.html.en

::: data/meson.build
@@ +9,3 @@
+# The desktop file
+i18n.merge_file(
+  input: 'gnome-chess.desktop.in',

Let's also take this opportunity to rename the desktop file: org.gnome.Chess.desktop.in. It will need to be added to the rename list in gnome-shell's appFavorites.js.

@@ +14,3 @@
+  po_dir: po_dir,
+  install: true,
+  install_dir: join_paths(get_option('datadir'), 'applications')

Don't need get_option() because you have a datadir variable already.

@@ +19,3 @@
+# The appdata file
+i18n.merge_file(
+  input: 'gnome-chess.appdata.xml.in',

So this would become org.gnome.Chess.appdata.xml.in. Then there's a reference to the desktop file name inside that needs to be updated as well.

@@ +24,3 @@
+  po_dir: po_dir,
+  install: true,
+  install_dir: join_paths(get_option('datadir'), 'metainfo')

Ditto regarding datadir variable.

::: data/pieces/meson.build
@@ +23,3 @@
+  pieces_theme_dir = join_paths(pieces_dir, theme)
+
+  # Can't use install_subdir due to the Makefiles that are still there.

Please delete the Autotools build in the same patch and use install_subdir. We will branch gnome-3-26 before landing.

::: meson.build
@@ +29,3 @@
+min_glib_version = '2.44.0'
+min_gtk_version = '3.19.0'
+min_librsvg_version = '2.32.0'

We don't need min_gtk_version or min_librsvg_version variables, since they're used only once. Only the min_glib_version variable is useful, since we use three different pkg-config dependencies provided by GLib.

@@ +33,3 @@
+gio = dependency('gio-unix-2.0', version: '>=' + min_glib_version)
+glib = dependency('glib-2.0', version: '>=' + min_glib_version)
+gmodule = dependency('gmodule-2.0')

Since gmodule is part of GLib, I would check that it's >= min_glib_version here too. Of course it's guaranteed to be the same as the Gio and GLib versions, so this is just a matter of style.

@@ +38,3 @@
+
+posix = meson.get_compiler('vala').find_library('posix')
+prctl = meson.get_compiler('c').find_library('prctl', required: false)

I guess you've tested to ensure this works. :)

::: src/meson.build
@@ +1,3 @@
+# The resource file
+resource_files = files('chess.gresource.xml')
+resources = gnome.compile_resources('org.gnome.chess', resource_files,

How hard would it be to rename it to org.gnome.Chess?
Comment 2 Niels De Graef 2017-09-06 16:28:13 UTC
Created attachment 359285 [details] [review]
[Patch] Port GNOME Chess to Meson (v2)

New patch. Autotools are now removed.

(In reply to Michael Catanzaro from comment #1)
> Review of attachment 359252 [details] [review] [review]:
> 
> Awesome, thanks very much!
> 
> ::: data/icons/meson.build
> @@ +1,3 @@
> +# The app icon, in different sizes
> +iconsdir = join_paths(datadir, 'icons', 'hicolor')
> +foreach size: ['16x16', '22x22', '24x24', '32x32', '48x48', '256x256',
> '512x512', 'scalable']
> 
> Let's take this opportunity to delete the legacy 22x22 icon:
> https://developer.gnome.org/hig/stable/icons-and-artwork.html.en
Done!

> ::: data/meson.build
> @@ +9,3 @@
> +# The desktop file
> +i18n.merge_file(
> +  input: 'gnome-chess.desktop.in',
> 
> Let's also take this opportunity to rename the desktop file:
> org.gnome.Chess.desktop.in. It will need to be added to the rename list in
> gnome-shell's appFavorites.js.
Tbh, I'd much more prefer it if this is in a separate patch/commit (since the current one is already getting quite large and it is a distinct feature).
 
> @@ +14,3 @@
> +  po_dir: po_dir,
> +  install: true,
> +  install_dir: join_paths(get_option('datadir'), 'applications')
> 
> Don't need get_option() because you have a datadir variable already.
Well spotted, fixed!

> @@ +19,3 @@
> +# The appdata file
> +i18n.merge_file(
> +  input: 'gnome-chess.appdata.xml.in',
> 
> So this would become org.gnome.Chess.appdata.xml.in. Then there's a
> reference to the desktop file name inside that needs to be updated as well.
Same as comment on desktop file.

> @@ +24,3 @@
> +  po_dir: po_dir,
> +  install: true,
> +  install_dir: join_paths(get_option('datadir'), 'metainfo')
> 
> Ditto regarding datadir variable.
Fixed!

> ::: data/pieces/meson.build
> @@ +23,3 @@
> +  pieces_theme_dir = join_paths(pieces_dir, theme)
> +
> +  # Can't use install_subdir due to the Makefiles that are still there.
> 
> Please delete the Autotools build in the same patch and use install_subdir.
> We will branch gnome-3-26 before landing.
Fixed! However, this does mean that the COPYING.html gets installed as well. Wanted behaviour or not?

> ::: meson.build
> @@ +29,3 @@
> +min_glib_version = '2.44.0'
> +min_gtk_version = '3.19.0'
> +min_librsvg_version = '2.32.0'
> 
> We don't need min_gtk_version or min_librsvg_version variables, since
> they're used only once. Only the min_glib_version variable is useful, since
> we use three different pkg-config dependencies provided by GLib.
> 
> @@ +33,3 @@
> +gio = dependency('gio-unix-2.0', version: '>=' + min_glib_version)
> +glib = dependency('glib-2.0', version: '>=' + min_glib_version)
> +gmodule = dependency('gmodule-2.0')
> 
> Since gmodule is part of GLib, I would check that it's >= min_glib_version
> here too. Of course it's guaranteed to be the same as the Gio and GLib
> versions, so this is just a matter of style.
All fixed!

> @@ +38,3 @@
> +
> +posix = meson.get_compiler('vala').find_library('posix')
> +prctl = meson.get_compiler('c').find_library('prctl', required: false)
> 
> I guess you've tested to ensure this works. :)
Well, I thought I did, but apparently `has_header()` was what I needed. :-) Fixed!

> ::: src/meson.build
> @@ +1,3 @@
> +# The resource file
> +resource_files = files('chess.gresource.xml')
> +resources = gnome.compile_resources('org.gnome.chess', resource_files,
> 
> How hard would it be to rename it to org.gnome.Chess?
Not too hard, but same comment as on the other app-id renames.


Lastly, I thought I might mention that the current application id is "org.gnome.chess", not "org.gnome.Chess" (with a capital C)
Comment 3 Michael Catanzaro 2017-09-06 16:47:34 UTC
Review of attachment 359285 [details] [review]:

It's a shame we're not more consistent about app IDs. :/ I prefer to capitalize the app name.
Comment 4 Michael Catanzaro 2017-09-09 04:10:19 UTC
It works for me. \o/
Comment 5 Niels De Graef 2017-09-09 08:14:27 UTC
(In reply to Michael Catanzaro from comment #4)
> It works for me. \o/
:-)


If you don't mind, I just made a trivial commit to correct the version (since it's already 3.26.0 now)