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

[Michael Stapelberg]

Thanks for the review. Answers inline:

On Wed, May 24, 2017 at 11:03 AM, Johannes Schauer <josch at debian.org> wrote:

> Hi,
>
> Quoting Geert Stappers (2017-05-21 08:43:31)
> > the debian/postinst now here inline
>
> thanks, that allows me to comment easily.
>
> > # Add to group sbuild all “dynamically allocated user accounts†; see
> > # https://www.debian.org/doc/debian-policy/ch-opersys.html
> > for user in $(getent passwd | perl -F: -nlE '$F[2] >= 1000 && $F[2] <=
> 59999 && say $F[0]')
> > do
> >     # Strictly speaking, we should use sbuild-adduser, but
> sbuild-adduser prints
> >     # setup instructions to STDERR which do not make sense in the
> context of
> >     # this package. Filtering STDERR is cumbersome, so we call adduser
> directly.
> >     adduser --quiet -- "$user" sbuild
> > done
>
> I'm doubtful whether adding *all* users with ids between 1000 and 59999 to
> the
> sbuild group is something sensible to do. This would give *all* these users
> instant access to schroot-based chroots. I don't think this is something
> anybody expects when running a script, much less something that anybody
> expects
> to happen in a package's postinstall.
>
> > # Create a chroot if it does not already exist
> > chroot="unstable-$(dpkg --print-architecture)-sbuild"
> > if ! schroot -i -c chroot:${chroot} >/dev/null 2>&1
> > then
> >     sbuild-createchroot \
> >         --include=eatmydata,ccache,gnupg \
>
> Why include ccache? I would not expect that a single build will compile the
> same source multiple times. Without bind-mounting the cache directory from
> the
> outside this will probably not help much.
>
> Why include gnupg?
>

This was copy&pasted from the wiki page. Removed.


>
> >         unstable \
> >         /srv/chroot/${chroot} \
> >         http://deb.debian.org/debian
>
> Why not let this new package depend on apt-cacher-ng and let
> http://127.0.0.1:3142/deb.debian.org/debian be the default mirror?
>

Done.


>
> >     # At this point, sbuild-createchroot created an schroot
> configuration with a
> >     # random suffix, e.g. /etc/schroot/chroot.d/unstable-amd64-sbuild-pyViYe.
> As
> >     # sbuild-createchroot recommends, we rename that file before making
> >     # adjustments.
> >     mv /etc/schroot/chroot.d/${chroot}-* /etc/schroot/chroot.d/${chroot}
>
> What happens with any existing /etc/schroot/chroot.d/${chroot}? You just
> overwrite it?
>
> What happens if more than one file match /etc/schroot/chroot.d/${
> chroot}-*?
>
> > fi
> >
> > # schroot config customizations:
> > config=/etc/schroot/chroot.d/${chroot}
> > tmp=$(mktemp ${config}-XXXXXX.dpkg-tmp)
> > trap 'rm -f "${tmp}"' TERM INT EXIT QUIT
> >
> > grep -v -E '^(aliases|command-prefix)=' "${config}" > "${tmp}"
> >
> > # For convenience, treat UNRELEASED as an alias for unstable (so that
> > # debian/changelog files containing UNRELEASED do not need to be
> modified before
> > # building). Also sid, because it is short to type when specifying -d.
> > echo "aliases=UNRELEASED,sid" >> "${tmp}"
>
> You can achieve the same effect using sbuild-createchroot by using the
> --alias
> option. No need to manually mangle the schroot config.
>

Done.


>
> > # Enable eatmydata: occasionally losing a test build is preferable over
> longer
> > # build times and disk wear.
> > echo "command-prefix=eatmydata" >> "${tmp}"
>
> I don't see sbuild-createchroot mangling with command-prefix at all. So
> why do
> you filter it out initially?
>
> Would it not be better to add a command line option to sbuild-createchroot
> which allows setting a custom command-prefix? Then you would not need to
> manually mangle the created config file at all and lots of the problems I
> mentioned above and other fragile things would go away.
>

Sure. Can you take care of this or do you want me to send a patch?


>
> > chmod 644 "${tmp}"
> > mv "${tmp}" "${config}"
> >
> > # Copy a modified example sbuildrc config file
> > for homedir in $(getent passwd | perl -F: -nlE '$F[2] >= 1000 && $F[2]
> <= 59999 && say $F[5]')
> > do
> >     userconfig="${homedir}/.sbuildrc"
> >     if [ ! -e "${userconfig}" ]
> >     then
> >         (grep -v -E "^(# don't remove this|1;\$)" /usr/share/doc/sbuild/examples/example.sbuildrc
> && cat /usr/share/doc/sbuild-debian-setup/sbuildrc) > "${userconfig}"
> >     fi
> > done
>
> Instead of crafting a custom sbuildrc, we can also talk about changing the
> existing defaults.
>
> For example you set "$build_arch_all = 1;" I can certainly see how it makes
> sense to change this default for sbuild but keep it at 0 for buildd.
>

Sounds good. Same question: can you take care of this, or should I send a
patch?


>
> Then you set "$build_source = 1;" but I don't know why you would need this.
> Sbuild is not there to build the source package. The source package is its
> input not its output. Why would you want sbuild to build it again?
>

The wiki page contained some confusing advice, which I now removed.


>
> Lastly, you set "$run_lintian = 1;" which I also think would be a sensible
> thing to change in the defaults.
>

Sounds good, see above.


>
> As a result, you would not need to manually craft a custom sbuildrc
> anymore.
>
> > # bind-mount the apt archive cache into chroots, so that packages are
> downloaded
> > # only once. The assumption is that users will not typically have a
> local apt
> > # mirror or caching proxy.
> > if ! grep -q '^/var/cache/apt/archives' /etc/schroot/sbuild/fstab
> > then
> >     echo "/var/cache/apt/archives /var/cache/apt/archives none rw,bind 0
> 0" \
> >          >>/etc/schroot/sbuild/fstab
> > fi
>
> And who cleans up /var/cache/apt/archives regularly when it becomes
> cluttered
> over time?
>
> I also don't like the idea of just randomly mixing the
> /var/cache/apt/archives
> directory of the host with multiple sbuild instances which might be
> running at
> the same time. Just imagine sbuild running for different distributions
> which
> have packages with the same name and version but different content. That is
> just calling for trouble.
>
> I think a much better option is to use something like apt-cacher-ng. This
> new
> package could easily depend on it and then use that as its default mirror
> as
> already explained above.
>

Done.


>
> > if [ ! -e "/etc/schroot/setup.d/04tmpfs" ]
> > then
> >     echo ""
> >     echo "  If you can spare the RAM, you can enable building in tmpfs
> using:"
> >     echo ""
> >     echo "    sudo ln -s /etc/schroot/setup-available.d/overlays-in-tmpfs
> /etc/schroot/setup.d/04tmpfs"
> >     echo ""
> > fi
>
> This will affect *all* schroot sessions and not just those used by sbuild.
> As
> such, this should rather go into documentation for schroot. But maybe
> there is
> a way to make this mounting specific to sbuild schroots?
>

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.


>
> I think in summary much of what this script does can be achieved by:
>
>  - improving existing documentation
>

I updated the wiki page based on your review.


>  - changing configuration defaults to more sensible values
>  - adding more options to existing tools like sbuild-createchroot
>  - using existing tools instead of using creative ways to work around them
>    (apt-cacher-ng)
>
> If we do all that, what is left that a new script should take care of?
>

The following things are still left:

• Adding the user(s) to the sbuild group
• Creating the chroot with all the options we discussed
• Suggesting to build in tmpfs
• Periodically updating the schroots (not strictly speaking part of the
script itself, but listing it here anyway)


>
> Thanks!
>
> cheers, josch
>



-- 
Best regards,
Michael
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.alioth.debian.org/pipermail/buildd-tools-devel/attachments/20170602/63c1ae58/attachment.html>