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.

Jamal
35.2k13 gold badges134 silver badges238 bronze badges
asked Jun 3, 2014 at 23:44
\$\endgroup\$
2
  • 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\$ Commented Jun 4, 2014 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\$ Commented Jun 4, 2014 at 1:36

1 Answer 1

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

answered Jun 4, 2014 at 10:17
\$\endgroup\$
2
  • \$\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\$ Commented Jun 4, 2014 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\$ Commented Jun 4, 2014 at 11:36

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.