patternbashMinor
Building/installing bash script
Viewed 0 times
scriptinstallingbuildingbash
Problem
I'm writing a bash script that has these goals:
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:
Here's the full code. You can also watch what it does for the user on YouTube:
#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
- 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
Solution
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:
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:
There should only be approximately one invocation of
Respecting the user's machine
It appears that the application installs primarily to
You also write to
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
In particular, the creation of symlinks
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
Answers to your questions
-
Calling functions from other functions is perfectly fine. In fact, a defined function can be thought of as just another command (like
-
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.
-
Ideally, parts of the script, such as the
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:
- git-buildpackage is a suite of commands to create Debian packages based on Git repositories.
- grunt-debian-package is a command to create a Debian package from a grunt build.
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
/etc/apt/sources.list.d
$HOME/.config/Popcorn-Time
/usr/bin/node
/lib/$(arch)-linux-gnu/libudev.so.1
/usr/bin/popcorn-time
/usr/share/pixmaps/popcorntime.png
/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
-
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.-
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.
-
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 placContext
StackExchange Code Review Q#52398, answer score: 5
Revisions (0)
No revisions yet.