GNOME Bugzilla – Bug 790178
Various meson related improvements
Last modified: 2017-12-18 09:03:18 UTC
This bug includes various meson improvements.
Created attachment 363360 [details] [review] build: Fix index used for PACKAGE_STRING In the PACKAGE_STRING variable generation wrong index is used. This patch uses string concatenation to fix the problem.
Created attachment 363361 [details] [review] build: Remove default warning level `network-manager-applet` uses 1 as the project's default warning level. However, meson already uses this level as the default warning level. This patch removes the warning level from project's default options.
Created attachment 363362 [details] [review] build: Rename build options Following the new meson porting guidelines[0], this patch renames the build options. The list of changes is as follows: - Remove the `enable` prefix from boolean options. - Remove the `with` prefix from string options. - The character separator from multi-word options has been changed to underscore (`_`). [0] https://wiki.gnome.org/Initiatives/GnomeGoals/MesonPorting
Created attachment 363364 [details] [review] Use meson 0.43.0 features meson 0.43.0 comes with a new function in the compiler's object called get_supported_arguments, which allows checking multiple optiones at once. This patch bumps meson's version number and also takes advantage of this new feature.
Created attachment 363395 [details] [review] build: Use a variable for the name A previous commit[0] fixed a mistake done by me regarding the names. This patch tries to improve that fix a little bit by introducing a new variable which holds the name (may I say short name?) used by network-manager-applet, which is `nm-applet`. Those would help in case some `nma` fields are renamed (let's say that PACKAGE define is moved from `nm-applet` to `network-manager-applet`) and also helps in consistency, since the same string is used where it is needed, avoiding any error caused by handwriting. The name variable is also used as the `gettext` domain, as both use the same string. If desired, a new variable can also be created to separate both concepts. I haven't done it because it's used two times, which in my eyes didn't seem to be reason enough. [0] https://git.gnome.org/browse/network-manager-applet/commit/?id=b1a7c2b93
Created attachment 363401 [details] [review] build: Remove unused defines There are a lot of defines in the `config.h` file mimicking those generated by autotools. However, most of them are not actually needed. `PACKAGE` defines, std headers defines, functions defines. I am used to not removed them because I'm not sure if they are necessary in non-linux systems (bsd?), or even on older linux systems. They are at least not necessary on my system. This patch removes the correspondant checks and their settings in the configuration object. The `HAVE_CONFIG_H` macro definition, which is also used by autotools, is also no necessary, so it has been removed. Regarding `PACKAGE*` defines, they are actually needed in the documentation generation by the `gtkdocentities.ent` file. They have been removed from the `config_h` configuration object and the template file used and a new configuration object has been created which will only be used when documentation generation is requested.
Created attachment 363402 [details] [review] build: Remove default options I've also noticed that setting `b_lundef` in the project's default options is unnecessary. This option was used by autotools so I ended setting it up in the default options to enforce its use. However it's already in meson's default options, so setting the option again may lead to confusion. This patch obsolotes the previous one, 363361, which only removed `warning_level` option.
Created attachment 363403 [details] [review] build: Improve AppStream metainfo handling I'm sorry for this mistake, I was confused and it also happened to me with other meson ports[0]. Thanks for fixing it in the commit c5786deb4. This patch improves it a little bit, avoiding to use the same string twice, so if the file name is changed it's less error prone. I've also updated the AppStream metainfo directory, which must be now `metainfo`. [0] https://bugzilla.gnome.org/show_bug.cgi?id=784922#c9
Created attachment 363404 [details] [review] build: Remove unused defines I've fixed a little bit the commit subject which was not the best.
`git bz apply` is unable to apply the patches without conflict. Can you instead make a github pull-request, link to a git-branch somewhere, or attach a complete set of patches which applies without conflicts? Thanks
(In reply to Thomas Haller from comment #10) > `git bz apply` is unable to apply the patches without conflict. I'm sorry about this. I had been uploading the patches while I was making changes, trying to separate them by concepts instead of uploading a `big` patch with multiple unrelated changes. In case you consider it more convenient, I can also squash the patches. > Can you instead make a github pull-request, link to a git-branch somewhere, or > attach a complete set of patches which applies without conflicts? Thanks Sure! You can find the changes in the following wip branch: https://git.gnome.org/browse/network-manager-applet/log/?h=wip/inigomartinez/meson-improvements
Thanks. wip/inigomartinez/meson-improvements lgtm
Looks good.
merged to master: https://git.gnome.org/browse/network-manager-applet/commit/?id=930f12b035593f7b89f535b701eb7062f26406f6
Thanks Iñigo!!
> I've also updated the AppStream metainfo directory, which must be now `metainfo`. Note that I've reverted this again. Not because that it would be an inherently bad change, but it regresses from what's done by autotools. Of course, the autotools could be changed too. However, it's not clear (from the commit message), whether this is going to break on older platforms. On my system (Fedora 27), most packages seems to use the /usr/share/appdata, not /usr/share/metainfo
(In reply to Lubomir Rintel from comment #16) > > I've also updated the AppStream metainfo directory, which must be now `metainfo`. > > Note that I've reverted this again. Not because that it would be an > inherently bad change, but it regresses from what's done by autotools. > > Of course, the autotools could be changed too. However, it's not clear (from > the commit message), whether this is going to break on older platforms. > > On my system (Fedora 27), most packages seems to use the /usr/share/appdata, > not /usr/share/metainfo I'm not sure what is the situation regarding `/usr/share/appdata`. I guess that in order to avoid breaking current applications, there is backward compatibility, but the current spec recommends `/usr/share/metainfo` for Desktop applications[0]. I wasn't aware of the new spec recommendation until some months ago[1], and following that comment I also made some mistakes[2]. I didn't care about autotools, because my idea was to provide a patch to remove it at some point. However, while autotools is supported, probably `appdata` should be renamed to `metainfo`. [0] https://www.freedesktop.org/software/appstream/docs/sect-Metadata-Application.html [1] https://bugzilla.gnome.org/show_bug.cgi?id=783205#c10 [2] https://bugzilla.gnome.org/show_bug.cgi?id=784922#c8