Problem
Okay all you Linux/shell folks out there. This is my first shell script (2nd if you count “Hello World”) and would appreciate some feedback.
Basically, I need to test whether htop
is installed; if it is, then move on; if it is not, then install it. If the user is root then just do it, and if not, sudo and install it. I have “borrowed” code from here to stitch this together. It works in my test environment. I would like to know:
What did I miss? What other variables do I need to account for? Are there better ways to go about this?
if ! [ -x "$(command -v htop)" ]; then
# set the var SUDO to "" (blank)
SUDO=""
# if the user ID is not 0 (root is 0)
if [ $EUID != "0" ] ; then
# set the var SUDO to "sudo"
SUDO="sudo"
fi
# Let the user know it is not installed & then install it, using sudo if not root
echo 'Error: htop is not installed.' && $SUDO apt install htop >&1
# exit the script
exit 0
fi
Solution
Overall it’s fine. My suggestions would be:
- Fix the indenting.
- If you find the comments helpful, great. Otherwise, consider saying “why” and not “what”, e.g.
# set the var SUDO to "" (blank)
-># This will be used as a prefix to run a command as root
- You can alternatively consider checking if the command exists by running it:
if htop --version > /dev/null 2>&1
. This does not discriminate against aliases, functions and builtins. - Prefer user lowercase variable names to avoid conflicting with existing environment variables.
SUDO="sudo"
is ok sinceSUDO
isn’t in use, but if you had tried the same withPWD="pwd"
orSHELL="sh"
you could have gotten some odd side effects. - Use
echo 'Error: htop is not installed.' >&2
to write the error message to stderr - The
>&1
aka1>&1
is redundant: it just redirects the FD to where it’s already going. - It’s generally considered rude to install packages without asking the user. A more canonical approach is to simply exit with an error saying that
htop
is missing and leave it to the user to install it (this also avoids tying the script to Ubuntu/Mint). - If you continue when
htop
exists, why is the script exiting after installinghtop
? If it’s to handle installation failures due to bad password or full disk, you should probably handle that explicitly. - You can use
exit
akaexit $?
to exit with the status of the previous command, so that if the installation failed, you don’t claim that the script ran successfully.
In addition to the excellent answer by that other guy:
-
It’s a good idea to start the script with a shebang specifying which interpreter to run. In this case, we’re not using any Bash features not present in standard POSIX shell, so we can write
#!/bin/sh
-
Consider setting the flags
-u
(using unset variables is an error) and-e
(exit when commands fail and are not otherwise tested):set -eu
-
It’s unusual to follow
echo
with&&
– we’re not depending on its success, so just use;
. -
Don’t exit with status 0 (success) when
apt
fails. The easy way to return its exit value is to replace the shell process usingexec
, like this:exec $SUDO apt install htop # this line not reached