overly complicated assignment
EMBEDDED=`echo $2`
This takes the second argument to the script and performs a split and a glob on it (surprise!) and passes the result of that to echo
and then assigns the result of whatever echo
does (which, owing to creeping featurism, may not be an echo) to EMBEDDED
. This is probably more sensibly written using a simple
EMBEDDED=$2
assignment statement.
overly complicated file path component determination
DIRECTORY=`echo $1 |cut -d'/' -f1`
This could be more intentfully written using the dirname(1)
command with also ""
to eliminate the POSIX split and glob as one probably does not want whatever results from the (surprise!) split and glob gunking up what gets assigned to DIRECTORY
.
DIRECTORY=`dirname "$1"`
However, dirname
and cut -d'/' -f1
are not the same unless the only input is somedirectory/somefile
. Someone would need to understand the inputs to the code to know whether the simplification to dirname
can be made. The expected inputs are not documented in the script posted in the question—are they documented anywhere?
On a related note there is a basename(1)
command that may help with
FILENAME=`echo $1 |cut -d'/' -f2 |cut -d'.' -f1 | sed -e "s|/|_|g"`
though it appears this pipeline with cut -d'.' -f1
attempts to obtain a prefix
from prefix.whatever
, and I have no idea what the sed -e "s|/|_|g"
is for, as /
cannot appear in filenames. If there are _
in the argument being passed it would be more sensible to pass them as _
and not add extra code to change /
into _
. So probably the above could be simplified to
FILENAME=`basename "$1" | cut -d'.' -f1`
needlessly dangerous commands
mkdir build
cd build
rm bin/${TARGET}
This sequence may or may not create a build
directory and then will try to delete various things from either the new build
directory or on failure the parent of that. This is sloppy and indeterminate. And by various things I mean that ${TARGET}
will be expanded with the POSIX shell split and glob so may contain quite unexpected filenames—luckily the rm
is not the usual "oh yeah, about that filesystem you just lost" rm -rf
so the hilarity will be limited to various (still surprising!) filenames, possibly from the wrong directory.
(Some might argue that configuration management or at least guarded commands should be used instead of fragile and error prone shell scripts, but here we are...)
[ -d build ] || { mkdir build; [ -d build ] || exit 1; }
cd build || exit 1
rm bin/"${TARGET}"
cmake .. -DSRC=../"$1" && make VERBOSE=1 && bin/"${TARGET}"
Here build
is a directory, is made as a directory, or failing that the script bails (you could also add a custom error message though mkdir
typically makes noise). This is not atomic in that something else could touch build
between the -d
check and subsequent mkdir
, so another approach is to "make the directory and then inspect the result to see whether it was a failure (not ok) or EEXIST
on a directory (ok). cd
is also checked for failures, and $TARGET
quoted to prevent the POSIX split and glob thing.
mkdir -p build || exit 1
instead of that slightly too complex thing you have, andrm -f "bin/$TARGET"
to haverm
not say anything if the file is missing. I would also have dropped those{...}
around variables when they're not needed. – Kusalananda Aug 08 '18 at 13:53mkdir -p
(though wanted also to show the full Guarded Commands Logic (which probably should be hidden behind an interface)) – thrig Aug 08 '18 at 15:20