josch, friendly ping? :) On Fri, Jun 2, 2017 at 6:23 PM, Michael Stapelberg <stapelb...@debian.org> wrote:
> Thanks for the review. Answers inline: > > On Wed, May 24, 2017 at 11:03 AM, Johannes Schauer <jo...@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 > -- Best regards, Michael
