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

[Jeff Schroeder]

On Fri, Jun 27, 2008 at 11:37 AM, Jim Meyering <jim at meyering.net> wrote:
> "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.

I was initially going to remove old bashims and was quite forcefully shot down.
Also I was following suit as per the rest of the script. Since the
script calls bash
in the shebang this is still portable. You might want to speak with
danpb about that one...
DONE

> A naming suggestion: call the new variable $KVM, since
> that file needn't be a binary.
Not a bad idea, but is there ever a time qemu-kvm would not be a
binary that is valid libvirt xml?
DONE

> 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=$?
DONE

>
> 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,
DONE

>
>    # 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
DONE

> Finally, please don't add trailing blanks (after "die...\nfi").
Again following existing coding standards already in the script.
DONE

An updated patch is attached that hopefully addresses your concerns.

Besides addressing your comments, the only real change was making the error
message when you are missing the packages dynamic. It uses $PACKAGES now
instead of hardcoding the list of Fedora packages.

There are a bunch of small patches in a git tree at home (where the
network is currently dead).
Expect a flurry of very small cleanup patches later tonight.

Thanks for the review

-- 
Jeff Schroeder

Don't drink and derive, alcohol and analysis don't mix.
http://www.digitalprognosis.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Addressing-comments-from-Jim-Meyering.patch
Type: text/x-diff
Size: 2432 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/ovirt-devel/attachments/20080627/bb9a6190/attachment.bin>