Shell install.sh for repository (rpi_videoloop)

The name of the pictureThe name of the pictureThe name of the pictureClash 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"






share|improve this question



























    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"






    share|improve this question























      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"






      share|improve this 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 stop"








      share|improve this question












      share|improve this question




      share|improve this question








      edited Apr 5 at 12:43
























      asked Mar 23 at 10:54









      HackXIt

      184




      184




















          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.






          share|improve this answer





















          • 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

















          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.






          share|improve this answer





















          • 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" 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











          • @HackXIt It does. Tested it on bash 4.4.12, ksh93u+ and zsh 5.3.1.
            – Gao
            Apr 9 at 15:05

















          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).






          share|improve this answer

















          • 1




            Very good to know since I didn't know of the install command until you mentioned it.
            – HackXIt
            Apr 9 at 11:47










          Your Answer




          StackExchange.ifUsing("editor", function ()
          return StackExchange.using("mathjaxEditing", function ()
          StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix)
          StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
          );
          );
          , "mathjax-editing");

          StackExchange.ifUsing("editor", function ()
          StackExchange.using("externalEditor", function ()
          StackExchange.using("snippets", function ()
          StackExchange.snippets.init();
          );
          );
          , "code-snippets");

          StackExchange.ready(function()
          var channelOptions =
          tags: "".split(" "),
          id: "196"
          ;
          initTagRenderer("".split(" "), "".split(" "), channelOptions);

          StackExchange.using("externalEditor", function()
          // Have to fire editor after snippets, if snippets enabled
          if (StackExchange.settings.snippets.snippetsEnabled)
          StackExchange.using("snippets", function()
          createEditor();
          );

          else
          createEditor();

          );

          function createEditor()
          StackExchange.prepareEditor(
          heartbeatType: 'answer',
          convertImagesToLinks: false,
          noModals: false,
          showLowRepImageUploadWarning: true,
          reputationToPostImages: null,
          bindNavPrevention: true,
          postfix: "",
          onDemand: true,
          discardSelector: ".discard-answer"
          ,immediatelyShowMarkdownHelp:true
          );



          );








           

          draft saved


          draft discarded


















          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






























          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.






          share|improve this answer





















          • 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














          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.






          share|improve this answer





















          • 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












          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.






          share|improve this answer













          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.







          share|improve this answer













          share|improve this answer



          share|improve this answer











          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
















          • 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












          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.






          share|improve this answer





















          • 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" 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











          • @HackXIt It does. Tested it on bash 4.4.12, ksh93u+ and zsh 5.3.1.
            – Gao
            Apr 9 at 15:05














          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.






          share|improve this answer





















          • 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" 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











          • @HackXIt It does. Tested it on bash 4.4.12, ksh93u+ and zsh 5.3.1.
            – Gao
            Apr 9 at 15:05












          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.






          share|improve this answer













          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.







          share|improve this answer













          share|improve this answer



          share|improve this answer











          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" 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











          • @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










          • 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











          • 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















          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










          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).






          share|improve this answer

















          • 1




            Very good to know since I didn't know of the install command until you mentioned it.
            – HackXIt
            Apr 9 at 11:47














          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).






          share|improve this answer

















          • 1




            Very good to know since I didn't know of the install command until you mentioned it.
            – HackXIt
            Apr 9 at 11:47












          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).






          share|improve this answer













          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).







          share|improve this answer













          share|improve this answer



          share|improve this answer











          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












          • 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












           

          draft saved


          draft discarded


























           


          draft saved


          draft discarded














          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













































































          Popular posts from this blog

          Greedy Best First Search implementation in Rust

          Function to Return a JSON Like Objects Using VBA Collections and Arrays

          C++11 CLH Lock Implementation