GNOME Bugzilla – Bug 781944
Build system improvements
Last modified: 2017-05-17 11:52:29 UTC
See the individual patches.
Created attachment 350732 [details] [review] build: Fix distcheck
Created attachment 350733 [details] [review] build: Use a more idiomatic way to find Python 3
Created attachment 350734 [details] [review] build: Simplify handling of the tests Most tests are declared the exact same way: they have a name, which corresponds to the source file, and they depend on libgom, without anything special. We can factorize these to simplify the build system.
Created attachment 350735 [details] [review] build: Add the meson files to EXTRA_DIST We're going to have to keep the autotools for some time, so let's make sure the tarballs contain everything to build with meson.
Created attachment 350736 [details] [review] build: Add the meson files to EXTRA_DIST We're going to have to keep the autotools for some time, so let's make sure the tarballs contain everything to build with meson.
Review of attachment 350732 [details] [review]: Would be good to mention autotools in the subject. Can you also add a little detail about what change you're making in the commit message? Looks good to commit after that.
Review of attachment 350733 [details] [review]: idiomatic for what? Can you add a reference in the commit message? Looks good otherwise.
Review of attachment 350734 [details] [review]: Mention meson in the commit subject, please. Otherwise, nice cleanup.
Review of attachment 350736 [details] [review]: build: Add meson build tools to dist tarballs seems better to me. > We're going to have to keep the autotools for some time, You can remove that bit. Looks good otherwise.
> Would be good to mention autotools in the subject. "distcheck" only exists with autotools, mentioning it seems redundant. > Can you also add a little detail about what change you're making in the commit message? I thought that was such an obvious one-liner change. But sure, I'll write a few paragraphs of prose explaining it. > You can remove that bit. I think that bit is important: the commit is only needed as long as the release tarballs are generated with the autotools. If we removed the autotools completely and moved fully to meson, we wouldn't need the change at all. Therefore, that bit explains why this commit is required.
Created attachment 350737 [details] [review] meson: Use a more idiomatic way to find Python 3 We were using the generic find_program function to find the python3 executable. However, Meson provides a builtin python3 module which, among other things, allows doing just that. http://mesonbuild.com/Python-3-module.html
Created attachment 350738 [details] [review] meson: Simplify handling of the tests Most tests are declared the exact same way: they have a name, which corresponds to the source file, and they depend on libgom, without anything special. We can factorize these to simplify the build system.
Created attachment 350739 [details] [review] autotools: Fix distcheck The doc/gom-scan.o file is generated as part of building Gom. That file is not cleaned automatically, which breaks `make distcheck`. We need to explicitly tell the autotools build system to clean this file, by adding it to CLEANFILES, so that `make distcheck` is finally happy.
Created attachment 350740 [details] [review] autotools: Add the Meson files to the dist tarballs We'd like everybody to build Gom with Meson in the near-future. However, for now we're still generating the release tarballs with the Autotools and their `make distcheck` target. This change makes sure tarballs contain the essentials to be built with Meson.
(In reply to Mathieu Bridon from comment #10) > > Would be good to mention autotools in the subject. > > "distcheck" only exists with autotools, mentioning it seems redundant. I'd rather it was explicitly mentioned, so we don't have a code archeologist wondering how meson provided a distcheck command, or somesuch. It's confusing enough having 2 build systems, it doesn't cost us anything being precise about the names. > > Can you also add a little detail about what change you're making in the commit message? > > I thought that was such an obvious one-liner change. But sure, I'll write a > few paragraphs of prose explaining it. I'll comment on that in the patch itself. > > You can remove that bit. > > I think that bit is important: the commit is only needed as long as the > release tarballs are generated with the autotools. If we removed the > autotools completely and moved fully to meson, we wouldn't need the change > at all. > > Therefore, that bit explains why this commit is required. No, that sentence makes a (probably inaccurate) comment about our autotools usage: > We're going to have to keep the autotools for some time, Or maybe we won't. What if we kept it just for the next release? What's forcing us? I'm saying it doesn't matter if we keep it for a short time, or a long time. Mentioning that it's necessary to allow building autools-generated tarballs using Meson is probably all that's needed, no?
Review of attachment 350739 [details] [review]: > The doc/gom-scan.o file is generated as part of building Gom. That file > is not cleaned automatically, which breaks `make distcheck`. > > We need to explicitly tell the autotools build system to clean this > file, by adding it to CLEANFILES, so that `make distcheck` is finally > happy. I don't know whether this was supposed to be a sarcastic commit message, but it certainly sounds like one. The purpose of asking for an explanation was to have a real explanation. Presumably you'd already investigated the problem (see below). > The doc/gom-scan.o file is generated as part of building Gom. That file > is not cleaned automatically, which breaks `make distcheck`. It's generated as part of building the docs, by gtk-doc. Seeing as a similar change wasn't needed in other modules using gtk-doc, I'm guessing it could be a bug in the newly Python-ed gtk-doc? Certainly doesn't look correct...
Review of attachment 350740 [details] [review]: ++
> I don't know whether this was supposed to be a sarcastic commit message, but it certainly sounds like one. It is not, that message is my best attempt at a serious explanation of the patch. I don't know what causes the problem with gtk-doc, and I don't really care to be honest. I was working on the meson port, which is what interests me, and I saw that distcheck was broken so I figured I'd help you with a fix. And if we're moving to Meson, then it makes no sense to spend time on the Autotools buildsystem. In fact I feel like I've already spent too much time on this specific commit (fixing a legacy buildsystem when we're moving to a new one) and so I will stop here. Feel free to fix it in another way if you prefer. Since you approved the other patches I'll push them now.
Attachment 350737 [details] pushed as fec80d9 - meson: Use a more idiomatic way to find Python 3 Attachment 350738 [details] pushed as 2a4e822 - meson: Simplify handling of the tests Attachment 350740 [details] pushed as ba71d70 - autotools: Add the Meson files to the dist tarballs
Review of attachment 350739 [details] [review]: As I can't reproduce this problem with an upstream version of gtk-doc (1.25), I'll reject this. Probably a regression of the Pythonised gtk-doc.
All good, thanks!