Shell install.sh for repository (rpi_videoloop)
Clash Royale CLAN TAG#URR8PPP
.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;
up vote
2
down vote
favorite
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 stop"
shell sh installer
add a comment |Â
up vote
2
down vote
favorite
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 stop"
shell sh installer
add a comment |Â
up vote
2
down vote
favorite
up vote
2
down vote
favorite
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 stop"
shell sh installer
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 stop"
shell sh installer
edited Apr 5 at 12:43
asked Mar 23 at 10:54
HackXIt
184
184
add a comment |Â
add a comment |Â
3 Answers
3
active
oldest
votes
up vote
1
down vote
accepted
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.
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
Jul 7 at 14:15
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.
â HackXIt
Jul 7 at 14:18
@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).
â janos
Jul 7 at 14:52
add a comment |Â
up vote
1
down vote
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 "$"
. 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.
Thank you! That's a very helpful suggestion. I'll look into it next time I get to the machine.
â HackXIt
Apr 9 at 11:45
Not sure what you mean with "Quote everything that could be expanded...", could you elaborate?
â HackXIt
Apr 9 at 11:49
@HackXIt e.g.mkdir "$PTH"
andcp "$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.
â Gao
Apr 9 at 12:43
Does yourgrep -E "$"
work when the variable is turned into an array?
â HackXIt
Apr 9 at 14:11
@HackXIt It does. Tested it on bash 4.4.12, ksh93u+ and zsh 5.3.1.
â Gao
Apr 9 at 15:05
 |Â
show 3 more comments
up vote
1
down vote
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).
1
Very good to know since I didn't know of the install command until you mentioned it.
â HackXIt
Apr 9 at 11:47
add a comment |Â
3 Answers
3
active
oldest
votes
3 Answers
3
active
oldest
votes
active
oldest
votes
active
oldest
votes
up vote
1
down vote
accepted
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.
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
Jul 7 at 14:15
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.
â HackXIt
Jul 7 at 14:18
@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).
â janos
Jul 7 at 14:52
add a comment |Â
up vote
1
down vote
accepted
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.
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
Jul 7 at 14:15
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.
â HackXIt
Jul 7 at 14:18
@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).
â janos
Jul 7 at 14:52
add a comment |Â
up vote
1
down vote
accepted
up vote
1
down vote
accepted
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.
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.
answered Jul 7 at 14:01
janos
95.6k12120343
95.6k12120343
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
Jul 7 at 14:15
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.
â HackXIt
Jul 7 at 14:18
@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).
â janos
Jul 7 at 14:52
add a comment |Â
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
Jul 7 at 14:15
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.
â HackXIt
Jul 7 at 14:18
@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).
â janos
Jul 7 at 14:52
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
Jul 7 at 14:15
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
Jul 7 at 14:15
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.
â HackXIt
Jul 7 at 14:18
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.
â HackXIt
Jul 7 at 14:18
@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).
â janos
Jul 7 at 14:52
@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).
â janos
Jul 7 at 14:52
add a comment |Â
up vote
1
down vote
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 "$"
. 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.
Thank you! That's a very helpful suggestion. I'll look into it next time I get to the machine.
â HackXIt
Apr 9 at 11:45
Not sure what you mean with "Quote everything that could be expanded...", could you elaborate?
â HackXIt
Apr 9 at 11:49
@HackXIt e.g.mkdir "$PTH"
andcp "$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.
â Gao
Apr 9 at 12:43
Does yourgrep -E "$"
work when the variable is turned into an array?
â HackXIt
Apr 9 at 14:11
@HackXIt It does. Tested it on bash 4.4.12, ksh93u+ and zsh 5.3.1.
â Gao
Apr 9 at 15:05
 |Â
show 3 more comments
up vote
1
down vote
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 "$"
. 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.
Thank you! That's a very helpful suggestion. I'll look into it next time I get to the machine.
â HackXIt
Apr 9 at 11:45
Not sure what you mean with "Quote everything that could be expanded...", could you elaborate?
â HackXIt
Apr 9 at 11:49
@HackXIt e.g.mkdir "$PTH"
andcp "$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.
â Gao
Apr 9 at 12:43
Does yourgrep -E "$"
work when the variable is turned into an array?
â HackXIt
Apr 9 at 14:11
@HackXIt It does. Tested it on bash 4.4.12, ksh93u+ and zsh 5.3.1.
â Gao
Apr 9 at 15:05
 |Â
show 3 more comments
up vote
1
down vote
up vote
1
down vote
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 "$"
. 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.
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 "$"
. 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.
answered Apr 5 at 16:35
Gao
686516
686516
Thank you! That's a very helpful suggestion. I'll look into it next time I get to the machine.
â HackXIt
Apr 9 at 11:45
Not sure what you mean with "Quote everything that could be expanded...", could you elaborate?
â HackXIt
Apr 9 at 11:49
@HackXIt e.g.mkdir "$PTH"
andcp "$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.
â Gao
Apr 9 at 12:43
Does yourgrep -E "$"
work when the variable is turned into an array?
â HackXIt
Apr 9 at 14:11
@HackXIt It does. Tested it on bash 4.4.12, ksh93u+ and zsh 5.3.1.
â Gao
Apr 9 at 15:05
 |Â
show 3 more comments
Thank you! That's a very helpful suggestion. I'll look into it next time I get to the machine.
â HackXIt
Apr 9 at 11:45
Not sure what you mean with "Quote everything that could be expanded...", could you elaborate?
â HackXIt
Apr 9 at 11:49
@HackXIt e.g.mkdir "$PTH"
andcp "$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.
â Gao
Apr 9 at 12:43
Does yourgrep -E "$"
work when the variable is turned into an array?
â HackXIt
Apr 9 at 14:11
@HackXIt It does. Tested it on bash 4.4.12, ksh93u+ and zsh 5.3.1.
â Gao
Apr 9 at 15:05
Thank you! That's a very helpful suggestion. I'll look into it next time I get to the machine.
â HackXIt
Apr 9 at 11:45
Thank you! That's a very helpful suggestion. I'll look into it next time I get to the machine.
â HackXIt
Apr 9 at 11:45
Not sure what you mean with "Quote everything that could be expanded...", could you elaborate?
â HackXIt
Apr 9 at 11:49
Not sure what you mean with "Quote everything that could be expanded...", could you elaborate?
â HackXIt
Apr 9 at 11:49
@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.â Gao
Apr 9 at 12:43
@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.â Gao
Apr 9 at 12:43
Does your
grep -E "$"
work when the variable is turned into an array?â HackXIt
Apr 9 at 14:11
Does your
grep -E "$"
work when the variable is turned into an array?â HackXIt
Apr 9 at 14:11
@HackXIt It does. Tested it on bash 4.4.12, ksh93u+ and zsh 5.3.1.
â Gao
Apr 9 at 15:05
@HackXIt It does. Tested it on bash 4.4.12, ksh93u+ and zsh 5.3.1.
â Gao
Apr 9 at 15:05
 |Â
show 3 more comments
up vote
1
down vote
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).
1
Very good to know since I didn't know of the install command until you mentioned it.
â HackXIt
Apr 9 at 11:47
add a comment |Â
up vote
1
down vote
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).
1
Very good to know since I didn't know of the install command until you mentioned it.
â HackXIt
Apr 9 at 11:47
add a comment |Â
up vote
1
down vote
up vote
1
down vote
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).
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).
answered Apr 5 at 18:11
Toby Speight
17.6k13490
17.6k13490
1
Very good to know since I didn't know of the install command until you mentioned it.
â HackXIt
Apr 9 at 11:47
add a comment |Â
1
Very good to know since I didn't know of the install command until you mentioned it.
â HackXIt
Apr 9 at 11:47
1
1
Very good to know since I didn't know of the install command until you mentioned it.
â HackXIt
Apr 9 at 11:47
Very good to know since I didn't know of the install command until you mentioned it.
â HackXIt
Apr 9 at 11:47
add a comment |Â
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f190294%2fshell-install-sh-for-repository-rpi-videoloop%23new-answer', 'question_page');
);
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password