GNOME Bugzilla – Bug 696182
Add pre-receive hook to jhbuild repository to catch XML errors
Last modified: 2016-01-05 15:12:04 UTC
As more things start depending on jhbuild (such as Jenkins and build bots), the validity of moduleset files becomes more important.
It would be quite useful if there was a pre-receive hook on the jhbuild git module to run `make check-modulesets` on jhbuild (which I’ll add to jhbuild as part of a separate bug) and abort the push if that fails.
That would allow anyone with a git account to run software on git.gnome.org. A hook should be on its own and be committed to sysadmin-bin/git.
(In reply to comment #0)
> It would be quite useful if there was a pre-receive hook on the jhbuild git
> module to run `make check-modulesets` on jhbuild (which I’ll add to jhbuild as
> part of a separate bug) and abort the push if that fails.
Filed as bug #696184.
The jhbuild build bots would immediately fail though, and we'd hopefully notice and fix it quickly. See https://bugzilla.gnome.org/show_bug.cgi?id=695149 for some ideas on how we could do that.
(In reply to comment #3)
> The jhbuild build bots would immediately fail though, and we'd hopefully notice
> and fix it quickly.
True, but this means lots of people get notified (e.g. through Jenkins) and module health stats get skewed unnecessarily. I think it’s better to get the original committer to fix their mistake before pushing, then nobody has to waste time context switching to “what’s gone wrong with Jenkins” mode.
(In reply to comment #4)
> (In reply to comment #3)
> > The jhbuild build bots would immediately fail though, and we'd hopefully notice
> > and fix it quickly.
> True, but this means lots of people get notified (e.g. through Jenkins) and
> module health stats get skewed unnecessarily. I think it’s better to get the
> original committer to fix their mistake before pushing, then nobody has to
> waste time context switching to “what’s gone wrong with Jenkins” mode.
Right; but as a general rule I think one has to carefully constrain what code gets run as part of a pre-commit. Checking for trailing whitespace and that sort of thing is pretty easy, runs in a predictable amount of time, etc. But growing that closer to "make check" type stuff is another world.
There's the simple fact that we're running arbitrary code from the repositories on the git server itself (does this code have write access to the repo?).
The other way to ensure that bad code doesn't make it into the repos is to automatically check incoming patches before they land.
Created attachment 239519 [details] [review]
Draft pre-receive hook to check jhbuild modulesets
Here’s a first draft of a pre-receive hook to implement this. As suggested, it avoids executing arbitrary code on the server (`make check-modulesets`) by replicating the xmllint call itself. It’s only one line of code. It checks every moduleset file updated in a commit, but doesn’t re-check all modulesets if the DTD changes (see the comment at the top of the file). I don’t know if we want this.
It’s completely untested and may have silly syntax errors or explode. I’m just attaching it to get feedback on whether sysadmins (and others) think this is the right approach.
The GNOME Infrastructure Team is currently migrating its bug / issue tracker away from Bugzilla to Request Tracker and therefore all the currently open bugs have been closed and marked as OBSOLETE.
The following move will also act as a cleanup for very old and ancient tickets that were still living on Bugzilla. If your issue still hasn't been fixed as of today please report it again on the relevant RT queue.
More details about the available queues you can report the bug against can be found at https://wiki.gnome.org/Sysadmin/RequestTracker.
Thanks for your patience,
the GNOME Infrastructure Team
Re-opening as this bug is still valid.
(In reply to comment #8)
> Re-opening as this bug is still valid.
So what do we still need to get this fixed? Looking back at the question of re-validating if the DTD changes, I think it would be too restrictive to do that, since it would disallow split commits:
1. Change DTD.
2. Update modulesets to validate against the new one.
Changes to the DTD are pretty rare; if they somehow become a problem in the future, we could re-evaluate.
Split commits are fairly easy to deal with. You just have the hook check the latest commit. A server hook only runs once you push. So you cannot push between commits, but you can afterwards.
This already happens a few times with e.g. doap file check, multiple commits each still with typo's. Only the last one fixes everything and then the push works.
Not enough interest in this, and as Colin says, we’d see failure in the buildbots quickly enough. Not worth the risk of arbitrary code execution on the git servers.
What we really want is a gating system for commits anyways with build systems that are sandboxed to some degree (e.g. linux-user-chroot, Docker, etc.) and that don't have write access to git.
(In reply to Colin Walters from comment #12)
> What we really want is a gating system for commits anyways with build
> systems that are sandboxed to some degree (e.g. linux-user-chroot, Docker,
> etc.) and that don't have write access to git.
That would be super. I guess the overall plan would be to gate things based on gnome-continuous failures/passes then? Is there a bug open for this?