[buildd-tools-devel] Bug#859867: Bug#859867: Bug#859867: Please add a package which automatically configures sbuild for Debian packaging

Michael Stapelberg stapelberg at debian.org
Sat Aug 5 17:38:25 UTC 2017


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 <josch at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.alioth.debian.org/pipermail/buildd-tools-devel/attachments/20170805/4e212799/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-add-sbuild-debian-developer-setup-package.patch
Type: text/x-patch
Size: 5182 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/buildd-tools-devel/attachments/20170805/4e212799/attachment-0001.bin>


More information about the Buildd-tools-devel mailing list