[Merge] lp:~rbalint/livecd-rootfs/minimize-unminimize into lp:~vorlon/livecd-rootfs/minimizing

[Steve Langasek]

Review: Needs Fixing

Thanks, comments inline.

Diff comments:

> === modified file 'live-build/auto/build'
> --- live-build/auto/build	2017-05-09 03:15:14 +0000
> +++ live-build/auto/build	2017-05-09 10:23:04 +0000
> @@ -72,6 +72,62 @@
>  
>  		# Remove docs installed by bootstrap
>  		Chroot chroot dpkg-query -f '${binary:Package}\n' -W | Chroot chroot xargs apt-get install --reinstall
> +
> +                # Add unminimizer script which restores default image behavior
> +                mkdir -p chroot/usr/local/bin

let's put this in /usr/sbin.  sbin instead of bin because it only makes sense to run as root; /usr instead of /usr/local because it's part of the OS (even if not dpkg-managed), not something "owned" by the local admin.

Speaking of which, do you think we should prefer to put this in a package rather than directly on the filesystem?  Advantages:
 - dpkg -S works
 - no undetected path collisions
 - clearer how to remove the bits from the filesystem when you're done ('apt purge $pkg')
Disadvantages:
 - requires SRUing a new package back into each stable release where we want to support this

What do you think?

> +                cat > chroot/usr/local/bin/unminimize <<'EOF'
> +#!/bin/sh
> +
> +set -e
> +
> +TMP=$(mktemp -d)
> +cleanup() {
> +  if [ -f $TMP/excludes ]; then
> +    echo "Re-enabling installation of all documentation in dpkg failed." 1>&2

apologies in advance, but I'm going to nitpick some English here (and in subsequent strings).  Should be: "Failed to reconfigure dpkg to install documentation"

> +    mv $TMP/excludes /etc/dpkg/dpkg.cfg.d/excludes
> +  fi
> +  rm -rf "$TMP"
> +}
> +trap cleanup EXIT
> +
> +if [ -f /usr/sbin/update-initramfs.divert-minimize ]; then

I may have given you different guidance on this before, and if so, I apologize.  I think we should not restore update-initramfs as part of unminimize, because using an initramfs to boot is orthogonal to the interactive CLI experience of the user.  It is a generically-useful boot speed optimization to not use initramfs when we don't need it, therefore I think we should not reintroduce the initramfs as part of the unminimize command.

> +    echo "Re-enabling intiramfs..."
> +    rm -f /usr/sbin/update-initramfs
> +    dpkg-divert --remove --rename /usr/sbin/update-initramfs
> +    update-initramfs -u -k all
> +else
> +    echo "Initramfs is already enabled, skipping re-enabling it."
> +fi
> +
> +if [ -f /etc/dpkg/dpkg.cfg.d/excludes ]; then
> +    echo "Re-enabling installation of all documentation in dpkg..."
> +    mv /etc/dpkg/dpkg.cfg.d/excludes $TMP/excludes

Do we need to worry that the file contents may have been changed by the user?

I understand what you're trying to do here by mv'ing this file to $TMP, then moving it back in a cleanup function if something fails.  However, if the machine powers off mid-process, you will still be in the situation that 'unminimize' will be rendered a no-op, but the documentation will not be installed.

I suggest instead doing:

if [ -f /etc/dpkg/dpkg.cfg.d/excludes ] || [ -f /etc/dpkg/dpkg.cfg.d/excludes.dpkg-tmp ]; then
    echo "Re-enabling installation of all documentation in dpkg..."
    if [ -f /etc/dpkg/dpkg.cfg.d/excludes ]; then
        mv /etc/dpkg/dpkg.cfg.d/excludes /etc/dpkg/dpkg.cfg.d/excludes.dpkg-tmp
[...]

According to dpkg.cfg(5) this should be sufficient to cause dpkg to ignore the file.

Then once the apt-get reinstall has succeeded, you can just rm -f /etc/dpkg/dpkg.cfg.d/excludes.dpkg-tmp, and drop the cleanup function.

> +    echo "Updating package list and upgrading packages..."
> +    apt-get update
> +    apt-get upgrade
> +    echo "Installing missing documentation from packages..."
> +    (cd $TMP

This whole section seems rather convoluted.  Why not use 'apt-get install --reinstall $pkglist' instead of unpacking by hand?

While not an issue for our existing images, extracting to a temp location and then copying to /usr is also less correct in cases where selinux is enabled.  So I would like us to leverage the package manager directly as much as possible.

> +        echo "Downloading packages with files in /usr/share/man/ ..."
> +        dpkg -S /usr/share/man/ |sed 's|, |\n|g;s|: [^:]*$||' | xargs apt-get download
> +        echo "Downloading packages with missing files in /usr/share/doc/ .."
> +        dpkg --verify | awk '/\?\?5\?\?\?\?\?\?   \/usr\/share\/doc/ {print $2}' | sed 's|/[^/]*$||' | sort |uniq \
> +             | xargs dpkg -S | sed 's|, |\n|g;s|: [^:]*$||' | uniq | xargs apt-get download

This part is cleverly done, +1, and now I've learned about dpkg --verify.  I just think the end of the pipeline should be 'DEBIAN_FRONTEND=noninteractive apt-get install --reinstall -y'.

>From the documentation, I note that the other columns may at some later date have other output when dpkg starts to support them.  So to future-proof this, I recommend doing 'awk '/..5......   \/usr\/share\/doc/' instead of hard-coding the ? for each column.

Also, I guess you should be able to handle both /usr/share/man and /usr/share/doc with a single dpkg --verify command and avoid calling dpkg -S entirely.

> +        mkdir target
> +        echo "Extracting packages to temporary location..."
> +        for i in *.deb; do dpkg-deb -x $i target/; done
> +        echo "Restoring missing documentation on the system ..."

"Restoring system documentation ..."

(In general, avoid the word 'missing' which suggests an error rather than a deliberate design decision.)

> +        cp -rpn target/usr/share/man/* /usr/share/man/
> +        cp -rpn target/usr/share/doc/* /usr/share/doc/
> +        if dpkg --verify > /dev/null; then
> +           echo "Documentation is restored successfully."

"Documentation has been restored successfully."

> +           rm $TMP/excludes
> +        fi
> +    )
> +else
> +    echo "Dpkg already installs documentation, skipping re-enabling it."

Again, I think we should omit the message for the no-op case.

> +fi
> +EOF

We discussed that the unminimize script should not call 'locale-gen'.  But I wonder if it /should/ install the ubuntu-minimal metapackage.  We have previously defined ubuntu-minimal as the smallest system that provides the Ubuntu experience.  So when expanding a system for human use, it seems to me that we should pull this in.

> +                chmod +x chroot/usr/local/bin/unminimize
>          fi
>  
>  	Chroot chroot "dpkg-divert --quiet --add \
> 
> === modified file 'live-build/ubuntu-cpc/hooks/032-disk-image.binary'
> --- live-build/ubuntu-cpc/hooks/032-disk-image.binary	2017-05-09 03:15:14 +0000
> +++ live-build/ubuntu-cpc/hooks/032-disk-image.binary	2017-05-09 10:23:04 +0000
> @@ -131,6 +131,8 @@
>  		cat > mountpoint/usr/sbin/update-initramfs <<'EOF'
>  #! /bin/sh
>  echo "initramfs disabled on this system.  To reenable, run:" >&2
> +echo "  sudo unminimize" >&2
> +echo "or" >&2

If unminimize doesn't restore initramfs, then obviously this comment should also be dropped.

>  echo "  sudo rm -f /usr/sbin/update-initramfs" >&2
>  echo "  sudo dpkg-divert --remove --rename /usr/sbin/update-initramfs" >&2
>  exit 0


-- 
https://code.launchpad.net/~rbalint/livecd-rootfs/minimize-unminimize/+merge/323157
Your team Ubuntu Core Development Team is requested to review the proposed merge of lp:~rbalint/livecd-rootfs/minimize-unminimize into lp:~vorlon/livecd-rootfs/minimizing.