GNOME Bugzilla – Bug 783205
Port to meson build system
Last modified: 2017-06-25 17:19:46 UTC
Created attachment 352802 [details] [review] Port to meson build system Complete port to meson build system. Although I have been testing it, some more testing will be very appreciated. I have tested it on my computer which has only Linux installed. Any suggestion is also welcome,
Review of attachment 352802 [details] [review]: Nice work! This will take a while, but here's a rough initial review... :-) ::: .gitmodules @@ +2,3 @@ path = libgd url = git://git.gnome.org/libgd +[submodule "subprojects/libgd"] I'd probably modify the Autotools build to pick libgd from subprojects/ as well. ::: data/appdata/meson.build @@ +1,3 @@ +appdata = 'org.gnome.Totem.appdata.xml' + +custom_target( It would be great to drop intltool for the appdata XML; this would become: i18n = import('i18n') i18n.merge_file('desktop', input: 'org.gnome.Totem.appdata.xml.in', output: 'org.gnome.Totem.appdata.xml', po_dir: join_paths(meson.source_root(), 'po'), install: true, install_dir: join_paths(totem_datadir, 'metainfo')) ::: data/meson.build @@ +6,3 @@ +data_dir = join_paths(meson.source_root(), 'data') + +desktop_sh = join_paths(data_dir, 'desktop.sh') This is not a good idea; you should use `files()` instead, to reference files inside the source directory. In this case, though, you want: desktop_sh = find_program('desktop.sh') thumbnailer_sh = find_program('thumbnailer.sh') ... @@ +67,3 @@ +) + +custom_target( Like appdata, you should use i18n.merge_file() for the desktop file localization: i18n.merge_file('desktop', type: 'desktop', input: 'org.gnome.Totem.desktop.in', output: 'org.gnome.Totem.desktop', po_dir: join_paths(meson.source_root(), 'po'), install: true, install_dir: join_paths(totem_datadir, 'applications')) @@ +149,3 @@ + +enum_headers = [ + join_paths(src_dir, 'icon-helpers.h'), Please, use files() instead, e.g. files('../src/icon-helpers.h'), files('../src/totem-grilo.h'), ... @@ +178,3 @@ +] + +custom_target( This should be: gnome = import('gnome') gnome.mkenums(...) It should also go into the source directory, not the data directory. @@ +216,3 @@ + +''' +# FIXME: should be removed Not really. The totem pkg-config file is a bit too complicated for the Meson pkgconfig module. @@ +232,3 @@ + +configure_file( + input: '@0@.in'.format(pc), This could simply be: input: pc + '.in' output: pc without using the format() method. ::: data/org.gnome.Totem.desktop.meson @@ +1,1 @@ +[Desktop Entry] You don't really need a whole new file. You can reuse the existing one. Additionally, if you modify the Autotools build, you can drop intltool. ::: data/totem-video-thumbnailer.1 @@ -1,1 @@ -.\" Automatically generated by Pod::Man 2.16 (Pod::Simple 3.07) There are a bunch of changes, here. They should go in a separate commit. ::: data/totem.thumbnailer.meson @@ +1,1 @@ +[Thumbnailer Entry] Same as the desktop file: you don't need a whole new file for this. ::: docs/reference/meson.build @@ +73,3 @@ + +version_xml = configure_file( + input: '@0@.in'.format(version), I would simply use string concatenation. @@ +94,3 @@ + '--ignore-headers=' + ' '.join(private_headers), + ], + mkdb_args: [ Currently, gnome.gtkdoc() does not support `mkdb_args`, but I filed a patch to enable it in 0.41.0. ::: help/meson.build @@ +5,3 @@ + +media = [ + join_paths('figures', 'totem_show_playlist_button.png'), Please, use: files('figures/totem_show_playlist_button.png'), ... You also don't need to use join_paths() in many cases: Meson will convert paths with the appropriate directory separator. ::: meson.build @@ +240,3 @@ +top_inc = include_directories('.') + +if get_option('enable-nls') There is no earthly reason why internationalization should ever be disabled. ::: src/meson.build @@ +6,3 @@ + '-export-dynamic', + '-no-undefined' +] These are really libtool flags, not linker flags. You don't need them. @@ +21,3 @@ + '-avoid-version', + '-module' +] Same as above. Both are automatically provided when building modules using `shared_module()`. @@ +59,3 @@ + '-DDBUS_API_SUBJECT_TO_CHANGE', + '-DGNOMELOCALEDIR="@0@"'.format(totem_localedir), + '-DDATADIR="@0@"'.format(totem_datadir), Indendation. @@ +68,3 @@ +headers = [ + join_paths('plugins', 'totem-plugin.h'), + join_paths('plugins', 'totem-dirs.h'), Use `plugins/totem-plugin.h` and `plugins/totem-dirs.h`. @@ +79,3 @@ + +libtotem_player_headers = [ + 'totem-time-label.h' No need to have headers listed. @@ +108,3 @@ + +libtotem_headers = [ + join_paths('plugins', 'totem-plugins-engine.h'), Use `plugins/totem-plugins-engine.h`. @@ +128,3 @@ +libtotem_sources = [ + join_paths('plugins', 'totem-dirs.c'), + join_paths('plugins', 'totem-plugins-engine.c'), Use `plugins/totem-dirs.c` and `plugins/totem-plugins-engine.c`. There's no need for `join_paths()`. @@ +162,3 @@ +libtotem = shared_library( + 'totem', + sources: libtotem_sources + libtotem_headers + headers + gen_sources, There's no need to add the headers here. The build will depend on them through the source files. @@ +182,3 @@ + +# FIXME: variables are not applied +pkg.generate( Use a configure_file() instead. @@ +215,3 @@ +totem_video_thumbnailer_sources = [ + 'totem-resources.c', + 'totem-resources.h', No need to list the header. @@ +246,3 @@ + 'totem-properties-main.c', + 'totem-properties-view.c', + 'totem-properties-view.h' No need to list the header. @@ +257,3 @@ + ] + + libtotem_properties_page = library( This is a shared_module(). @@ +293,3 @@ +test_icons_sources = [ + 'icon-helpers.c', + 'icon-helpers.h', No need to lead headers. ::: src/plugins/apple-trailers/meson.build @@ +3,3 @@ +plugin_dir = join_paths(plugins_dir, plugin_name) + +library( This is a shared_module(). ::: src/plugins/autoload-subtitles/meson.build @@ +3,3 @@ +plugin_dir = join_paths(plugins_dir, plugin_name) + +library( This is a shared_module(). ::: src/plugins/brasero-disc-recorder/meson.build @@ +3,3 @@ +plugin_dir = join_paths(plugins_dir, plugin_name) + +library( This is a shared_module(). ::: src/plugins/gromit/meson.build @@ +3,3 @@ +plugin_dir = join_paths(plugins_dir, plugin_name) + +library( This is a shared_module() ::: src/plugins/im-status/meson.build @@ +3,3 @@ +plugin_dir = join_paths(plugins_dir, 'im-status') + +library( This is a shared_module(). ::: src/plugins/lirc/meson.build @@ +3,3 @@ +plugin_dir = join_paths(plugins_dir, plugin_name) + +library( This is a shared_module(). ::: src/plugins/media-player-keys/meson.build @@ +3,3 @@ +plugin_dir = join_paths(plugins_dir, plugin_name) + +library( This is a shared_module(). ::: src/plugins/meson.build @@ +10,3 @@ +] + +plugins_ldflags = totem_common_ext_ldflags This is not necessary. ::: src/plugins/ontop/meson.build @@ +3,3 @@ +plugin_dir = join_paths(plugins_dir, plugin_name) + +library( This is a shared_module(). ::: src/plugins/properties/meson.build @@ +3,3 @@ +plugin_dir = join_paths(plugins_dir, 'properties') + +library( This is a shared_module. ::: src/plugins/recent/meson.build @@ +3,3 @@ +plugin_dir = join_paths(plugins_dir, plugin_name) + +library( This is a shared_module(). ::: src/plugins/rotation/meson.build @@ +15,3 @@ +] + +library( This is a shared_module(). ::: src/plugins/sample-vala/meson.build @@ +10,3 @@ +] + +library( This is a shared_module(). ::: src/plugins/save-file/meson.build @@ +3,3 @@ +plugin_dir = join_paths(plugins_dir, plugin_name) + +library( This is a shared_module(). ::: src/plugins/screensaver/meson.build @@ +3,3 @@ +plugin_dir = join_paths(plugins_dir, plugin_name) + +library( This is a shared_module(). ::: src/plugins/screenshot/meson.build @@ +27,3 @@ +endforeach + +library( This is a shared_module(). ::: src/plugins/skipto/meson.build @@ +11,3 @@ +] + +library( This is a shared_module(). ::: src/plugins/variable-rate/meson.build @@ +3,3 @@ +plugin_dir = join_paths(plugins_dir, plugin_name) + +library( This is a shared_module(). ::: src/plugins/vimeo/meson.build @@ +3,3 @@ +plugin_dir = join_paths(plugins_dir, plugin_name) + +library( This is a shared_module(). ::: src/plugins/zeitgeist-dp/meson.build @@ +15,3 @@ +] + +library( This is a shared_module().
Created attachment 352990 [details] [review] Port to meson build system Thank you very much for the review. I have updated the patch with your useful hints. You can find some comments below: (In reply to Emmanuele Bassi (:ebassi) from comment #1) > Review of attachment 352802 [details] [review] [review]: > > Nice work! This will take a while, but here's a rough initial review... :-) > > ::: .gitmodules > @@ +2,3 @@ > path = libgd > url = git://git.gnome.org/libgd > +[submodule "subprojects/libgd"] > > I'd probably modify the Autotools build to pick libgd from subprojects/ as > well. Done! Although the libgd version used in the meson port is an updated version where they have also implemented meson, I haven't tested it enough to ensure that it doesn't break anything else. > ::: data/appdata/meson.build > @@ +1,3 @@ > +appdata = 'org.gnome.Totem.appdata.xml' > + > +custom_target( > > It would be great to drop intltool for the appdata XML; this would become: > > i18n = import('i18n') > i18n.merge_file('desktop', > input: 'org.gnome.Totem.appdata.xml.in', > output: 'org.gnome.Totem.appdata.xml', > po_dir: join_paths(meson.source_root(), 'po'), > install: true, > install_dir: join_paths(totem_datadir, 'metainfo')) > Done! I have migrated intltool to an updated gettext version. I have already submitted the patch[0]. > ::: data/meson.build > @@ +6,3 @@ > +data_dir = join_paths(meson.source_root(), 'data') > + > +desktop_sh = join_paths(data_dir, 'desktop.sh') > > This is not a good idea; you should use `files()` instead, to reference > files inside the source directory. > > In this case, though, you want: > > desktop_sh = find_program('desktop.sh') > thumbnailer_sh = find_program('thumbnailer.sh') > ... Done! > @@ +67,3 @@ > +) > + > +custom_target( > > Like appdata, you should use i18n.merge_file() for the desktop file > localization: > > i18n.merge_file('desktop', > type: 'desktop', > input: 'org.gnome.Totem.desktop.in', > output: 'org.gnome.Totem.desktop', > po_dir: join_paths(meson.source_root(), 'po'), > install: true, > install_dir: join_paths(totem_datadir, 'applications')) Done![0] > @@ +149,3 @@ > + > +enum_headers = [ > + join_paths(src_dir, 'icon-helpers.h'), > > Please, use files() instead, e.g. > > files('../src/icon-helpers.h'), > files('../src/totem-grilo.h'), > ... > > @@ +178,3 @@ > +] > + > +custom_target( > > This should be: > > gnome = import('gnome') > gnome.mkenums(...) > > It should also go into the source directory, not the data directory. Done! > @@ +216,3 @@ > + > +''' > +# FIXME: should be removed > > Not really. > > The totem pkg-config file is a bit too complicated for the Meson pkgconfig > module. I tried to use to the meson pkgconfig module as a better integration for the port. I have moved back to the configure file version. > @@ +232,3 @@ > + > +configure_file( > + input: '@0@.in'.format(pc), > > This could simply be: > > input: pc + '.in' > output: pc > > without using the format() method. Done! > ::: data/org.gnome.Totem.desktop.meson > @@ +1,1 @@ > +[Desktop Entry] > > You don't really need a whole new file. You can reuse the existing one. I had a problem here because I still haven't figured out how to solve it without modifying how those scripts work. A way of doing this could be to use a custom target with the capture option on, but that option truncates the file losing its data. I thought that a new meson file could be the cleanest way of doing so. > Additionally, if you modify the Autotools build, you can drop intltool. > > ::: data/totem-video-thumbnailer.1 > @@ -1,1 @@ > -.\" Automatically generated by Pod::Man 2.16 (Pod::Simple 3.07) > > There are a bunch of changes, here. They should go in a separate commit. I added this file by mistake. There is a command which builds this file from a .pod file everytime. Maybe it would be better to disable this command. > ::: data/totem.thumbnailer.meson > @@ +1,1 @@ > +[Thumbnailer Entry] > > Same as the desktop file: you don't need a whole new file for this. > > ::: docs/reference/meson.build > @@ +73,3 @@ > + > +version_xml = configure_file( > + input: '@0@.in'.format(version), > > I would simply use string concatenation. > > @@ +94,3 @@ > + '--ignore-headers=' + ' '.join(private_headers), > + ], > + mkdb_args: [ > > Currently, gnome.gtkdoc() does not support `mkdb_args`, but I filed a patch > to enable it in 0.41.0. Nice! I left the option as it doesn't cause any warning/error until it is supported. > ::: help/meson.build > @@ +5,3 @@ > + > +media = [ > + join_paths('figures', 'totem_show_playlist_button.png'), > > Please, use: > > files('figures/totem_show_playlist_button.png'), > ... > > You also don't need to use join_paths() in many cases: Meson will convert > paths with the appropriate directory separator. I tried to use join_paths as much as possible as I thought that this could be the way for multiplatform support. Regarding the media images, files is not applicable here because they are not real files; they are the files to point at on language directories. > ::: meson.build > @@ +240,3 @@ > +top_inc = include_directories('.') > + > +if get_option('enable-nls') > > There is no earthly reason why internationalization should ever be disabled. Removed! > ::: src/meson.build > @@ +6,3 @@ > + '-export-dynamic', > + '-no-undefined' > +] > > These are really libtool flags, not linker flags. You don't need them. > > @@ +21,3 @@ > + '-avoid-version', > + '-module' > +] > > Same as above. Both are automatically provided when building modules using > `shared_module()`. Done! > @@ +59,3 @@ > + '-DDBUS_API_SUBJECT_TO_CHANGE', > + '-DGNOMELOCALEDIR="@0@"'.format(totem_localedir), > + '-DDATADIR="@0@"'.format(totem_datadir), > > Indendation. My fault, I missed these tabs. It's fixed now. > @@ +68,3 @@ > +headers = [ > + join_paths('plugins', 'totem-plugin.h'), > + join_paths('plugins', 'totem-dirs.h'), > > Use `plugins/totem-plugin.h` and `plugins/totem-dirs.h`. > > @@ +79,3 @@ > + > +libtotem_player_headers = [ > + 'totem-time-label.h' > > No need to have headers listed. > > @@ +108,3 @@ > + > +libtotem_headers = [ > + join_paths('plugins', 'totem-plugins-engine.h'), > > Use `plugins/totem-plugins-engine.h`. > > @@ +128,3 @@ > +libtotem_sources = [ > + join_paths('plugins', 'totem-dirs.c'), > + join_paths('plugins', 'totem-plugins-engine.c'), > > Use `plugins/totem-dirs.c` and `plugins/totem-plugins-engine.c`. There's no > need for `join_paths()`. > > @@ +162,3 @@ > +libtotem = shared_library( > + 'totem', > + sources: libtotem_sources + libtotem_headers + headers + gen_sources, > > There's no need to add the headers here. The build will depend on them > through the source files. Removed all the join_paths and headers. > @@ +182,3 @@ > + > +# FIXME: variables are not applied > +pkg.generate( > > Use a configure_file() instead. Done! > @@ +215,3 @@ > +totem_video_thumbnailer_sources = [ > + 'totem-resources.c', > + 'totem-resources.h', > > No need to list the header. > > @@ +246,3 @@ > + 'totem-properties-main.c', > + 'totem-properties-view.c', > + 'totem-properties-view.h' > > No need to list the header. Removed all the headers. > @@ +257,3 @@ > + ] > + > + libtotem_properties_page = library( > > This is a shared_module(). > > @@ +293,3 @@ > +test_icons_sources = [ > + 'icon-helpers.c', > + 'icon-helpers.h', > > No need to lead headers. > > ::: src/plugins/apple-trailers/meson.build > @@ +3,3 @@ > +plugin_dir = join_paths(plugins_dir, plugin_name) > + > +library( > > This is a shared_module(). > > ::: src/plugins/autoload-subtitles/meson.build > @@ +3,3 @@ > +plugin_dir = join_paths(plugins_dir, plugin_name) > + > +library( > > This is a shared_module(). > > ::: src/plugins/brasero-disc-recorder/meson.build > @@ +3,3 @@ > +plugin_dir = join_paths(plugins_dir, plugin_name) > + > +library( > > This is a shared_module(). > > ::: src/plugins/gromit/meson.build > @@ +3,3 @@ > +plugin_dir = join_paths(plugins_dir, plugin_name) > + > +library( > > This is a shared_module() > > ::: src/plugins/im-status/meson.build > @@ +3,3 @@ > +plugin_dir = join_paths(plugins_dir, 'im-status') > + > +library( > > This is a shared_module(). > > ::: src/plugins/lirc/meson.build > @@ +3,3 @@ > +plugin_dir = join_paths(plugins_dir, plugin_name) > + > +library( > > This is a shared_module(). > > ::: src/plugins/media-player-keys/meson.build > @@ +3,3 @@ > +plugin_dir = join_paths(plugins_dir, plugin_name) > + > +library( > > This is a shared_module(). > > ::: src/plugins/meson.build > @@ +10,3 @@ > +] > + > +plugins_ldflags = totem_common_ext_ldflags > > This is not necessary. > > ::: src/plugins/ontop/meson.build > @@ +3,3 @@ > +plugin_dir = join_paths(plugins_dir, plugin_name) > + > +library( > > This is a shared_module(). > > ::: src/plugins/properties/meson.build > @@ +3,3 @@ > +plugin_dir = join_paths(plugins_dir, 'properties') > + > +library( > > This is a shared_module. > > ::: src/plugins/recent/meson.build > @@ +3,3 @@ > +plugin_dir = join_paths(plugins_dir, plugin_name) > + > +library( > > This is a shared_module(). > > ::: src/plugins/rotation/meson.build > @@ +15,3 @@ > +] > + > +library( > > This is a shared_module(). > > ::: src/plugins/sample-vala/meson.build > @@ +10,3 @@ > +] > + > +library( > > This is a shared_module(). > > ::: src/plugins/save-file/meson.build > @@ +3,3 @@ > +plugin_dir = join_paths(plugins_dir, plugin_name) > + > +library( > > This is a shared_module(). > > ::: src/plugins/screensaver/meson.build > @@ +3,3 @@ > +plugin_dir = join_paths(plugins_dir, plugin_name) > + > +library( > > This is a shared_module(). > > ::: src/plugins/screenshot/meson.build > @@ +27,3 @@ > +endforeach > + > +library( > > This is a shared_module(). > > ::: src/plugins/skipto/meson.build > @@ +11,3 @@ > +] > + > +library( > > This is a shared_module(). > > ::: src/plugins/variable-rate/meson.build > @@ +3,3 @@ > +plugin_dir = join_paths(plugins_dir, plugin_name) > + > +library( > > This is a shared_module(). > > ::: src/plugins/vimeo/meson.build > @@ +3,3 @@ > +plugin_dir = join_paths(plugins_dir, plugin_name) > + > +library( > > This is a shared_module(). > > ::: src/plugins/zeitgeist-dp/meson.build > @@ +15,3 @@ > +] > + > +library( > > This is a shared_module(). Done! [0] https://bugzilla.gnome.org/show_bug.cgi?id=783316
Created attachment 352991 [details] [review] Updated autotools to use libgd module at subproject directory
Created attachment 353511 [details] [review] Port to meson build system Given the situation with gettext patch, which will probably be sometime until it's merged and published, I have removed its support and I have gone back to intltool commands. I have also done some changes and improvements. One of those changes is that I was using two new files as templates for enum source files generations, but I have also gone back to the commands used by autotools, though, I still think that the templates are the way to go as it's cleaner approach. The 352991 patch, that updates autotools to use the same libgd module as meson at subproject directory, is still applicable.
Created attachment 353513 [details] [review] Migrated meson to gettext This patch migrates meson to use gettext instead of intltool, in case of plugin support being merged in gettext[0] or finding a similar/better approach. Translation files have also been changed to be supported by gettext (desktop and plugin files). [0] http://lists.gnu.org/archive/html/bug-gettext/2017-06/msg00002.html
Review of attachment 353513 [details] [review]: Please merge all 3 patches. The autotools changes can be made separately, after the fact, though we'll likely remove all of it once the meson build works. ::: src/plugins/apple-trailers/meson.build @@ +18,2 @@ plugin_data, + type: 'plugin', Which version of meson is needed for this to work? Please update the top-level meson.build to reflect that dependency. Meson encountered an error in file src/plugins/apple-trailers/meson.build, line 17, column 5: i18n: "plugin" is not a valid type ('xml', 'desktop')
Review of attachment 352991 [details] [review]: ::: .gitmodules @@ -4,3 @@ [submodule "subprojects/libgd"] path = subprojects/libgd url = https://git.gnome.org/browse/libgd With a clean repo, and running "git submodule update --init --recursive" didn't pull anything.
(In reply to Bastien Nocera from comment #6) > Review of attachment 353513 [details] [review] [review]: > > Please merge all 3 patches. The autotools changes can be made separately, > after the fact, though we'll likely remove all of it once the meson build > works. > > ::: src/plugins/apple-trailers/meson.build > @@ +18,2 @@ > plugin_data, > + type: 'plugin', > > Which version of meson is needed for this to work? Please update the > top-level meson.build to reflect that dependency. > > Meson encountered an error in file src/plugins/apple-trailers/meson.build, > line 17, column 5: > i18n: "plugin" is not a valid type ('xml', 'desktop') meson-0.40.1-1.fc26.noarch fwiw
The 'plugin' type is not part of Meson master — i.e. what will be released as 0.41.0. I think attachment 353513 [details] [review] is not meant to be merged.
Review of attachment 353511 [details] [review]: ::: .gitmodules @@ +2,3 @@ path = libgd url = git://git.gnome.org/libgd +[submodule "subprojects/libgd"] You're still checking out libgd twice. ::: data/appdata/meson.build @@ +7,3 @@ + command: intltool_xml_cmd, + install: true, + install_dir: join_paths(totem_datadir, 'appdata') Side note: the new, canonical location for AppStream XML is `$datadir/metainfo`. The appdata.xml file should also be renamed to `org.gnome.Totem.metainfo.xml`, but that's not strictly necessary. ::: data/meson.build @@ +13,3 @@ +man_thumbnailer = 'totem-video-thumbnailer' + +# FIXME: This causes the file to be modified every time. You should use a custom target, instead. ``` thumbnailer_name = 'totem-video-thumbnailer' pod2man = find_program('pod2man') pod2man_opts = [ '-c', '""', '-s', '1', '-q', 'none', '-n', thumgnailer_name, '-r', 'GNOME', ] custom_target( 'totem-video-thumbnailer.1', input: thumbnailer_name + '.pod', output: thumbnailer_name + '.1', command: [ pod2man, pod2man_opts, '@INPUT@', '@OUTPUT@', ], install: true, install_dir: join_paths(totem_mandir, 'man1') ) ``` Extra points for using: ``` pod2man = find_program('pod2man', required: false) if pod2man.found() pod2man_opts = [ ... ] custom_target(... else message('Man page generation disabled') endif ``` Which allows us to build Totem in Flatpak runtimes without pod2man.
Created attachment 353602 [details] [review] Port to meson build system (In reply to Bastien Nocera from comment #6) > ::: src/plugins/apple-trailers/meson.build > @@ +18,2 @@ > plugin_data, > + type: 'plugin', > > Which version of meson is needed for this to work? Please update the > top-level meson.build to reflect that dependency. > > Meson encountered an error in file src/plugins/apple-trailers/meson.build, > line 17, column 5: > i18n: "plugin" is not a valid type ('xml', 'desktop') (In reply to Emmanuele Bassi (:ebassi) from comment #9) > The 'plugin' type is not part of Meson master — i.e. what will be released > as 0.41.0. > > I think attachment 353513 [details] [review] [review] is not meant to be merged. True, the last patch is not valid as it is right now. It relies on the gettext patch that uses a new "input file language" used to support plugin files. I missed to say that it will also need a modified version of meson to support this language ('plugin') since meson's i18n module, only supports 'xml' (by default) and 'desktop' languages. It was created just in case all of this happens, so at the moment, this patch should be avoided. (In reply to Emmanuele Bassi (:ebassi) from comment #10) > Review of attachment 353511 [details] [review] [review]: > > ::: .gitmodules > @@ +2,3 @@ > path = libgd > url = git://git.gnome.org/libgd > +[submodule "subprojects/libgd"] > > You're still checking out libgd twice. It was already done in a separated patch[0], though I have squashed it in the same patch. > ::: data/appdata/meson.build > @@ +7,3 @@ > + command: intltool_xml_cmd, > + install: true, > + install_dir: join_paths(totem_datadir, 'appdata') > > Side note: the new, canonical location for AppStream XML is > `$datadir/metainfo`. The appdata.xml file should also be renamed to > `org.gnome.Totem.metainfo.xml`, but that's not strictly necessary. Although I changed the code to install the resulting file at `$datadir/metainfo` as you pointed me out[1], I wasn't sure if this relied on the proper translation by using the i18n module, so I moved back to `$datadir/appdata` as autotools does. Although it is not strictly necessary, if its better/future proof, I could change it. > ::: data/meson.build > @@ +13,3 @@ > +man_thumbnailer = 'totem-video-thumbnailer' > + > +# FIXME: This causes the file to be modified every time. > > You should use a custom target, instead. > > ``` > thumbnailer_name = 'totem-video-thumbnailer' > > pod2man = find_program('pod2man') > pod2man_opts = [ > '-c', '""', > '-s', '1', > '-q', 'none', > '-n', thumgnailer_name, > '-r', 'GNOME', > ] > custom_target( > 'totem-video-thumbnailer.1', > input: thumbnailer_name + '.pod', > output: thumbnailer_name + '.1', > command: [ > pod2man, pod2man_opts, '@INPUT@', '@OUTPUT@', > ], > install: true, > install_dir: join_paths(totem_mandir, 'man1') > ) > ``` > > Extra points for using: > > ``` > pod2man = find_program('pod2man', required: false) > if pod2man.found() > pod2man_opts = [ > ... > ] > custom_target(... > else > message('Man page generation disabled') > endif > ``` > > Which allows us to build Totem in Flatpak runtimes without pod2man. Despite the fact that I wasn't checking properly if pod2man was found (I've fixed it now), I wasn't using custom_target because, iirc, it left the file uncompressed while the totem.1, which was installed using install_man, was compressed. install_man looked the proper way for installing man files with the same consistent result. If only I could use the output of custom_target as an input for install_man... but as far as I know, it doesn't work (not even with full_patch custom target returning object method). The downside is what it's stated in the comment, it rewrites the 'totem-video-thumbnailer.1' file which is controlled by git. I have fixed it by using my original approach and using the build directory in the output file. (In reply to Bastien Nocera from comment #6) > Review of attachment 353513 [details] [review] [review]: > > Please merge all 3 patches. The autotools changes can be made separately, > after the fact, though we'll likely remove all of it once the meson build > works. Would you like me to merge the patch in the master branch then? [0] https://bugzilla.gnome.org/attachment.cgi?id=352991 [1] https://bugzilla.gnome.org/show_bug.cgi?id=783205#c1
(In reply to Iñigo Martínez from comment #11) <snip> > Would you like me to merge the patch in the master branch then? No, not in their current states. This means "make the 3 patches one patch". Squash them.
(In reply to Iñigo Martínez from comment #11) > Created attachment 353602 [details] [review] [review] > Port to meson build system > > (In reply to Bastien Nocera from comment #6) > > ::: src/plugins/apple-trailers/meson.build > > @@ +18,2 @@ > > plugin_data, > > + type: 'plugin', > > > > Which version of meson is needed for this to work? Please update the > > top-level meson.build to reflect that dependency. > > > > Meson encountered an error in file src/plugins/apple-trailers/meson.build, > > line 17, column 5: > > i18n: "plugin" is not a valid type ('xml', 'desktop') > > (In reply to Emmanuele Bassi (:ebassi) from comment #9) > > The 'plugin' type is not part of Meson master — i.e. what will be released > > as 0.41.0. > > > > I think attachment 353513 [details] [review] [review] [review] is not meant to be merged. > > True, the last patch is not valid as it is right now. It relies on the > gettext patch that uses a new "input file language" used to support plugin > files. > > I missed to say that it will also need a modified version of meson to > support this language ('plugin') since meson's i18n module, only supports > 'xml' (by default) and 'desktop' languages. > > It was created just in case all of this happens, so at the moment, this > patch should be avoided. > > (In reply to Emmanuele Bassi (:ebassi) from comment #10) > > Review of attachment 353511 [details] [review] [review] [review]: > > > > ::: .gitmodules > > @@ +2,3 @@ > > path = libgd > > url = git://git.gnome.org/libgd > > +[submodule "subprojects/libgd"] > > > > You're still checking out libgd twice. > > It was already done in a separated patch[0], though I have squashed it in > the same patch. > > > ::: data/appdata/meson.build > > @@ +7,3 @@ > > + command: intltool_xml_cmd, > > + install: true, > > + install_dir: join_paths(totem_datadir, 'appdata') > > > > Side note: the new, canonical location for AppStream XML is > > `$datadir/metainfo`. The appdata.xml file should also be renamed to > > `org.gnome.Totem.metainfo.xml`, but that's not strictly necessary. > > Although I changed the code to install the resulting file at > `$datadir/metainfo` as you pointed me out[1], I wasn't sure if this relied > on the proper translation by using the i18n module, so I moved back to > `$datadir/appdata` as autotools does. Although it is not strictly necessary, > if its better/future proof, I could change it. Most tools will keep looking for AppStream files at $datadir/appdata and support appdata.xml files for a while; GNOME Software and Gettext already support $datadir/metainfo and metainfo.xml. > > ::: data/meson.build > > @@ +13,3 @@ > > +man_thumbnailer = 'totem-video-thumbnailer' > > + > > +# FIXME: This causes the file to be modified every time. > > > > You should use a custom target, instead. > > > > ``` > > thumbnailer_name = 'totem-video-thumbnailer' > > > > pod2man = find_program('pod2man') > > pod2man_opts = [ > > '-c', '""', > > '-s', '1', > > '-q', 'none', > > '-n', thumgnailer_name, > > '-r', 'GNOME', > > ] > > custom_target( > > 'totem-video-thumbnailer.1', > > input: thumbnailer_name + '.pod', > > output: thumbnailer_name + '.1', > > command: [ > > pod2man, pod2man_opts, '@INPUT@', '@OUTPUT@', > > ], > > install: true, > > install_dir: join_paths(totem_mandir, 'man1') > > ) > > ``` > > > > Extra points for using: > > > > ``` > > pod2man = find_program('pod2man', required: false) > > if pod2man.found() > > pod2man_opts = [ > > ... > > ] > > custom_target(... > > else > > message('Man page generation disabled') > > endif > > ``` > > > > Which allows us to build Totem in Flatpak runtimes without pod2man. > > Despite the fact that I wasn't checking properly if pod2man was found (I've > fixed it now), I wasn't using custom_target because, iirc, it left the file > uncompressed while the totem.1, which was installed using install_man, was > compressed. > I have fixed it by using my original approach and using the build directory > in the output file. No. Don't do that. Seriously: use custom_target() to generate the man page out of the POD file at install time. Using run_target() at build time on a file that is not needed for the rest of the build just makes the build slower for no reason. Compressing man pages is something better left to distributions that also enable libz support in man; packagers will still zip the man pages if found uncompressed.
Comment on attachment 353513 [details] [review] Migrated meson to gettext Not useful until gettext and meson are patched.
Created attachment 353603 [details] [review] Port to meson build system > Most tools will keep looking for AppStream files at $datadir/appdata and > support appdata.xml files for a while; GNOME Software and Gettext already > support $datadir/metainfo and metainfo.xml. I have updated this rename the file to `org.gnome.Totem.metainfo.xml` and install it into $datadir/metainfo, so I suppose that it is "future proof" now. > No. Don't do that. > > Seriously: use custom_target() to generate the man page out of the POD file > at install time. Using run_target() at build time on a file that is not > needed for the rest of the build just makes the build slower for no reason. > > Compressing man pages is something better left to distributions that also > enable libz support in man; packagers will still zip the man pages if found > uncompressed. Ok, I have changed it to use custom_target now. totem.1 file is also installed uncompressed.
Created attachment 353663 [details] [review] Port to meson build system meson 0.41.0 was released yesterday[0]. Among other interesting features, it adds support for custom variables in Pkgconfig module[1] and dist target for creating tarballs[2]. IMHO, pkgconfig module's generate function is an interesting option because it avoids the need for an external template file when generating the pc file. I have tested it and, unlike previous versions, I have been able to generate the expected content properly. I have also tested the dist target, which is also working properly after solving a minor issue regarding 'test-icons' and 'bvw-test'. These were defined as tests programs but in fact they are not (at least not in the sense of unattended test programs which are supposed to be run in a CI service). Finally I have raised required meson version to 0.41.0. Although I thought that smaller patches would help for their review, I have merged these changes in the meson build port patch. [0] https://groups.google.com/forum/#!topic/mesonbuild/o2mosa0U8Vs [1] http://mesonbuild.com/Release-notes-for-0-41-0.html#pkgconfig-support-for-custom-variables [2] http://mesonbuild.com/Release-notes-for-0-41-0.html#a-target-for-creating-tarballs
Created attachment 353816 [details] [review] Port to meson build system I have discovered today the add_languages function[0], that adds a new language out of the project description/function, which is something that totem uses to load vala support in case it is enabled. I have modified the patch to take advantage of this feature. [0] http://mesonbuild.com/Reference-manual.html#add_languages
Review of attachment 353816 [details] [review]: Looks mostly correct otherwise. ::: meson_options.txt @@ +1,3 @@ +option('enable-easy-codec-installation', type: 'boolean', value: true, description: 'Whether to enable easy codec installation support for GStreamer') +option('enable-python', type: 'combo', choices: ['yes', 'no', 'auto'], value: 'auto', description: 'Enable python support') +option('enable-vala', type: 'boolean', value: false, description: 'whether Vala plugin support is requested') This is supposed to be a compile-time check. If vala is available, it should use it. Right now, it doesn't check for it at all. @@ +3,3 @@ +option('enable-vala', type: 'boolean', value: false, description: 'whether Vala plugin support is requested') +option('with-plugins', type: 'combo', choices: ['all', 'none', 'auto'], value: 'auto', description: 'Which Totem plugins to compile (default: auto; "all", "none" and "auto" are valid)') +option('enable-nautilus', type: 'boolean', value: false, description: 'compile the nautilus plugin') Ditto.
Created attachment 354253 [details] [review] Port to meson build system Since I started working on the meson build port, I have had some questions regarding auto options. If those options aren't explicitly enabled by a user, should they be enabled? The reasons behind this question are the following: - They add detection logic in the build files adding complexity to them. - They add compilation time, which might be unnecessary. - They add storage space in the objects (libraries, executables), which also might be unnecessary. - They add support for features that the user might not worry about. I suppose that, if the users are interested enough, they could enable them by using available options. Anyway, it's true that I already added auto support to some options (python, plugins), so this actually wasn't consistent. Hence, I have reviewed the patch adding proper auto support for every option that had it in autotools. Finally, just as a side note and please correct me if I’m mistaken, but when looking at autotools files, I have come across some cases where they could fail. For example, it's not really possible to disable easy-codec-installation, as there are some fixed dependencies. Nor can python be fully disabled because plugins would be included all the time. I could fix these in the meson build.
Created attachment 354306 [details] [review] build: Remove autotools
Created attachment 354307 [details] [review] WIP meson: automatically enable nautilus support
Created attachment 354308 [details] [review] WIP meson: build internal libs as static
Created attachment 354309 [details] [review] WIP meson: libtotem is supposed to be a shared lib otherwise g-ir-scanner can't inspect it, and it breaks plugins.
Created attachment 354310 [details] [review] WIP meson: vala plugins depend on gir support
Created attachment 354311 [details] [review] WIP meson: vala plugins depend on gir support
(In reply to Bastien Nocera from comment #24) > Created attachment 354310 [details] [review] [review] > WIP meson: vala plugins depend on gir support totem_gir was there because I tried to generate a Vala API file by following a meson example[0], to be used as a dependency in vala plugins, but I wasn't able to get it working due to several errors in the generation: Generating Totem-1.0.vapi with a custom command. FAILED: src/Totem-1.0.vapi /usr/bin/vapigen --quiet --library=Totem-1.0 --directory=_build/src --metadatadir=src _build/src/Totem-1.0.gir ** (vapigen:28999): CRITICAL **: vala_gir_parser_push_node: assertion 'name != NULL' failed ** (vapigen:28999): CRITICAL **: vala_struct_construct: assertion 'name != NULL' failed ** (vapigen:28999): CRITICAL **: vala_symbol_set_access: assertion 'self != NULL' failed ** (vapigen:28999): CRITICAL **: vala_symbol_set_external: assertion 'self != NULL' failed ** (vapigen:28999): CRITICAL **: vala_symbol_set_comment: assertion 'self != NULL' failed ** (vapigen:28999): CRITICAL **: vala_gir_parser_push_node: assertion 'name != NULL' failed ** (vapigen:28999): CRITICAL **: vala_struct_construct: assertion 'name != NULL' failed ** (vapigen:28999): CRITICAL **: vala_symbol_set_access: assertion 'self != NULL' failed ** (vapigen:28999): CRITICAL **: vala_symbol_set_external: assertion 'self != NULL' failed ** (vapigen:28999): CRITICAL **: vala_symbol_set_comment: assertion 'self != NULL' failed ** ERROR:arraylist.c:383:vala_array_list_real_get: assertion failed: (index >= 0 && index < _size) Aborted Actually I've never made anything with Vala and I don't know how it works, so I wasn't aware that the gir file was missing. Probably libtotem_gir is a better name because it is generated using libtotem library. btw, there is an space in front of plugin_files which might not be needed[1]. [0] https://github.com/mesonbuild/meson/tree/master/test%20cases/vala/11%20generated%20vapi [1] https://bugzilla.gnome.org/attachment.cgi?id=354310&action=diff#a/src/plugins/rotation/meson.build_sec1
(In reply to Iñigo Martínez from comment #26) > (In reply to Bastien Nocera from comment #24) > > Created attachment 354310 [details] [review] [review] [review] > > WIP meson: vala plugins depend on gir support > > totem_gir was there because I tried to generate a Vala API file by following > a meson example[0], to be used as a dependency in vala plugins, but I wasn't > able to get it working due to several errors in the generation: We don't need a vapi file generated, we just need a dependency on the generated .gir file. The dependency doesn't seem to work in "ninja dist" however...
Created attachment 354312 [details] [review] WIP meson: vala plugins depend on gir support I have modified your last patch[0] with this patch, which also modifies zeitgeist-dp. I have also changed totem_gir to libtotem_gir, because I think that it may help to anyone getting in touch with totem build files. Hope this helps. [0] https://bugzilla.gnome.org/attachment.cgi?id=354311
Created attachment 354314 [details] [review] removed test-properties-page as unit test You may also need this patch, which removes test-properties-page as an unit test. "ninja dist" tries to execute it when packing, when actually it shouldn't.
Created attachment 354321 [details] [review] build: Port to meson build system With additional testing and patches from Bastien Nocera <hadess@hadess.net>
Created attachment 354322 [details] [review] build: Remove autotools
Attachment 354321 [details] pushed as 9525607 - build: Port to meson build system Attachment 354322 [details] pushed as eca1ece - build: Remove autotools
I have seen commit 919ac2e891b4aae62e4f2b4900e607ddcdd2012c where AppStream XML data destination directory has been changed from metainfo back to appdata. The destination directory in the original patch was 'appdata', but I changed it to 'metadata' following the comments below: https://bugzilla.gnome.org/show_bug.cgi?id=783205#c1 > ::: data/appdata/meson.build > @@ +1,3 @@ > +appdata = 'org.gnome.Totem.appdata.xml' > + > +custom_target( > > It would be great to drop intltool for the appdata XML; this would become: > > i18n = import('i18n') > i18n.merge_file('desktop', > input: 'org.gnome.Totem.appdata.xml.in', > output: 'org.gnome.Totem.appdata.xml', > po_dir: join_paths(meson.source_root(), 'po'), > install: true, > install_dir: join_paths(totem_datadir, 'metainfo')) https://bugzilla.gnome.org/show_bug.cgi?id=783205#c10 > ::: data/appdata/meson.build > @@ +7,3 @@ > + command: intltool_xml_cmd, > + install: true, > + install_dir: join_paths(totem_datadir, 'appdata') > > Side note: the new, canonical location for AppStream XML is `$datadir/metainfo`. The appdata.xml file should also be renamed to `org.gnome.Totem.metainfo.xml`, but that's not strictly necessary. I have also made the same change in the following meson ports: * bijiben: https://git.gnome.org/browse/bijiben/tree/data/meson.build?h=wip/inigomartinez/meson#n25 * gnome-todo: https://git.gnome.org/browse/gnome-todo/tree/data/appdata/meson.build#n9 * devhelp: https://git.gnome.org/browse/devhelp/tree/data/meson.build?h=wip/meson#n9 These changes were done because I thought that it was the proper way to do it. Is it actually wrong? Should I change it?
(In reply to Iñigo Martínez from comment #33) > I have seen commit 919ac2e891b4aae62e4f2b4900e607ddcdd2012c where AppStream > XML data destination directory has been changed from metainfo back to > appdata. > > The destination directory in the original patch was 'appdata', but I changed > it to 'metadata' following the comments below: > > https://bugzilla.gnome.org/show_bug.cgi?id=783205#c1 I missed that comment. > > ::: data/appdata/meson.build > > @@ +1,3 @@ > > +appdata = 'org.gnome.Totem.appdata.xml' > > + > > +custom_target( > > > > It would be great to drop intltool for the appdata XML; this would become: > > > > i18n = import('i18n') > > i18n.merge_file('desktop', > > input: 'org.gnome.Totem.appdata.xml.in', > > output: 'org.gnome.Totem.appdata.xml', > > po_dir: join_paths(meson.source_root(), 'po'), > > install: true, > > install_dir: join_paths(totem_datadir, 'metainfo')) > > https://bugzilla.gnome.org/show_bug.cgi?id=783205#c10 > > > ::: data/appdata/meson.build > > @@ +7,3 @@ > > + command: intltool_xml_cmd, > > + install: true, > > + install_dir: join_paths(totem_datadir, 'appdata') > > > > Side note: the new, canonical location for AppStream XML is `$datadir/metainfo`. The appdata.xml file should also be renamed to `org.gnome.Totem.metainfo.xml`, but that's not strictly necessary. > > I have also made the same change in the following meson ports: > > * bijiben: > https://git.gnome.org/browse/bijiben/tree/data/meson.build?h=wip/ > inigomartinez/meson#n25 > * gnome-todo: > https://git.gnome.org/browse/gnome-todo/tree/data/appdata/meson.build#n9 > * devhelp: > https://git.gnome.org/browse/devhelp/tree/data/meson.build?h=wip/meson#n9 > > These changes were done because I thought that it was the proper way to do > it. Is it actually wrong? Should I change it? Please change it as a separate commit. It should have been a separate commit in any case, just as you wouldn't commit code changes along with the meson port, I don't think we should have changed the contents or the install dir of the appdata/metainfo file in the port. A separate bug would also be nice, thanks for the follow-up!