[PATCH] qmake: use a dedicated build script to generate ssrf-version.h

Cristian Ionescu-Idbohrn cristian.ionescu-idbohrn at axis.com
Fri Jan 30 14:45:36 PST 2015


On Fri, 30 Jan 2015, Lubomir I. Ivanov wrote:

I'll just nitpick on this one:

> diff --git a/scripts/write-version b/scripts/write-version
> new file mode 100644
> index 0000000..42505ae
> --- /dev/null
> +++ b/scripts/write-version
> @@ -0,0 +1,61 @@
> +#!/bin/sh

Try this same thing with adding:

set -eu
#set -x	#xtrace

> +#
> +# arguments:
> +#	$1 - target H file
> +#	$2 - fallback version string
> +#	$3 - os name {linux|darwin|win}
> +#
> +# !!! has no error checking ATM !!!
> +# should be started from where .git is!
> +#
> +
> +# set OS and TARGET

Any of these may fail in absence of

[ $# -eq 3 ] || {
	echo "ERROR: missing argument(s)"	# alternative, usage message
	exit 1
}

> +TARGET=$1
> +TARGET_TEMP=$TARGET.tmp
> +VERSION=$2
> +OS=$3
> +
> +# check if git is installed
> +GIT_ERROR=1

This is, AFAIK, not portable:

> +HAS_GIT=`git --version 2> NUL`
                             ^^^
I'd write this:

> +if [ "$HAS_GIT" ]; then

as:

if HAS_GIT=$(git --version 2>/dev/null) && [ "$HAS_GIT" ]; then

Backticks are considered deprecated.

> +	# check if .git exists
> +	if [ -d ".git" ]; then
	        ^    ^
Quotes are useless in that context.

> +		FULL_VER=`sh ./scripts/get-version linux`
> +		GIT_ERROR=0
> +	else
> +		echo "WARNING: not a git repository"
> +	fi
> +else
> +	echo "WARNING: git command not found"
> +fi
> +
> +#if there was an error, fallback to .gitversion or use the dummy version
> +if [ "$GIT_ERROR" == "1" ]; then
                     ^^ ^ ^
'==' is a bashism and quotes are useless.

> +	if [ -f .gitversion ]; then
	     ^^
Existence of that regular file and readabylity of the same is not
realy the same thing.

> +		FULL_VER=`cat .gitversion`

		read FULL_VER <.gitversion

is more efficient.

> +	else
> +		FULL_VER=$VERSION
> +	fi
> +fi
> +
> +# grab some strings from get-version

Both these:

> +CANONICAL_VER=`sh ./scripts/get-version full $FULL_VER`
> +OS_USABLE_VER=`sh ./scripts/get-version $OS $FULL_VER`

lack error handling, and deprecated backticks apply.

> +
> +# write to a temp file

Error handling (I know, this is really paranoic, but):

> +echo "#define VERSION_STRING \"$OS_USABLE_VER\"" > $TARGET_TEMP
> +echo "#define GIT_VERSION_STRING \"$FULL_VER\"" >> $TARGET_TEMP
> +echo "#define CANONICAL_VERSION_STRING \"$CANONICAL_VER\"" >> $TARGET_TEMP

that won't work if you happen to not have write permissions.

> +
> +# if the target file is missing create it
> +if [ ! -f $TARGET ]; then

or [ ! -w $TARGET ] ?

and more backticks:

> +	CMD=`touch $TARGET`
> +fi
> +
> +# if the temp file and the target file differ, replace the target file with the temp file
> +CMD=`diff -q $TARGET $TARGET_TEMP || cp $TARGET_TEMP $TARGET`
> +
> +# remove the temp file
> +CMD=`rm -f $TARGET_TEMP`


Cheers,

-- 
Cristian


More information about the subsurface mailing list