Quoting Christian Hofstaedtler (2017-06-26 11:14:22) > * Johannes Schauer <jo...@debian.org> [170625 12:58]: > > If the pkg-ruby-extras team is interested in a review of the setup script, > > then > > I can put that into a separate mail. > > I think we'd be interested; mostly our scripts are "this has worked > for me" (where me is one or more of our team members), but they surely aren't > comprehensive.
Okay, then here are my comments about the script inline. They apply to sbuild in Debian Stretch > #!/bin/sh > > set -exu > > sudo apt-get install -qy \ > autopkgtest \ > build-essential \ > gem2deb \ > git \ > git-buildpackage \ > myrepos \ > quilt \ > sbuild \ > lxc \ > debci \ > "" -qy might not be a good choice here because that will also remove packages silently so that apt can install the package selection. Why is build-essential required if the build happens in sbuild? Then you'd just need dpkg-dev. > sudo mkdir -p /root/.gnupg # To work around #792100 Fixed since 0.66.0-1. Also note that gnupg isn't needed anymore by sbuild since sbuild 0.67.0-1 which also makes this unnecessary. > sudo sbuild-update --keygen Gnupg is not needed anymore since sbuild 0.67.0-1. > sudo sbuild-adduser $USER This should only be run once, so before running it, there should be a check whether the current user is already in the sbuild group. > chrootname=unstable-$(dpkg --print-architecture)-sbuild > chroot=/srv/chroots/$chrootname The sbuild-createchroot manpage suggests /src/chroot instead of /src/chroots. I don't think there is any good reason that makes one better than the other but I guess it would be good to just decide for one instead of using two very slightly different versions. > if schroot --list --all-source-chroots | grep unstable-amd64-sbuild; then This hardcodes amd64 but should instead use $(dpkg --print-architecture). The grep should also include the source: prefix. Also, the schroot command can fail (as it happened for Ross) in which case the script should abort. > : > else > sudo sbuild-createchroot unstable $chroot http://httpredir.debian.org/debian httpredir.debian.org has been discontinued and now redirects to deb.debian.org which should be used instead. > fi The whole check for whether the chroot already exists can be removed once bug #866006 gets fixed. > for conf in $(grep -l '^union-type=' /etc/schroot/chroot.d/*-sbuild*); do > if ! grep -q "^union-overlay-directory=" "$conf" ; then > echo 'union-overlay-directory=/dev/shm' | sudo tee --append "$conf" > fi > done It might be dangerous to force /dev/shm usage for users with not many gigabytes of RAM. It might be better to give users the choice and mount /var/lib/schroot/union/overlay as tmpfs instead via fstab: tmpfs /var/lib/schroot/union/overlay tmpfs nosuid,size=8G 0 0 This would then also allow users to choose the tmpfs size depending on their local setup. Also, it is questionable whether /dev/shm should be used in the first place because since Debian 7, /run/shm is the new location. Also, the first check succeeds even if union-type=none is given which should not happen. It is also questionable whether this code should really modify all configs matching that glob or whether it should really only operate on the schroot config that was created by the sbuild-createchroot call made earlier by this script. > if ! grep -q /var/cache/apt/archives /etc/schroot/sbuild/fstab; then > sudo sh -c 'echo /var/cache/apt/archives /var/cache/apt/archives none > rw,bind 0 0 >>/etc/schroot/sbuild/fstab' > fi This should not be done. /var/cache/apt/archives is an implementation detail of apt. Multiple apts should not share the same directory. Also imagine situations in which sbuild operates on a Debian derivative with the same package names and versions but different package contents. Then, a package build would ultimately confuse the system apt. Or imagine multiple apts trying to read from the directory while another one tries to write to it or delete from it. Sharing this directory completely circumvents the locks set by apt which usually prevent this kind of mess. Instead of this hack, you could use apt-cacher or apt-cacher-ng. > # configure lxc networking if needed > if grep -q '^lxc.network.type\s*=\s*empty' /etc/lxc/default.conf; then > sudo apt-get install -qy libvirt-clients libvirt-daemon-system iptables > ebtables dnsmasq-base > if ! (sudo virsh net-list | grep -q default); then > sudo virsh net-start default > sudo virsh net-autostart default > fi > sudo sed -i -e '/lxc.network.type/d' /etc/lxc/default.conf > sudo tee --append /etc/lxc/default.conf <<EOF > lxc.network.type = veth > lxc.network.link = virbr0 > lxc.network.flags = up > EOF > sudo tee /etc/sudoers.d/lxc <<EOF > %sudo ALL = NOPASSWD:SETENV: /usr/bin/lxc-*, /usr/bin/timeout > EOF > > fi > > sudo debci setup I'm neither familiar enough with lxc nor debci to be able to comment on this part of the script. Thanks! cheers, josch
signature.asc
Description: signature
