[Ovirt-devel] [PATCH trivial] Create developer appliances under Ubuntu

[Jim Meyering]

"Jeff Schroeder" <jeffschroed at gmail.com> wrote:
> This small patch allows you to create a wui-appliance developer iso
> image on Ubuntu 8.04.
...
> diff --git a/wui-appliance/create-wui-appliance.sh b/wui-appliance/create-wui-appliance.sh
> index 116d572..ac13805 100755
> --- a/wui-appliance/create-wui-appliance.sh
> +++ b/wui-appliance/create-wui-appliance.sh
...
>  # now make sure the packages we need are installed
> -rpm -q libvirt -q kvm -q virt-manager -q virt-viewer >& /dev/null
> -if [ $? -ne 0 ]; then
> +if [ -e /etc/redhat-release ]; then
> +    PACKAGES="libvirt kvm virt-manager virt-viewer"
> +    CHECK=$(rpm $(printf " -q %s " "$PACKAGES")  &> /dev/null; echo $?)
> +    KVM_BINARY=/usr/bin/qemu-kvm
> +elif [ -e /etc/debian_version ]; then
> +    # Works in Ubuntu 8.04. Still needs testing in Debian
> +    PACKAGES="libvirt0 libvirt-bin kvm qemu virt-manager virt-viewer"
> +    CHECK=$(dpkg -l $PACKAGES &> /dev/null; echo $?)
> +    KVM_BINARY=/usr/bin/kvm
> +else
> +    die "Not a supported system"
> +fi 
> +
> +if [ $CHECK -ne 0 ]; then
>      # one of the previous packages wasn't installed; bail out
>      die "Must have the libvirt, kvm, virt-manager, and virt-viewer packages installed"
>  fi

Hi Jeff

A minor detail: &> is a bashism, so better avoided:
    $ bash -c '(echo bar 1>&2) &> foo'
    $ dash -c '(echo bar 1>&2) &> foo'
    bar
Use "> /dev/null 2>&1" instead.

A naming suggestion: call the new variable $KVM, since
that file needn't be a binary.

An efficiency/readability suggestion: don't fork a whole new shell
for $CHECK: (which could be usefully renamed)
Also, no need to "echo $?".

  dpkg -l $PACKAGES > /dev/null 2>&1; have_required_packages=$?

Very minor readability nit: use single quotes for literals
so readers don't wonder if you used double quotes because there are
$var or `...` expansions:

  PACKAGES='libvirt0 libvirt-bin kvm qemu virt-manager virt-viewer'

maintainability: since this comment is obviously
intended to be temporary,

    # Works in Ubuntu 8.04. Still needs testing in Debian

Mark it with something like "FIXME" so it will less likely to be forgotten.

    # Works in Ubuntu 8.04. FIXME: test in Debian

Finally, please don't add trailing blanks (after "die...\nfi").