Shell install.sh for repository (rpi_videoloop)

Question

I've written my first installation script in shell and was wondering on how I can improve the install procedure to make it more clean?

Here's the code:

#!/bin/bash
#Variables
DEPENDENCIES="omxplayer screen cron usbmount ntfs-3g"
PTH="/home/pi/rpi_videoloop"
INIT="/etc/init.d"
SCRIPT="videoloop_v2.sh"
CONTROLLER="vid_controller"
USBCONF="usbmount.conf"
CRONSTUFF=$(cat cron.txt)
# Checking | Installing Dependancies
if ! apt list "$DEPENDENCIES" | grep -v installed | grep -E 'omxplayer|screen|cron|usbmount|ntfs-3g' > /dev/null
then
echo "Dependencies already installed. Continuing."
else
echo "Installing dependencies."
apt-get update
apt-get install "$DEPENDENCIES" -y
fi
echo "Installing Configurations..."
#Configuring
if [ ! -d "$PTH" ]; then 
 mkdir $PTH
fi
cp $SCRIPT $PTH
if [ ! -f $INIT/$CONTROLLER ]; then
 sudo cp $CONTROLLER $INIT
fi
cp $USBCONF /etc/usbmount/
sudo chmod 755 $PTH/$SCRIPT
sudo chmod 755 $INIT/$CONTROLLER
sudo update-rc.d $CONTROLLER defaults
sudo echo -n " consoleblank=10" | sudo tee -a /boot/cmdline.txt
#cat alias.txt >> ~/.bashrc
#sudo cat alias.txt >> ~/.bashrc
(crontab -u pi -l; echo "$CRONSTUFF" ) | crontab -u pi -
echo "FINISHED INSTALLATION: Service control -> /etc/init.d/vid_controller {start|stop|check|repair}"

Answer 1

Pointless sudo

When some dependencies are missing, the script installs them using apt-get install, without sudo. This suggests that the script will be run as root, otherwise it would not work. In that case all the sudo are unnecessary.

It would be good to add a check at the top of the script if it's being executed as root, and if that's not the case then warn the user and exit with error.

If the script is in fact not running as root, then I recommend to make it so, by running it with sudo script.

Such trickery also becomes unnecessary:

sudo echo -n " consoleblank=10" | sudo tee -a /boot/cmdline.txt

You can simply write:

echo " consoleblank=10" >> /boot/cmdline.txt

I'm not sure why you used echo -n. It's a good practice to avoid all flags of echo, because they are not portable. If it's really important to strip the trailing newline, then consider using printf instead, if you don't mind that it's not POSIX compliant.

Checking if a package is installed or not

My version of man apt shows that the list sub-command is "(work-in-progress)". And if I run any commands on it, for example apt list | grep -q ., it prints a warning:

WARNING: apt does not have a stable CLI interface. Use with caution in scripts.

As such it's not a good idea to use in scripts. Maybe your version is newer. For maximum robustness, I would rather use more stable techniques, such as dpkg -s. One caveat is that checking multiple packages is not trivial, so it's better to check them in a loop. If any one package is not found then you can lazily fall back to apt-get install "$DEPENDENCIES" -y.

Always double-quote variables used in command parameters

This is almost correct:

if [ ! -d "$PTH" ]; then 
 mkdir $PTH
fi

The if condition correctly double-quotes $PTH, but then the mkdir command doesn't. Even if you know that some path will never contain unsafe characters, it's good to always double-quote to build a good habit.

This comment applies to many other places in the script.

• HackXIt: The code as it is there is outdated and should be replaced, because I already fixed some issues addressed in the first answer. You can check out the current state of it at github.com/hackxit/rpi_videoloop.git
• HackXIt: However I very much appreciate your answer, it's always a good learning experience to read such improvement suggestions. I'll check if anything you addressed is still a matter in the current code.
• janos: @HackXIt if you would like the newer version of your code reviewed, you can post it in a new question (do not update this one, as that would invalidate the existing answers).

Answer 2

Set these options at the beginning:

set -eu

so that errors fail early

---

Don't use sudo in a script, because it can hang when not connected to a terminal. Possible exception: once at the beginning:

if ! test $UID -eq 0
then
 test -t 0 && exec sudo "0ドル" "$@"
 # Not a terminal - or exec failed
 echo "Insufficient privileges - try sudo 0ドル $*" >&2
 exit 1
fi

---

You probably want to be using the standard install command instead of cp and mkdir. That lets you specify the permissions as you create the targets.

---

Consider using a $DESTDIR prefix when specifying the targets. This makes it easier for package builders (such as debhelper) to re-use your script. They will need to omit the dependency checks, so make that separable if you can (or at least disable it when $DESTDIR is set).

• HackXIt: Very good to know since I didn't know of the install command until you mentioned it.

Answer 3

Your shebang should be #!/bin/sh if you're writing portable shell script. If not, read this section of man bash:

The command substitution $(cat file) can be replaced by the equivalent but faster $(< file).

Don't repeat yourself: grep -E 'omxplayer|screen|cron|usbmount|ntfs-3g' may be rewritten using pattern substitution, grep -E "${DEPENDENCIES// /|}". But this may produce undesirable result if the words are not separated by exactly one space. One remedy is to define DEPENDENCIES as an array first, then reassign to its own output:

DEPENDENCIES=(omxplayer screen cron usbmount ntfs-3g)
DEPENDENCIES=${DEPENDENCIES[@]}

One potential problem is if PTH is an existing file, then mkdir $PTH would fail, and cp $SCRIPT $PTH would overwrite the file instead of placing a copy under the directory that couldn't be created. Adding an extra / at the end of PTH to indicate it's a directory would prevent that tragedy (if you needed that file) from happening.

Do you really need sudo to chmod 755 $PTH/$SCRIPT and echo -n " consoleblank=10"? By the way, I'd write printf ' consoleblank=10' instead.

Quote everything that could be expanded (except those that follow the rule of pathname expansion) if word splitting might mess things up.

• HackXIt: Thank you! That's a very helpful suggestion. I'll look into it next time I get to the machine.
• HackXIt: Not sure what you mean with "Quote everything that could be expanded...", could you elaborate?
• Gao: @HackXIt e.g. mkdir "$PTH" and cp "$SCRIPT" "$PTH"; things that start with $ will, without quoting, be expanded and split into words by the shell according to the internal field separator (IFS), which by default is the white space characters. You usually want each of your variables to fill exactly one argument of a command, so you should quote them, especially if they might contain characters from the IFS.
• HackXIt: Does your grep -E "${DEPENDENCIES// /|}" work when the variable is turned into an array?
• Gao: @HackXIt It does. Tested it on bash 4.4.12, ksh93u+ and zsh 5.3.1.