GNOME Bugzilla – Bug 784922
Port to meson build system
Last modified: 2017-10-17 06:10:52 UTC
Created attachment 355545 [details] [review] Port to meson build system Complete port to meson build system.
Note: I'll be doing a dconf-editor release on behalf of release team with this patch and the patch in bug #784921 if there's no response from maintainers here after a few days.
Created attachment 360587 [details] [review] Port to meson build system Due to recent changes in #784921, I have updated slightly the patch. One of the major changes is the project license which was wrong (GPL3 vs GPL3+). Some minor improvements on how valac uses some libraries is also in place.
Created attachment 360601 [details] [review] Remove autotools Just in case it is considered, this patch removes autotools.
So, I’ve had a look to your patches, knowing nothing about Meson and more or less nothing about Autotools (!), and I took some notes about what looked strange for me in the conversion from the 2nd to the 1st. So many things are probably normal, but… * I don’t see a ´valac´ version test. * I don’t see a ´gettext´ version test. * The ‘.gresource.xml’ file is listed in ‘resource_data’, but also processed in the resources compilation. Does that remain the correct way to do? * You switched to a ‘.metainfo.xml’ thing that I don’t hear about. Why? Is that necessary? * There’s no ‘.appdata.valid’ thing. What was it, by the way? * In ‘meson_post_install.py’, there are environment updates that are not processed if DESTDIR is given. Looks quite logical, but won’t it be useful to add a textual reminder to manually update when DESTDIR is given? When could that happen? * Project version last release is now 3.26.1. * What should be PACKAGE_URL? A presentation? a manual? * Did you manually check all needed headers? Thanks for taking time to modernise a bit all these things.
(In reply to Arnaud B. from comment #4) > So, I’ve had a look to your patches, knowing nothing about Meson and more or > less nothing about Autotools (!), and I took some notes about what looked > strange for me in the conversion from the 2nd to the 1st. So many things are > probably normal, but… > > * I don’t see a ´valac´ version test. > * I don’t see a ´gettext´ version test. No, there is not. I used basic meson's mechanisms and these inherently don't do any version check. It should be noted that if they aren't recent enough, build will fail. It would be nice to have that version check though it can be easily checked by using meson's functions. If you think it's convenient, I can add a version check which also shows a help message. > * The ‘.gresource.xml’ file is listed in ‘resource_data’, but also > processed in the resources compilation. Does that remain the correct way to > do? That's totally my fault. For some time now, I keep the files sorted alphabetically to avoid this problem. I'll fix this. > * You switched to a ‘.metainfo.xml’ thing that I don’t hear about. Why? Is > that necessary? I also didn't hear about that until I ported totem to meson[0]. I then looked at AppStream's documentation[1] and noticed the change, so I take advantage of the switch to meson to also start switching the names. > * There’s no ‘.appdata.valid’ thing. What was it, by the way? Do you mean `.gschema.valid`? The `.valid` file is created when the `.gschema` file is valid. It check it's done by performing a `dry run` and `touching` the `.valid` file. This is the exact command: glib-compile-schemas --strict --dry-run --schema-file=ca.desrt.dconf-editor.gschema.xml && mkdir -p . && touch ca.desrt.dconf-editor.gschema.valid > * In ‘meson_post_install.py’, there are environment updates that are not > processed if DESTDIR is given. Looks quite logical, but won’t it be useful > to add a textual reminder to manually update when DESTDIR is given? When > could that happen? Although there is some kind of support[2], I just followed the example[3]. Actually I'm sorry to say that I've never used `DESTDIR`, so I'm not aware of how it will affect. > * Project version last release is now 3.26.1. I'll update the project version. It's very easy as the package version is stated in the `version` attribute in the `project` function. Then you can access to that data by using `meson.project_version()` (though a variable it's used in `dconf-editor` which helps later). > * What should be PACKAGE_URL? A presentation? a manual? Not many packages define `PACKAGE_URL`. Those who do it, define it with their URL are `wiki.gnome.org`. For example, `gnome-photos` defines it as `https://wiki.gnome.org/Apps/Photos`. It can be left empty if it's not used. > * Did you manually check all needed headers? Although I haven't checked if all the headers and functions tested in the meson port are actually needed, they are tested by autotools. I usually try to do an 1:1 port from autotools, and this implies creating all the `defines` at the `config.h` file. However although usually many of them are not apparently used, as I'm not aware of all operating systems/distros on which packages run, I don't remove them. If you think it's convenient to remove those that probably are unnecessary, I'll remove them. > Thanks for taking time to modernise a bit all these things. You are welcome. Thank you for taking the time to check the patch :) [0] https://bugzilla.gnome.org/show_bug.cgi?id=783205#c10 [1] https://www.freedesktop.org/software/appstream/docs/chap-Metadata.html [2] http://mesonbuild.com/Running-Meson.html#installing [3] http://mesonbuild.com/Porting-from-autotools.html#gsettings
Created attachment 361095 [details] [review] Port to meson build system I have updated the patch with the following major changes: - dconf-editor version number has been bumped to 3.26.1. - vala version is checked, and a help message is shown if it's not recent enough. Although `gettext` version checked is possible by parsing the `-V` option output, it also includes the license and it may difficult a proper implementation. As meson's `i18n` module ensures a recent enough `gettext` version, I thought that maybe it's not worth the job.
Created attachment 361096 [details] [review] Remove autotools A follow up update.
> > * You switched to a ‘.metainfo.xml’ thing that I don’t hear about. Why? Is > > that necessary? > > I also didn't hear about that until I ported totem to meson[0]. I then > looked at AppStream's documentation[1] and noticed the change, so I take > advantage of the switch to meson to also start switching the names. But the spec recommends /usr/share/metainfo/%{id}.appdata.xml....
Created attachment 361291 [details] [review] Port to meson build system (In reply to Michael Catanzaro from comment #8) > > > * You switched to a ‘.metainfo.xml’ thing that I don’t hear about. Why? Is > > > that necessary? > > > > I also didn't hear about that until I ported totem to meson[0]. I then > > looked at AppStream's documentation[1] and noticed the change, so I take > > advantage of the switch to meson to also start switching the names. > > But the spec recommends /usr/share/metainfo/%{id}.appdata.xml.... I might have made a mistake. I have changed the patch and it applies the following text now: > Applications are a special case here, because they are usually treated differently by software centers (and also for > historical reasons). If your metainfo file contains an application, as described in Section 2.2, “Desktop Applications”, > you may want to install it as /usr/share/metainfo/%{id}.appdata.xml. So the file is generated as `ca.desrt.dconf-editor.appdata.xml` and installed at `$datadir/metainfo`. I hope it's correct now.
Something was bugging me when running it, and I just understood what: the translations are not working. ^^ For historical reasons, ´dconf-editor´ uses gettext name “dconf”. Yeah, that’s bad. Yeah, there should be a plan to move all translations to a more usual name (“dconf-editor”) or a more future-proof one (“ca.desrt.dconf-editor”). Anyway, that’s not taken into account in the Meson configuration. Thanks for the `valac` version test, that’s sometimes quite helpful, notably as it contains some vapi files that describe how libraries should be used.
Created attachment 361634 [details] [review] Port to meson build system (In reply to Arnaud B. from comment #10) > Something was bugging me when running it, and I just understood what: the > translations are not working. ^^ For historical reasons, ´dconf-editor´ uses > gettext name “dconf”. Yeah, that’s bad. Yeah, there should be a plan to move > all translations to a more usual name (“dconf-editor”) or a more > future-proof one (“ca.desrt.dconf-editor”). Anyway, that’s not taken into > account in the Meson configuration. > > Thanks for the `valac` version test, that’s sometimes quite helpful, notably > as it contains some vapi files that describe how libraries should be used. Thank you for catching the issue. I usually use the english locale in my computer so I miss this problems :(. I fixed it, and I've also created a new variable called `dconf_editor_gettext`, which holds the string used as `GETTEXT_PACKAGE`, so if this changes in the future (from `dconf` to `dconf-editor`), it would only need to change it here. BTW, applications usually use the application name as `GETTEXT_PACKAGE` so it should be `dconf-editor`. I have also created a new quick patch `Change gettext package`[0], which changes autotools to move from `dconf` to `dconf-editor`. I have also tweaked a bit more the patch to include the `dconf-editor` namespace used in some files. [0] https://bugzilla.gnome.org/show_bug.cgi?id=789024
I didn’t make a real testing of that last version, but pushed (both patches) in the unstable 3.27.1 release; let’s now see what happens. We’ll do the translations domain transition in the next unstable release, if all looks good. Thanks a lot for having worked on all that. :)
I hope this helps testing it enough so we had a stable version before 3.28. Please let me know if there is any issue.
Comment on attachment 361634 [details] [review] Port to meson build system Pushed as 46204df - build: Port to meson build system
Comment on attachment 361096 [details] [review] Remove autotools Pushed as dd4612d - build: Remove autotools