How can I improve this BASH script?

dugan · 11-23-2014, 06:48 PM

I know there are some shell experts (which I'm not) here. How can I improve this BASH script?

It's named bottle-run.sh

And here's an actual command-line that it will need to support:

Code:

setarch i386 -3 bottle-run D3 wine ~/.local/share/wineprefixes/D3/drive_c/Program\ Files/Diablo\ III/Diablo\ III.exe

Here is the script, as I currently have it:

Code:

#!/bin/sh

if [ "$#" -lt 2 ]; then
    echo Usage: bottle-run bottle command \[args\]
    echo E.g. bottle-run PvZ winecfg
    echo e.g. bottle-run PvZ wine notepad
    echo e.g. bottle-run PvZ wine iexplore http://www.google.ca/
    echo e.g. bottle-run PvZ winetricks --no-isolate steam
    exit 1
fi

# Will deal with this repetition later.

if [ ! -d "$HOME/.local/share/wineprefixes/$1" ]; then
    echo "$1 is not a bottle."
    echo Usage: bottle-run bottle command \[args\]
    echo E.g. bottle-run PvZ winecfg
    echo e.g. bottle-run PvZ wine notepad
    echo e.g. bottle-run PvZ wine iexplore http://www.google.ca/
    echo e.g. bottle-run PvZ winetricks --no-isolate steam
    exit 1
fi

RUNRC="$HOME/.local/share/wineprefixes/$1/bin/runrc"

if [ ! -f "$RUNRC" ];  then
    echo "$RUNRC not found."
    echo "Please use mkbottle to recreate the bottle."
    exit 1
fi

# If we already have an environment loaded, clear it out.
if [ -f "$WINEPREFIX/bin/uncorkrc.sh" ]; then
    . "$WINEPREFIX/bin/uncorkrc.sh"
fi

. "$HOME/.local/share/wineprefixes/$1/bin/runrc.sh"

# This works as long as you have at most 7 command-line parameters.
# Let me know if there's a better solution (which works if the
# parameters include paths with spaces).
"$2" "$3" "$4" "$5" "$6" "7" "$8" "$9"

In particular I'm interested in trimming down the repetition and making the last line less of a hack.

I want it to support BASH, DASH and ZSH, so no bashisms please.

With the example command-line above, the last line of the script will expand to:

Code:

wine ~/.local/share/wineprefixes/D3/drive_c/Program\ Files/Diablo\ III/Diablo\ III.exe

ntubski · 11-23-2014, 07:28 PM

shell functions are available in POSIX shell

Code:

usage() {
    echo Usage: bottle-run bottle command \[args\]
    echo E.g. bottle-run PvZ winecfg
    echo e.g. bottle-run PvZ wine notepad
    echo e.g. bottle-run PvZ wine iexplore http://www.google.ca/
    echo e.g. bottle-run PvZ winetricks --no-isolate steam
    exit 1
}
if [ "$#" -lt 2 ]; then
    usage
fi
...

For the last line, you want an array. sh doesn't support arrays; except it sort of has a single instance of an array: the command line parameters in "$@".

Code:

shift # drop the first arg
"$@" # invoke the rest

Also, it seems you forgot to actually use the RUNRC variable?

dugan · 11-24-2014, 12:02 AM

1. Yes, I forgot to use the RUNRC variable. Thanks for catching that.

2. I didn't know about shift. Perfect. Thanks!

grail · 11-24-2014, 03:20 AM

Well I have a few ideas which may or may not help:

1. quote everything - not always needed but in the case where you have escapes in your echoes they would not be needed with quotes

2. as above - use functions

3. I personally find here-documents are much cleaner than several echo statements

4. you source runrc.sh but never test it exists?

5. is number 7 not supposed to have a $ sign in front of it (maybe typo)

6. why does it have to accept a path with spaces when this could be simply quoted on the command line??

7. you do not test to see if WINEPREFIX is set, so if we assume it is not and /bin/uncorkrc.sh does exist, you will now possibly source the wrong file

8. Instead of passing $1 around everywhere, assign it to a variable early so it is well named and people can follow what it is ... think in the future when this script may be 100s of lines long

dugan · 11-24-2014, 09:05 AM

Thanks you two. This is what I have now:

Code:

#!/bin/sh


if [ "$#" -lt 2 ]; then
	cat <<- EOF
	Usage: bottle-run bottle command [args]
	E.g. bottle-run PvZ winecfg
	e.g. bottle-run PvZ wine notepad
	e.g. bottle-run PvZ wine iexplore http://www.google.ca/
	e.g. bottle-run PvZ winetricks --no-isolate steam
	EOF
    exit 1
fi

BOTTLE=$1

if [ ! -d "$HOME/.local/share/wineprefixes/$BOTTLE" ]; then
	cat <<- EOF
	Bottle "$BOTTLE" not found"
	Run "lsp" to list bottles
	Run "bottle" to create bottles
	EOF
	exit 1
fi

RUNRC="$HOME/.local/share/wineprefixes/$BOTTLE/bin/runrc.sh"

if [ ! -f "$RUNRC" ];  then
	cat <<- EOF
	Path not found: $RUNRC
	Please use "bottle" to (re)create the bottle "$BOTTLE".
	EOF
	exit 1
fi

# If we already have an environment loaded, clear it out.
if [ "$WINEPREFIX" != "" ] &&  [ -f "$WINEPREFIX/bin/uncorkrc.sh" ]; then
    . "$WINEPREFIX/bin/uncorkrc.sh"
fi

. "$RUNRC"

shift 1
"$@"

grail · 11-24-2014, 07:44 PM

Cool

So now we are down to nit picking ... meant to be positive and as always up to you

1. Your first test is numerical but then you have quoted $# ... i know i know ... I said quote everything ... like i said, nit picky. When it comes to numbers I try and leave them as numbers and in bash I would then specifically use (()) but i know we can't here

2. You use a consistent path to wineprefixes, so again I would assign to a variable. My thoughts here are that should it change in the future you can edit in a single place

3. Instead of != "" you could simply use -n

4. shift on its own will default to shift 1

Like I said, mostly personal preference ... i like the new look better though

dugan · 11-25-2014, 01:06 AM

I decided that the project is complete enough to announce:

https://github.com/duganchen/wine_env

More code reviews are welcome, of course.

grail · 11-25-2014, 01:49 AM

Nice work

I would probably like to see you utilise some of the same features above in your other scripts.

I had a quick look at bottler.sh and seeing you are using this to source functions to be used later, it might be nice to have exit values to advise users
about success or failure should they try to make their own scripts using yours.

For example, in the bottle function in option 2 under the else clause and also all other numbers greater than 2, these options would be an error.
However, should the user place it in a script, the last command executed is an echo which will return success (just a thought)

I may be tempted to use though as wine has often been a bit of a bug bear for me so I shy away and generally find alternatives on a software level, but for games I would like to use my main machine