GNOME Bugzilla – Bug 787346
[Patch] Port to meson
Last modified: 2017-09-09 08:14:27 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
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?
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)
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.
It works for me. \o/
(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)