6
\$\begingroup\$

I'm writing a bash script that has these goals:

  • to build a node program from sources
  • to install the built program on Ubuntu Linux
  • to allow the user to rebuilt from most recent sources
  • to add a small CLI (it has none)
  • to add a small GUI for support (like report-bug,fix common errors, etc.)

So, it's pretty complete in my opinion, and the good thing is, it works well. But what concerns me is that the code seems chaotic and not optimized, even if it's entirely fine from a user's point-of-view.

Here's the code's resume:

#Part1 : VARIABLES
11 variables I use for URLs, version number, log-files, etc.

#Part 2 : FUNCTIONS
9 functions.
- 4 of them are quite simple and "regular" ones, for example "error checking" 
  and "exit if the user is root"
- 1 of them contains almost only text to write inside text-files
- 4 of them call other function

#part 3 : SCRIPT
- a 'case' to watch options like "-update"
- the list of 7 functions that I need to run everytime with or without the 
  susmentionned 'options'

Total = 409 lines (~150 without all the simple text to write somewhere or to display)

Here's the full code. You can also watch what it does for the user on YouTube:

#!/bin/bash
installdir="/opt"
version="dev-0.3"
OfficialURL="http://get-popcorn.com"
githubURL="https://github.com/popcorn-official/popcorn-app"
issueURL="https://github.com/popcorn-official/popcorn-app/issues"
icon="https://github.com/popcorn-official/popcorn-app/raw/master/src/app/images/icon.png"
log="$HOME/popcorn-build.log"
buildscriptURL="https://raw.githubusercontent.com/MrVaykadji/misc/master/Popcorn-Time/0.3.0/"
buildscript="build-popcorn"
[ $(arch) == "x86_64" ] && arch=64 || arch=32
buildtime="`date +%Y.%m.%d-%Hh%M`"

#FUNCTIONS
func_apt() {
for lock in synaptic update-manager software-center apt-get "dpkg " aptitude
do
 if ps -U root -u root u | grep "$lock" | grep -v grep > /dev/null; then 
  echo "
Unexpected Error:
=================
Please close $lock then try again.";
  exit 1
 fi
done 
}

func_root() {
[ "$EUID" == "0" ] && 
echo "Error. You need to run this without 'root' or 'sudo' privileges." && 
exit 2
}

func_error() {
[ -n $error ] && return 0
echo "
Unexpected Error:
================="
cat $log
echo "
Please try again."
exit 1 
}

func_clean() {
case $1 in 
    -save)
        sudo mkdir -p /tmp/popcorn-config
        sudo cp -r $HOME/.config/Popcorn-Time/data /tmp/popcorn-config/ &> /dev/null
        sudo rm -rf $HOME/.config/Popcorn-Time/*
        sudo cp -r /tmp/popcorn-config/data $HOME/.config/Popcorn-Time/ &> /dev/null && 
        sudo chown -Rf $USER:$USER $HOME/.config/Popcorn-Time/data && 
        sudo chmod -R 774 $HOME/.config/Popcorn-Time/data
        ;;
    -all)
        sudo rm -rf $installdir/Popcorn-Time /usr/share/pixmaps/popcorntime.png /usr/bin/popcorn-time $HOME/tmp $HOME/popcorn-app-$version $HOME/npm-debug.log $HOME/.npm $HOME/.cache/bower $HOME/.config/configstore/insight-bower.yml $HOME/.config/configstore/update-notifier-bower.yml $HOME/.local/share/bower $log $HOME/$version.zip
        ;;
    -package)
        sudo apt-get purge nodejs -y &> /dev/null && 
        sudo apt-get autoremove -y &> /dev/null && 
        sudo rm -rf /usr/bin/node && 
        sudo add-apt-repository -yr ppa:chris-lea/node.js &> /dev/null && 
        echo -e "... Done.\n" 
        ;;
    -building)
 sudo rm -rf $HOME/tmp $HOME/popcorn-app-$version $HOME/npm-debug.log $HOME/.npm $HOME/.cache/bower $HOME/.config/configstore/insight-bower.yml $HOME/.config/configstore/update-notifier-bower.yml $HOME/.local/share/bower $log && 
        echo -e "... Done.\n"
        ;;
esac
}

func_ptexists() {
if [ "$1" == "-update" ] ; then
    func_clean -save
else
    [ -e "$installdir/Popcorn-Time" ] && 
    read -p "
WARNING: Popcorn-Time is already installed in '$installdir' and will be erased. Do you want to keep the configuration files (bookmarks, watched list, settings, ...) [y/n] ? "
    if [ "$REPLY" == "y" ] ; then
        func_clean -save
    else    
        sudo rm -rf $HOME/.config/Popcorn-Time/
    fi
    sudo rm -rf /usr/share/applications/popcorn-time.desktop
fi
func_clean -all
}

func_dependencies() {
[[ -n `egrep -v '^#|^ *$' /etc/apt/sources.list /etc/apt/sources.list.d/* | grep chris-lea/node.js` ]] && nodeppa=1 || nodeppa=0
if [ -n "`dpkg-query -W -f='${Status}\n' nodejs wget unzip | grep not`" ] || [ $nodeppa == "0" ] ; then
echo "- Checking for dependencies 'nodejs', 'wget' and 'unzip'..."
sudo apt-add-repository -y ppa:chris-lea/node.js &> $log &&
sudo apt-get update &> $log &&
sudo apt-get install nodejs wget unzip -y &> $log && echo -e " ...Ok !" || error=1
func_error
fi

#npm dep
if [ -e "/usr/lib/node_modules/bower" ] && [ -e "/usr/lib/node_modules/grunt-cli" ] ; then
    echo -e "\n- Updating NPM 'grunt-cli' and 'bower'..."
else
    echo -e "\n- Installing NPM 'grunt-cli' and 'bower'..."
fi
sudo npm install -g grunt-cli bower &> $log && echo -e "  ...Ok !\n" || error=1
func_error

#repair broken nodejs symlink
[ ! -e /usr/bin/node ] && sudo ln -s /usr/bin/nodejs /usr/bin/node 

#symlink libudev.so on 12.04
[ `lsb_release -cs` == "precise" ] && [ ! -e /lib/$(arch)-linux-gnu/libudev.so.1 ] && sudo ln -s /lib/$(arch)-linux-gnu/libudev.so.0 /lib/$(arch)-linux-gnu/libudev.so.1 
}

func_build() {
#get sources
echo "- Downloading '$version' sources from GitHub..."
cd
wget $githubURL/archive/$version.zip -O $version.zip &> $log && unzip -o $version.zip &> $log && rm $version.zip && echo -e "  ...Ok !\n" || error=1
func_error

#npm
cd popcorn-app-$version
echo "- Running 'npm install'..."
sudo chown -Rf $USER:$USER $HOME/popcorn-app-$version/ $HOME/tmp
npm install --yes &> $log && echo -e "  ...Ok !\n" || error=1
func_error

#build
if [ "$1" == "-update" ] ; then
 buildcommand="linux$arch"
else
 buildvar=0
 echo -e "You can build for this machine only (linux$arch) or for all plateforms, including : Mac, Windows, Linux 32-bits, Linux 64-bits.\n\nFor what platforms do you wish to build (for multiple builds, separate each platform with a comma)"
 read -p "[mac/win/linux32/linux64/all] : " input
 IFS=',' read -a options <<< "$input"
 shopt -s extglob
 for option in "${options[@]}"; do
    case "$option" in
        win|mac|linux32|linux64|all)
            buildcommand="${buildcommand:+$buildcommand,}$option"
            buildvar=1;;
        *)
            printf 'Invalid option "%s" ignored.\n' "$option";;
    esac
 done
 if (( !buildvar )); then
    echo "Incorrect input. Default build 'linux$arch' selected."
    buildcommand="linux$arch"
 fi
 [[ -n "`echo $buildcommand | grep all`" ]] && buildcommand="all"
fi

echo -e "\n- Building with 'grunt'..."
grunt build --platforms=$buildcommand &> $log && echo -e "   ...Ok !\n" || error=1
func_error

echo -e "Popcorn-Time has been built in :\n  «$HOME/popcorn-app-$version/build/releases/Popcorn-Time/»\n" 
}

func_install() {
[ "`echo $buildcommand | grep -v linux$arch`" ] && exit 0

if [ "$1" != "-update" ] ; then
read -p "Do you wish to install Popcorn-Time on this computer [y/n] ? "
[ "$REPLY" != "y" ] && exit 0
fi  

sudo mkdir -p $installdir
sudo cp -r $HOME/popcorn-app-$version/build/releases/Popcorn-Time/linux$arch/Popcorn-Time $installdir

echo -e "\n- Creating commandline launcher..."
echo "#!/bin/bash
echo \"Popcorn Time
============\"

[ \"\$EUID\" == \"0\" ] && echo \"Error: You need to run this without 'root' or 'sudo' privileges.\" && exit 2

helpsection() {
echo \"Version $version 
Built on $buildtime from $githubURL
Official website : $OfficialURL

Options:
  -h, --help        Display this help.
  -q,--quiet        Launch Popcorn-Time without output.
  --flush       Flush databases.
  --fix-node        Fix the node-webkit 'blank' error.
  --uninstall       Uninstall Popcorn-Time.
  --issue       Report an issue.
  --build       Build latest version from sources.\"
}

flush_all() {
echo \"- Flushing databases...\"
sudo rm -rf $HOME/.config/Popcorn-Time
}

uninstall() {
echo \"- Uninstalling Popcorn-Time and removing configuration files...\"
sudo bash $installdir/Popcorn-Time/uninstall.sh
}

popcorntimequiet() {
echo \"Starting...\"
nohup $installdir/Popcorn-Time/Popcorn-Time &> /dev/null &
exit 0
}

popcorntime() {
$installdir/Popcorn-Time/Popcorn-Time
}

reportissue() {
echo \"Here is what a great bug report looks like:


###############################
Describe the problem here

Version: $version for Linux $arch bits
         Built on $buildtime
Downloaded from: $githubURL
OS: `lsb_release -si` `lsb_release -sr` `arch`
Connection: X mbps

How to reproduce:
 - Step 1
 - Step 2
 - Step 3
Actual result:
 - X goes wrong
Expected result:
 - X should go like that
###############################\"
xdg-open $issueURL &> /dev/null
}

fix_node() {
echo \"Fixing node-webkit...\"
rm -rf $HOME/.config/node-webkit
}

build_pt() {
cd
echo \"Building script fetched from GitHub...\"
wget -q $buildscriptURL$buildscript
bash $buildscript -update
}

case \$1 in
    -h|--help)
        helpsection
        ;;
    --uninstall)
        uninstall
        ;;
    --flush)
        flush_all
        ;;
    --fix-node)
        fix_node
        ;;
    --issue)
        reportissue
        ;;
    -q|--quiet)
        popcorntimequiet
        ;;
    --build)
        build_pt
        ;;
    *)
        popcorntime
        ;;
esac" | sudo tee /usr/bin/popcorn-time &> /dev/null
sudo chmod +x /usr/bin/popcorn-time
echo -e "    «/usr/bin/popcorn-time»\n"

echo "- Creating launcher... "
sudo wget $icon -qO /tmp/popcorntime.png && sudo cp /tmp/popcorntime.png /usr/share/pixmaps/
echo "[Desktop Entry]
Comment=Watch movies in streaming with P2P.
Comment[fr]=Regarder des films en streaming.
Name=Popcorn Time
Exec=/usr/bin/popcorn-time
StartupNotify=false
Type=Application
Icon=popcorntime
Actions=ForceClose;ReportIssue;FlushDB;FixNode;BuildUpdate;
Keywords=P2P;streaming;movies;tv;series;shows;
Keywords[fr]=P2P;streaming;films;séries;télévision;tv;

[Desktop Action ForceClose]
Name=Force close
Name[fr]=Forcer la fermeture
Exec=killall Popcorn-Time
OnlyShowIn=Unity;

[Desktop Action ReportIssue]
Name=Report Issue
Name[fr]=Rapporter un problème
Exec=sh -c \"popcorn-time --issue\"
OnlyShowIn=Unity;

[Desktop Action FlushDB]
Name=Flush databases
Name[fr]=Vider les bases de données
Exec=sh -c \"killall Popcorn-Time ; rm -rf $HOME/.config/Popcorn-Time ; /usr/bin/popcorn-time\"
OnlyShowIn=Unity;

[Desktop Action FixNode]
Name=Fix Node-Webkit
Name[fr]=Réparer Node-Webkit
Exec=sh -c \"rm -rf $HOME/.config/node-webkit ; killall Popcorn-Time ; /usr/bin/popcorn-time\"
OnlyShowIn=Unity;

[Desktop Action BuildUpdate]
Name=Build latest version
Name[fr]=Construire la dernière version
Exec=sh -c 'killall Popcorn-Time ; xterm -fa monaco -fs 13 -bg black -fg white -title \"Build latest Popcorn Time\" -e \"popcorn-time --build\" ; /usr/bin/popcorn-time'
OnlyShowIn=Unity;" | sudo tee /usr/share/applications/popcorn-time.desktop &> /dev/null
sudo chmod +x /usr/share/applications/popcorn-time.desktop
echo -e "    «/usr/share/applications/popcorn-time.desktop»\n"

echo "- Creating uninstall script..."
echo "#!/bin/bash
#uninstallation script for Popcorn-Time

#clean directory
sudo rm -rf $installdir/Popcorn-Time

#clean config
sudo rm -rf $HOME/.config/Popcorn-Time

#clean icon
sudo rm -rf /usr/share/pixmaps/popcorntime.png

#clean launchers
sudo rm -rf /usr/bin/popcorn-time
sudo rm -rf /usr/share/applications/popcorn-time.desktop
" | sudo tee $installdir/Popcorn-Time/uninstall.sh &> /dev/null
sudo chmod +x $installdir/Popcorn-Time/uninstall.sh
echo -e "    «$installdir/Popcorn-Time/uninstall.sh»\n" 
}

func_end() {
if [ "$buildcommand" == "linux$arch" ] ; then
 if [ "$1" == "-update" ] ; then
    func_clean -building
    sudo rm -rf $0
 else
  read -p "Do you wish to remove all the building files [y/n] ? "
  [ "$REPLY" == "y" ] && func_clean -building
 fi
fi

if [ "$nodeppa" == "0" ] ; then
    read -p "Do you wish to uninstall the packages installed for this build, they will be needed in case of new build [y/n] ? "
    [ "$REPLY" == "y" ] && func_clean -package
fi 
}

#SCRIPT#
func_root
func_apt

echo "
Popcorn-Time $version for Ubuntu-Linux
=====================================
Popcorn Time streams movies from Torrents.
Downloading copyrighted material may be illegal in your country.
!!! Use at your own risk !!!
"

sudo test 

case $1 in
    -update)
        option="-update"
        ;;
    *)
        [ -n "$1" ] && echo -e "\nUnauthorized option '$1' will be ignored."
        ;;
esac

func_ptexists $option
func_dependencies
func_build $option
func_install $option
func_end $option

echo "=================================================
Popcorn-Time is now installed !

Type «popcorn-time --help» for more information."
exit 0

My questions concern some techniques that I use:

  1. I call functions from within other functions. Can I do that?

  2. Is 150 lines too long (without text to display and commentaries) for this kind of code?

  3. Should I split the code if I can? Leaving, for example, one script for the build that will download the installation script and the needed text-files, and another that will only be downloaded when the user wants to rebuild, ... these kinds of things.

  4. I would be grateful if you could skim through the code and give me an honest opinion about its globality.

\$\endgroup\$
  • 2
    \$\begingroup\$ I have to ask, why are you using a bash script to duplicate the functionality of dpkg? Just build a deb and be done with it. I would think three, maybe four times before running a script with all those sudo's \$\endgroup\$ – rolfl Jun 4 '14 at 0:50
  • \$\begingroup\$ Because a deb won't allow the user to build it whenever they want with the latest commit (it's a VERY active development, maybe 3 to 10 commits a day). There's a Webupd8 PPA for it, but it's not updated at all (just 'stable beta' releases), and official daily builds, well, are "daily", so if there's a bug, you have to wait the next day even if a patch has already aired. Plus, nothing on the web has as much options as I want to offer, and none of them offer a CLI. \$\endgroup\$ – MrVaykadji Jun 4 '14 at 1:36
5
\$\begingroup\$

Working with the ecosystem

It important to know when not to write code. If you're going to work in the APT ecosystem, then you should work with other package management tools, rather than invent your own approach. There are at least two tools you should be leveraging:

Consider the advantages of working with the ecosystem:

  • Less code for you to maintain.
  • The installation process works the way users expect it to. I, for one, don't like to run strangers' scripts as root, as I have no idea how they might be trashing my system. (And in my opinion, you are trashing my system, as I will discuss below.)
  • Users get the benefits of package management, such as clean uninstallation and upgrades, dependency and conflict checking, standard filesystem paths, etc.
  • You'll eventually need to package the product properly for release anyway, so you might as well start now.

Maybe you think it's OK for Popcorn Time to do its own thing. But if every application did the same, it would be chaos.

Building vs. installing

Your script commingles the build and installation steps. I'd like to see them separated:

  • The build process fetches the code from the Internet and generates installable packages. This step should be done without root privileges, as it runs a lot of complex programs, such as compilers.
  • The installation process, in contrast, usually does require root privileges. It should therefore be restricted to simple, trustworthy tasks, namely unpacking the previously generated packages and maybe running some pre- and post-install scripts. I'd like to have the opportunity to inspect the packages and the pre-/post-install scripts before performing the installation.

There should only be approximately one invocation of sudo that encompasses all of the installation process. After all, you shouldn't assume that /etc/sudoers contains a conveniently permissive entry for the user.

Respecting the user's machine

It appears that the application installs primarily to /opt/Popcorn-Time, which is good (for an application that is not managed as a .deb package). Anything that you install outside of /opt/Popcorn-Time would be unexpected, and should deserve special mention, perhaps even confirmation, and ideally avoided altogether.

You also write to

  1. /etc/apt/sources.list.d
  2. $HOME/.config/Popcorn-Time
  3. /usr/bin/node
  4. /lib/$(arch)-linux-gnu/libudev.so.1
  5. /usr/bin/popcorn-time
  6. /usr/share/pixmaps/popcorntime.png
  7. /usr/share/applications/popcorn-time.desktop

You do provide an uninstall script to clean up after the last three, but I would still consider it littering. The FHS requires non-package-managed files to go in /usr/local, I believe.

In particular, the creation of symlinks /usr/bin/node/usr/bin/nodejs and /lib/$(arch)-linux-gnu/libudev.so.1/lib/$(arch)-linux-gnu/libudev.so.0 is precisely the kind of secret shenanigans I fear most when running untrustworthy scripts as root. I expect every file in system directories such as /usr/bin and /lib to be either part of an installed Debian package or be managed by a post-install / pre-remove script of an installed Debian package — in other words, fully managed by the package management system. Furthermore, an increment in the major version of a shared library indicates an API incompatibility. Library versioning is the Unix solution to DLL Hell, and by creating a symlink that spans major version numbers, you have trashed my system without so much as a courtesy notice! An executable that is properly built for the target OS should never need such a hack.

Distinction between normal users and root

Is this meant to be a machine-wide installer? If so, then it has no business trying to manage per-user directories such as $HOME/.config/Popcorn-Time. Keep in mind that in a multi-user system, each user who has ever run the application could have his/her own $HOME/.config/Popcorn-Time directory. There's nothing special about the user who is running the uninstaller.

Answers to your questions

  1. Calling functions from other functions is perfectly fine. In fact, a defined function can be thought of as just another command (like ls or date) — except that it's only available within your script.

  2. My concern is not the line count, but that you try to do too much.

    The script implements a build/installation process that doesn't play well with Ubuntu tools. You would be writing less code and making your users happier if you leveraged the appropriate tools for solving this kind of problem.

  3. Ideally, parts of the script, such as the popcorn-time.desktop file and the popcorn-time launch script, would be distributed in the Popcorn Time application's Git repository itself. Your build script would merely need to copy them into place, perhaps doing some string substitutions using sed.

  4. You misspelled "platform".

\$\endgroup\$
  • \$\begingroup\$ Thanks! 1. Unfortunatley, every symlink is required, because Ubuntu 12.04 is too old and 14.04 doesn't have by default the latest nodejs. But would you like to see a quick "manual" before the script runs that explains everything it does to the machine and makes sure the user is fine with it? 2. Thanks for the $HOME thing, it's indeed something I missed because I'm the only user. 3. I "need" to access /usr/bin, but would it be better to symlink there the script that would then be stored in /opt ? \$\endgroup\$ – MrVaykadji Jun 4 '14 at 10:59
  • 2
    \$\begingroup\$ 1. You can limit the scope of the libudev.so hack. Make symlink /opt/Popcorn-Time/lib/libudev.so.0/lib/$(arch)-linux-gnu/libudev.so.1 instead, and set LD_LIBRARY_PATH=/opt/Popcorn-Time/lib in your launch script. Likewise, you can find a solution that doesn't pollute /usr/bin. 3. What's wrong with just /opt/Popcorn-Time/Popcorn-Time? The .desktop file can just refer to it. Are you sure you also need a /usr/bin/popcorn-time symlink? How about /usr/local/bin/popcorn-time instead? \$\endgroup\$ – 200_success Jun 4 '14 at 11:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.