Jeff Schroeder
2008-Jun-27 23:13 UTC
[Ovirt-devel] [PATCH trivial] Create developer appliances under Ubuntu v2
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 DebianDONE> 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>
Jeff Schroeder
2008-Jun-27 23:46 UTC
[Ovirt-devel] [PATCH trivial] Create developer appliances under Ubuntu v2
On Fri, Jun 27, 2008 at 4:13 PM, Jeff Schroeder <jeffschroed at gmail.com> wrote: ...snip...> An updated patch is attached that hopefully addresses your concerns.Ok I changed something after testing the previous patch and flipped the logic for some stupid reason. This makes more sense and fixes the package dependency check. Please ignore the last email and take a look at this patch instead. -- 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: cleaned-up-patch-v3.patch Type: text/x-diff Size: 2018 bytes Desc: not available URL: <http://listman.redhat.com/archives/ovirt-devel/attachments/20080627/ac9a4f7c/attachment.bin>