<div dir="ltr">Thanks for the review. Answers inline:<div class="gmail_extra"><br><div class="gmail_quote">On Wed, May 24, 2017 at 11:03 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br>
<br>
Quoting Geert Stappers (2017-05-21 08:43:31)<br>
<span class="">> the debian/postinst now here inline<br>
<br>
</span>thanks, that allows me to comment easily.<br>
<br>
> # Add to group sbuild all “dynamically allocated user accounts†; see<br>
<span class="">> # <a href="https://www.debian.org/doc/debian-policy/ch-opersys.html" rel="noreferrer" target="_blank">https://www.debian.org/doc/<wbr>debian-policy/ch-opersys.html</a><br>
> for user in $(getent passwd | perl -F: -nlE '$F[2] >= 1000 && $F[2] <= 59999 && say $F[0]')<br>
> do<br>
> # Strictly speaking, we should use sbuild-adduser, but sbuild-adduser prints<br>
> # setup instructions to STDERR which do not make sense in the context of<br>
> # this package. Filtering STDERR is cumbersome, so we call adduser directly.<br>
> adduser --quiet -- "$user" sbuild<br>
> done<br>
<br>
</span>I'm doubtful whether adding *all* users with ids between 1000 and 59999 to the<br>
sbuild group is something sensible to do. This would give *all* these users<br>
instant access to schroot-based chroots. I don't think this is something<br>
anybody expects when running a script, much less something that anybody expects<br>
to happen in a package's postinstall.<br>
<span class=""><br>
> # Create a chroot if it does not already exist<br>
> chroot="unstable-$(dpkg --print-architecture)-sbuild"<br>
> if ! schroot -i -c chroot:${chroot} >/dev/null 2>&1<br>
> then<br>
> sbuild-createchroot \<br>
> --include=eatmydata,ccache,<wbr>gnupg \<br>
<br>
</span>Why include ccache? I would not expect that a single build will compile the<br>
same source multiple times. Without bind-mounting the cache directory from the<br>
outside this will probably not help much.<br>
<br>
Why include gnupg?<br></blockquote><div><br></div><div>This was copy&pasted from the wiki page. Removed.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> unstable \<br>
> /srv/chroot/${chroot} \<br>
> <a href="http://deb.debian.org/debian" rel="noreferrer" target="_blank">http://deb.debian.org/debian</a><br>
<br>
</span>Why not let this new package depend on apt-cacher-ng and let<br>
<a href="http://127.0.0.1:3142/deb.debian.org/debian" rel="noreferrer" target="_blank">http://127.0.0.1:3142/deb.<wbr>debian.org/debian</a> be the default mirror?<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> # At this point, sbuild-createchroot created an schroot configuration with a<br>
> # random suffix, e.g. /etc/schroot/chroot.d/<wbr>unstable-amd64-sbuild-pyViYe. As<br>
> # sbuild-createchroot recommends, we rename that file before making<br>
> # adjustments.<br>
> mv /etc/schroot/chroot.d/${<wbr>chroot}-* /etc/schroot/chroot.d/${<wbr>chroot}<br>
<br>
</span>What happens with any existing /etc/schroot/chroot.d/${<wbr>chroot}? You just<br>
overwrite it?<br>
<br>
What happens if more than one file match /etc/schroot/chroot.d/${<wbr>chroot}-*?<br>
<span class=""><br>
> fi<br>
><br>
> # schroot config customizations:<br>
> config=/etc/schroot/chroot.d/$<wbr>{chroot}<br>
> tmp=$(mktemp ${config}-XXXXXX.dpkg-tmp)<br>
> trap 'rm -f "${tmp}"' TERM INT EXIT QUIT<br>
><br>
> grep -v -E '^(aliases|command-prefix)=' "${config}" > "${tmp}"<br>
><br>
> # For convenience, treat UNRELEASED as an alias for unstable (so that<br>
> # debian/changelog files containing UNRELEASED do not need to be modified before<br>
> # building). Also sid, because it is short to type when specifying -d.<br>
> echo "aliases=UNRELEASED,sid" >> "${tmp}"<br>
<br>
</span>You can achieve the same effect using sbuild-createchroot by using the --alias<br>
option. No need to manually mangle the schroot config.<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> # Enable eatmydata: occasionally losing a test build is preferable over longer<br>
> # build times and disk wear.<br>
> echo "command-prefix=eatmydata" >> "${tmp}"<br>
<br>
</span>I don't see sbuild-createchroot mangling with command-prefix at all. So why do<br>
you filter it out initially?<br>
<br>
Would it not be better to add a command line option to sbuild-createchroot<br>
which allows setting a custom command-prefix? Then you would not need to<br>
manually mangle the created config file at all and lots of the problems I<br>
mentioned above and other fragile things would go away.<br></blockquote><div><br></div><div>Sure. Can you take care of this or do you want me to send a patch?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> chmod 644 "${tmp}"<br>
> mv "${tmp}" "${config}"<br>
><br>
> # Copy a modified example sbuildrc config file<br>
> for homedir in $(getent passwd | perl -F: -nlE '$F[2] >= 1000 && $F[2] <= 59999 && say $F[5]')<br>
> do<br>
> userconfig="${homedir}/.<wbr>sbuildrc"<br>
> if [ ! -e "${userconfig}" ]<br>
> then<br>
> (grep -v -E "^(# don't remove this|1;\$)" /usr/share/doc/sbuild/<wbr>examples/example.sbuildrc && cat /usr/share/doc/sbuild-debian-<wbr>setup/sbuildrc) > "${userconfig}"<br>
> fi<br>
> done<br>
<br>
</span>Instead of crafting a custom sbuildrc, we can also talk about changing the<br>
existing defaults.<br>
<br>
For example you set "$build_arch_all = 1;" I can certainly see how it makes<br>
sense to change this default for sbuild but keep it at 0 for buildd.<br></blockquote><div><br></div><div>Sounds good. Same question: can you take care of this, or should I send a patch?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Then you set "$build_source = 1;" but I don't know why you would need this.<br>
Sbuild is not there to build the source package. The source package is its<br>
input not its output. Why would you want sbuild to build it again?<br></blockquote><div><br></div><div>The wiki page contained some confusing advice, which I now removed.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Lastly, you set "$run_lintian = 1;" which I also think would be a sensible<br>
thing to change in the defaults.<br></blockquote><div><br></div><div>Sounds good, see above.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
As a result, you would not need to manually craft a custom sbuildrc anymore.<br>
<span class=""><br>
> # bind-mount the apt archive cache into chroots, so that packages are downloaded<br>
> # only once. The assumption is that users will not typically have a local apt<br>
> # mirror or caching proxy.<br>
> if ! grep -q '^/var/cache/apt/archives' /etc/schroot/sbuild/fstab<br>
> then<br>
> echo "/var/cache/apt/archives /var/cache/apt/archives none rw,bind 0 0" \<br>
> >>/etc/schroot/sbuild/fstab<br>
> fi<br>
<br>
</span>And who cleans up /var/cache/apt/archives regularly when it becomes cluttered<br>
over time?<br>
<br>
I also don't like the idea of just randomly mixing the /var/cache/apt/archives<br>
directory of the host with multiple sbuild instances which might be running at<br>
the same time. Just imagine sbuild running for different distributions which<br>
have packages with the same name and version but different content. That is<br>
just calling for trouble.<br>
<br>
I think a much better option is to use something like apt-cacher-ng. This new<br>
package could easily depend on it and then use that as its default mirror as<br>
already explained above.<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> if [ ! -e "/etc/schroot/setup.d/04tmpfs" ]<br>
> then<br>
> echo ""<br>
> echo " If you can spare the RAM, you can enable building in tmpfs using:"<br>
> echo ""<br>
> echo " sudo ln -s /etc/schroot/setup-available.<wbr>d/overlays-in-tmpfs /etc/schroot/setup.d/04tmpfs"<br>
> echo ""<br>
> fi<br>
<br>
</span>This will affect *all* schroot sessions and not just those used by sbuild. As<br>
such, this should rather go into documentation for schroot. But maybe there is<br>
a way to make this mounting specific to sbuild schroots?<br></blockquote><div><br></div><div>I don’t know, you’re the expert :). I think users who know what schroot is (to me it’s “the thing which sbuild uses”) and who use it for anything but sbuild will realize what’s happening here. We could add some wording to that extent.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
I think in summary much of what this script does can be achieved by:<br>
<br>
- improving existing documentation<br></blockquote><div><br></div><div>I updated the wiki page based on your review.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- changing configuration defaults to more sensible values<br>
- adding more options to existing tools like sbuild-createchroot<br>
- using existing tools instead of using creative ways to work around them<br>
(apt-cacher-ng)<br>
<br>
If we do all that, what is left that a new script should take care of?<br></blockquote><div><br></div><div>The following things are still left:</div><div><br></div><div>• Adding the user(s) to the sbuild group</div><div>• Creating the chroot with all the options we discussed</div><div>• Suggesting to build in tmpfs</div><div>• Periodically updating the schroots (not strictly speaking part of the script itself, but listing it here anyway)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Thanks!<br>
<br>
cheers, josch<br>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature" data-smartmail="gmail_signature">Best regards,<br>Michael</div>
</div></div>