control: block -1 by 870260
Thanks for the thorough review. It took me quite a bit to address all these
comments :).
Find the updated patch attached, and answers inline:
On Wed, Aug 2, 2017 at 8:11 AM, Johannes Schauer <jo...@debian.org> wrote:
> Quoting Michael Stapelberg (2017-08-01 23:15:04)
> > Alright! Patch attached and provided inline, for your convenience:
>
> Cool!
>
> > +if (!defined($ENV{SUDO_USER})) {
> > + die "Please run sudo $0";
> > +}
>
> Should you not rather check the UID instead?
>
Why? We need SUDO_USER later, for the adduser command.
>
> > +system("adduser", "--quiet", "--", $ENV{SUDO_USER}, "sbuild");
>
> I still don't understand the problem with sbuild-adduser?
>
sbuild-adduser prints a whole bunch of extra messages after running
adduser. In fact, looking at its source, its only purpose seems to be to
print these messages after wrapping adduser. These messages don’t make
sense for the use-case at hand, I think:
'''
# Setup tasks for sudo users:
# BUILD
# HOME directory in chroot, user:sbuild, 0770 perms, from
# passwd/group copying to chroot, filtered
# Maybe source 50sbuild, or move into common location.
Next, copy the example sbuildrc file to the home directory of each user and
set the variables for your system:
cp /usr/share/doc/sbuild/examples/example.sbuildrc /home/michael/.sbuildrc
Now try a build:
cd /path/to/source
sbuild-update -ud <distribution>
(or "sbuild-apt <distribution> apt-get -f install"
first if the chroot is broken)
sbuild -d <distribution> <package>_<version>
'''
>
> > +chomp(my $arch = `dpkg --print-architecture`);
> > +
> > +system("sbuild-createchroot",
> > + "--command-prefix=eatmydata",
>
> There is no --command-prefix option yet. Should this bug not be blocked by
> #870260?
>
Done.
>
> > + "--include=eatmydata",
> > + "--alias=UNRELEASED",
> > + "--alias=sid",
> > + "unstable",
> > + "/srv/chroot/unstable-$arch-sbuild",
> > + "http://localhost:3142/deb.debian.org/debian";);
>
> What happens if any of this fails? You don't catch any errors.
>
I use the autodie module, which catches the error.
>
> What happens if the user runs the script a second time?
>
All actions except for the chroot creation are idempotent.
>
> You could also check some things *before* you attempt them like:
>
> - is the user already in the sbuild group?
>
adduser is a noop if the user is already in the sbuild group.
> - does the output path already exist?
>
Done.
>
> > +open(my $fh, ">", "/etc/cron.d/sbuild-debian-dev
> eloper-setup-update-all");
> > +say $fh q|@daily root /usr/share/doc/sbuild/examples
> /sbuild-update-all|;
> > +close($fh);
>
> Why not a simple symlink in /etc/cron.daily/?
>
Wasn’t aware of cron.daily, I use systemd timer units :). Done.
>
> > +say "Now run `newgrp sbuild', or log out and log in again.";
>
> You could tell the user more about what the script did here. For example
> you
> could say things like:
>
> - added $USER to the sbuild group
> - created schroot called "unstable" with chroot locatled in /srv/...
> - created daily schroot upgrade cronjob
>
Done.
>
> The user should also instruct the user that they should not run the script
> again.
>
No; I implemented your other suggestion, so running the script multiple
times is okay.
>
> And you could also print *how* to use sbuild to build packages, for
> example by
> printing:
>
> $ sbuild -d unstable hello
> $ sbuild -d unstable hello.dsc
> $ cd hello && sbuild
>
> > diff --git a/debian/control b/debian/control
> > index 7249e630..7b4bd21b 100644
> > --- a/debian/control
> > +++ b/debian/control
> > @@ -75,6 +75,22 @@ Description: Tool for building Debian binary packages
> > from Debian sources
> > build-essential packages, plus those in the package build
> > dependencies.
> >
> > +Package: sbuild-debian-developer-setup
>
> I guess you need the extra package for the apt-cacher-ng dependency? How
> about
> putting the script into the sbuild package and making apt-cacher-ng a
> Recommends?
>
> > +Architecture: all
> > +Depends: sbuild,
> > + apt-cacher-ng,
>
> Will this not also work with the apt-cacher package in the same way?
>
It will, just verified it. Added apt-cacher as an alternative dependency. I
prefer apt-cacher-ng, because it doesn’t ask my questions during
installation.
>
> > + lintian,
>
> I still don't understand why you need lintian as a runtime dependency. To
> run
> lintian, sbuild installs lintian *inside* the chroot. Lintian on the host
> running sbuild is never used.
>
Ah, I didn’t realize that. Removed.
>
> You forgot a dependency on cron.
>
Added.
>
> Doesn't systemd not also provide a cron-replacement? Maybe you could
> provide a
> solution for that to and the depend on "cron | systemd-sysv"
>
Isn’t a better way to do that to depend on the virtual package
“cron-daemon”? Did that instead.
>
> > + ${misc:Depends},
> > + ${perl:Depends},
> > + ${shlibs:Depends}
> > +Description: Convenience script to set up an sbuild environment for
> Debian
> > Developers
> > + Run "sudo sbuild-debian-developer-setup" to add the current user to the
> > sbuild
> > + group, create an schroot for building packages for Debian unstable, and
> > create
> > + a cronjob which updates said schroot daily.
> > + .
> > + This script assumes you are on an un-metered internet connection (daily
> > schroot
> > + updates might be costly otherwise).
>
> It's nice that you put "debian" in the package name. Why is "debian" not
> also
> part of the executable name?
>
…but it is?
>
> How about letting the script take two optional arguments: distribution and
> suite. Those could default to "debian" and "unstable" and then downstreams
> could easily adjust these defaults.
>
Done for both, but I don’t see what the correct interface is to tell
sbuild-createchroot to use a different distribution. Can you suggest how to
call sbuild-createchroot please?
>
> You should provide a man page to the script.
>
Done.
>
> There should be at least the --help command line option which explains how
> to
> use the script.
>
Done.
--
Best regards,
Michael
From caceb17c74044690912eeed64fa40127c15e578c Mon Sep 17 00:00:00 2001
From: Michael Stapelberg <stapelb...@debian.org>
Date: Tue, 1 Aug 2017 23:13:36 +0200
Subject: [PATCH] add sbuild-debian-developer-setup package
---
bin/Makefile.am | 1 +
bin/sbuild-debian-developer-setup | 71 ++++++++++++++++++++++++++++
debian/control | 16 +++++++
debian/sbuild-debian-developer-setup.install | 1 +
man/Makefile.am | 1 +
5 files changed, 90 insertions(+)
create mode 100755 bin/sbuild-debian-developer-setup
create mode 100644 debian/sbuild-debian-developer-setup.install
diff --git a/bin/Makefile.am b/bin/Makefile.am
index 138fc0b3..0fcbda09 100644
--- a/bin/Makefile.am
+++ b/bin/Makefile.am
@@ -28,6 +28,7 @@ bin_SCRIPTS = \
sbuild-abort \
sbuild-apt \
sbuild-checkpackages \
+ sbuild-debian-developer-setup \
sbuild-update \
sbuild-upgrade \
sbuild-distupgrade \
diff --git a/bin/sbuild-debian-developer-setup b/bin/sbuild-debian-developer-setup
new file mode 100755
index 00000000..062fcf9e
--- /dev/null
+++ b/bin/sbuild-debian-developer-setup
@@ -0,0 +1,71 @@
+#!/usr/bin/perl
+#
+# Set up sbuild so that packages for Debian unstable can be built and
+# maintenance is done automatically via a daily update cronjob.
+# Copyright © 2017 Michael Stapelberg <stapelb...@debian.org>.
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+# General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see
+# <http://www.gnu.org/licenses/>.
+#
+#######################################################################
+
+use strict;
+use warnings;
+use autodie qw(:all);
+use v5.10;
+use Getopt::Long;
+use Sbuild qw(help_text version_text usage_error check_group_membership);
+
+my $dist = "debian";
+my $suite = "unstable";
+
+my @options = (
+ 'distribution=s' => \$dist,
+ 'suite=s' => \$suite,
+ 'help' => sub { help_text(1, "sbuild-debian-developer-setup") },
+ );
+GetOptions(@options);
+
+if (!defined($ENV{SUDO_USER})) {
+ die "Please run sudo $0";
+}
+
+system("adduser", "--", $ENV{SUDO_USER}, "sbuild");
+
+chomp(my $arch = `dpkg --print-architecture`);
+
+sub chroot_exists {
+ no autodie qw(system);
+ system("schroot -i -c chroot:$suite-$arch-sbuild >/dev/null 2>&1") == 0
+}
+
+if (!chroot_exists()) {
+ system("sbuild-createchroot",
+ "--command-prefix=eatmydata",
+ "--include=eatmydata",
+ "--alias=UNRELEASED",
+ "--alias=sid",
+ "$suite",
+ "/srv/chroot/$suite-$arch-sbuild",
+ "http://localhost:3142/deb.debian.org/debian";);
+} else {
+ say "chroot $suite-$arch-sbuild already exists";
+}
+
+if (! -e "/etc/cron.daily/sbuild-debian-developer-setup-update-all") {
+ symlink("/usr/share/doc/sbuild/examples/sbuild-update-all", "/etc/cron.daily/sbuild-debian-developer-setup-update-all");
+ say "ln -s /usr/share/doc/sbuild/examples/sbuild-update-all /etc/cron.daily/sbuild-debian-developer-setup-update-all";
+}
+
+say "Now run `newgrp sbuild', or log out and log in again.";
diff --git a/debian/control b/debian/control
index 7249e630..9fa8deae 100644
--- a/debian/control
+++ b/debian/control
@@ -75,6 +75,22 @@ Description: Tool for building Debian binary packages from Debian sources
build-essential packages, plus those in the package build
dependencies.
+Package: sbuild-debian-developer-setup
+Architecture: all
+Depends: sbuild,
+ apt-cacher-ng | apt-cacher,
+ cron-daemon,
+ ${misc:Depends},
+ ${perl:Depends},
+ ${shlibs:Depends}
+Description: Convenience script to set up an sbuild environment for Debian Developers
+ Run "sudo sbuild-debian-developer-setup" to add the current user to the sbuild
+ group, create an schroot for building packages for Debian unstable, and create
+ a cronjob which updates said schroot daily.
+ .
+ This script assumes you are on an un-metered internet connection (daily schroot
+ updates might be costly otherwise).
+
Package: buildd
Architecture: all
Depends: adduser,
diff --git a/debian/sbuild-debian-developer-setup.install b/debian/sbuild-debian-developer-setup.install
new file mode 100644
index 00000000..406b3af9
--- /dev/null
+++ b/debian/sbuild-debian-developer-setup.install
@@ -0,0 +1 @@
+usr/bin/sbuild-debian-developer-setup
diff --git a/man/Makefile.am b/man/Makefile.am
index 61754e27..aaaca683 100644
--- a/man/Makefile.am
+++ b/man/Makefile.am
@@ -35,6 +35,7 @@ man_MANS = \
sbuild-apt.1 \
sbuild-checkpackages.1 \
sbuild-createchroot.8 \
+ sbuild-debian-developer-setup.1 \
sbuild-destroychroot.8 \
sbuild-hold.1 \
sbuild-setup.7 \
--
2.13.2
