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
2 Answers 2
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.
-
\$\begingroup\$ Thank you. 1. Got it. 2. Its just there for me as i learn - so i can go back and remember what i was doing. But point taken. Thanks. 3. Got it. 4. good point. thanks. 5. Got it. 6. Got it. 7. This is purely for me, but i understand and appreciate the feedback. 8. That is really all i was trying to do in this script. 9. Got it. \$\endgroup\$TheWonkyTwit– TheWonkyTwit2020年02月20日 21:34:42 +00:00Commented Feb 20, 2020 at 21:34
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
-
\$\begingroup\$ Thank you. 1. yes i just left that out when i pasted it in. 2. I need to dig in to this and i am not sure i understand. 3. Got it. 4. Got it. \$\endgroup\$TheWonkyTwit– TheWonkyTwit2020年02月20日 21:32:11 +00:00Commented Feb 20, 2020 at 21:32
-
\$\begingroup\$ For item 2, obviously read the manual, but also look at BashFAQ/105 for some pitfalls. Some argue that the pitfalls make
set -e
useless; I disagree, but add that it needs to be well understood to be effective and useful. \$\endgroup\$Toby Speight– Toby Speight2020年02月21日 09:14:57 +00:00Commented Feb 21, 2020 at 9:14
sudo
for a user if they haven't explicitly granted it (by running the script viasudo
in the first place), and it's possible the user would prefer a different installation mechanism, such as compiling from source. \$\endgroup\$