<div dir="ltr">control: block -1 by 870260<br><div class="gmail_extra"><br></div><div class="gmail_extra">Thanks for the thorough review. It took me quite a bit to address all these comments :).</div><div class="gmail_extra"><br></div><div class="gmail_extra">Find the updated patch attached, and answers inline:</div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Aug 2, 2017 at 8:11 AM, Johannes Schauer <span dir="ltr"><<a href="mailto:josch@debian.org" target="_blank">josch@debian.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Quoting Michael Stapelberg (2017-08-01 23:15:04)<br>
<span class="gmail-m_5094786850249725712gmail-">> Alright! Patch attached and provided inline, for your convenience:<br>
<br>
</span>Cool!<br>
<span class="gmail-m_5094786850249725712gmail-"><br>
> +if (!defined($ENV{SUDO_USER})) {<br>
> +    die "Please run sudo $0";<br>
> +}<br>
<br>
</span>Should you not rather check the UID instead?<br></blockquote><div><br></div><div>Why? We need SUDO_USER later, for the adduser command.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class="gmail-m_5094786850249725712gmail-"><br>
> +system("adduser", "--quiet", "--", $ENV{SUDO_USER}, "sbuild");<br>
<br>
</span>I still don't understand the problem with sbuild-adduser?<br></blockquote><div><br></div><div>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:</div><div><br></div><div>'''</div><div><div># Setup tasks for sudo users:</div><div><br></div><div># BUILD</div><div># HOME directory in chroot, user:sbuild, 0770 perms, from</div><div># passwd/group copying to chroot, filtered</div><div># Maybe source 50sbuild, or move into common location.</div><div><br></div><div>Next, copy the example sbuildrc file to the home directory of each user and</div><div>set the variables for your system:</div><div><br></div><div>  cp /usr/share/doc/sbuild/examples/example.sbuildrc /home/michael/.sbuildrc</div><div>        </div><div>Now try a build:</div><div><br></div><div>  cd /path/to/source</div><div>  sbuild-update -ud <distribution></div><div>  (or "sbuild-apt <distribution> apt-get -f install"</div><div>       first if the chroot is broken)</div><div>  sbuild -d <distribution> <package>_<version></div></div><div>'''</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class="gmail-m_5094786850249725712gmail-"><br>
> +chomp(my $arch = `dpkg --print-architecture`);<br>
> +<br>
> +system("sbuild-createchroot",<br>
> +       "--command-prefix=eatmydata",<br>
<br>
</span>There is no --command-prefix option yet. Should this bug not be blocked by<br>
#870260?<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class="gmail-m_5094786850249725712gmail-"><br>
> +       "--include=eatmydata",<br>
> +       "--alias=UNRELEASED",<br>
> +       "--alias=sid",<br>
> +       "unstable",<br>
> +       "/srv/chroot/unstable-$arch-s<wbr>build",<br>
> +       "<a href="http://localhost:3142/deb.debian.org/debian" rel="noreferrer" target="_blank">http://localhost:3142/deb.de<wbr>bian.org/debian</a>");<br>
<br>
</span>What happens if any of this fails? You don't catch any errors.<br></blockquote><div><br></div><div>I use the autodie module, which catches the error.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
What happens if the user runs the script a second time?<br></blockquote><div><br></div><div>All actions except for the chroot creation are idempotent.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
You could also check some things *before* you attempt them like:<br>
<br>
 - is the user already in the sbuild group?<br></blockquote><div><br></div><div>adduser is a noop if the user is already in the sbuild group.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
 - does the output path already exist?<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class="gmail-m_5094786850249725712gmail-"><br>
> +open(my $fh, ">", "/etc/cron.d/sbuild-debian-dev<wbr>eloper-setup-update-all");<br>
> +say $fh q|@daily root /usr/share/doc/sbuild/examples<wbr>/sbuild-update-all|;<br>
> +close($fh);<br>
<br>
</span>Why not a simple symlink in /etc/cron.daily/?<br></blockquote><div><br></div><div>Wasn’t aware of cron.daily, I use systemd timer units :). Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class="gmail-m_5094786850249725712gmail-"><br>
> +say "Now run `newgrp sbuild', or log out and log in again.";<br>
<br>
</span>You could tell the user more about what the script did here. For example you<br>
could say things like:<br>
<br>
 - added $USER to the sbuild group<br>
 - created schroot called "unstable" with chroot locatled in /srv/...<br>
 - created daily schroot upgrade cronjob<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
The user should also instruct the user that they should not run the script<br>
again.<br></blockquote><div><br></div><div>No; I implemented your other suggestion, so running the script multiple times is okay.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
And you could also print *how* to use sbuild to build packages, for example by<br>
printing:<br>
<br>
    $ sbuild -d unstable hello<br>
    $ sbuild -d unstable hello.dsc<br>
    $ cd hello && sbuild<br>
<span class="gmail-m_5094786850249725712gmail-"><br>
> diff --git a/debian/control b/debian/control<br>
> index 7249e630..7b4bd21b 100644<br>
> --- a/debian/control<br>
> +++ b/debian/control<br>
> @@ -75,6 +75,22 @@ Description: Tool for building Debian binary packages<br>
> from Debian sources<br>
>   build-essential packages, plus those in the package build<br>
>   dependencies.<br>
><br>
> +Package: sbuild-debian-developer-setup<br>
<br>
</span>I guess you need the extra package for the apt-cacher-ng dependency? How about<br>
putting the script into the sbuild package and making apt-cacher-ng a<br>
Recommends?<br>
<span class="gmail-m_5094786850249725712gmail-"><br>
> +Architecture: all<br>
> +Depends: sbuild,<br>
> +         apt-cacher-ng,<br>
<br>
</span>Will this not also work with the apt-cacher package in the same way?<br></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +         lintian,<br>
<br>
I still don't understand why you need lintian as a runtime dependency. To run<br>
lintian, sbuild installs lintian *inside* the chroot. Lintian on the host<br>
running sbuild is never used.<br></blockquote><div><br></div><div>Ah, I didn’t realize that. Removed.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
You forgot a dependency on cron.<br></blockquote><div><br></div><div>Added.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Doesn't systemd not also provide a cron-replacement? Maybe you could provide a<br>
solution for that to and the depend on "cron | systemd-sysv"<br></blockquote><div><br></div><div>Isn’t a better way to do that to depend on the virtual package “cron-daemon”? Did that instead.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class="gmail-m_5094786850249725712gmail-"><br>
> +         ${misc:Depends},<br>
> +         ${perl:Depends},<br>
> +         ${shlibs:Depends}<br>
> +Description: Convenience script to set up an sbuild environment for Debian<br>
> Developers<br>
> + Run "sudo sbuild-debian-developer-setup" to add the current user to the<br>
> sbuild<br>
> + group, create an schroot for building packages for Debian unstable, and<br>
> create<br>
> + a cronjob which updates said schroot daily.<br>
> + .<br>
> + This script assumes you are on an un-metered internet connection (daily<br>
> schroot<br>
> + updates might be costly otherwise).<br>
<br>
</span>It's nice that you put "debian" in the package name. Why is "debian" not also<br>
part of the executable name?<br></blockquote><div><br></div><div>…but it is?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
How about letting the script take two optional arguments: distribution and<br>
suite. Those could default to "debian" and "unstable" and then downstreams<br>
could easily adjust these defaults.<br></blockquote><div><br></div><div>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?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
You should provide a man page to the script.<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
There should be at least the --help command line option which explains how to<br>
use the script.<br></blockquote><div><br></div><div>Done.</div></div><br clear="all"><div><br></div>-- <br><div class="gmail-m_5094786850249725712gmail_signature">Best regards,<br>Michael</div>
</div></div>