Code review comment for lp:~michihenning/unity-scopes-api/single-tree

On 26 Aug 2015, at 14:10 , Robert Bruce Park <email address hidden> wrote:

> Nitpicky style complaints inline.

It's a shell script, not a bash script :-)

If using bash is OK, no problem, I can use more modern constructs :-)

Thanks for the review, I appreciate it!

Cheers,

Michi.

>
> Diff comments:
>
>>
>> === added file 'debian/gen-debian-files'
>> --- debian/gen-debian-files 1970-01-01 00:00:00 +0000
>> +++ debian/gen-debian-files 2015-08-26 02:41:12 +0000
>> @@ -0,0 +1,138 @@
>> +#!/bin/sh
>> +
>> +[ $# -ne 1 ] && {
>> + echo "usage: `basename $0` build-root_dir" >&2
>> + exit 1
>> +}
>> +dir="$1"/debian
>
> The requirement of passing `pwd` as the first argument is strange. When the train calls './debian/rules clean' the CWD is the root of the source tree, so you should be able to just say "dir=./debian". If you don't want to depend on the CWD being set right (which is slightly more robust), you can say 'dir=$(dirname "$0")' instead.
>
>> +
>> +# Set soversion depending on whether we are running on vivid or wily.
>> +
>> +distro=`lsb_release -c -s`
>
> Personally I prefer using $() over `` as I feel it's more readable but whatever.
>
>> +
>> +[ "$distro" = "vivid" ] && {
>> + suffix="-vivid"
>> +}
>> +
>> +echo "gen-debian-files: detected distribution: $distro"
>> +
>> +full_version=`cat "${dir}"/VERSION${suffix}`
>> +qt_full_version=`cat "${dir}"/QT-VERSION${suffix}`
>> +
>> +major=`echo $full_version | cut -d'.' -f1`
>> +minor=`echo $full_version | cut -d'.' -f2`
>> +major_minor="${major}.${minor}"
>> +
>> +qt_major=`echo $qt_full_version | cut -d'.' -f1`
>> +qt_minor=`echo $qt_full_version | cut -d'.' -f2`
>> +qt_major_minor="${qt_major}.${qt_minor}"
>> +
>> +if [ "$distro" = "vivid" ]
>> +then
>> + [ $major -gt 0 ] && {
>> + echo "impossible major version for Vivid: $major (Vivid major version must be 0)" >&2
>
> I'm not sure what you're doing here, the train really can not have two different version numbers for two different series. You really do want both vivid and wily to have the same upstream version number as this indicates that they are built from the same source tree.
>
>> + exit 1
>> + }
>> + [ $minor -lt 6 ] && {
>> + echo "impossible minor version for Vivid: $minor (Vivid minor version must be >= 6)" >&2
>> + exit 1
>> + }
>> + soversion=`expr $minor - 3`
>> +else
>> + soversion="${major}.${minor}"
>> +fi
>> +
>> +[ "$distro" = "vivid" -a $qt_major -gt 0 ] && {
>> + echo "impossible Qt major version for Vivid: $qt_major (Vivid Qt major version must be 0)" >&2
>> + exit 1
>> +}
>> +qt_soversion="${qt_major}.${qt_minor}"
>> +
>> +warning_msg()
>> +{
>> + file=`basename "$@"`
>> + echo "`cat <<EOF
>> +# This file is autogenerated. DO NOT EDIT!
>> +#
>> +# Modifications should be made to $file instead.
>> +# This file is regenerated automatically in the clean target.
>> +#
>> +EOF`"
>> +}
>
> This 'warning_msg' function is really bizarre. Much nicer would be like:
>
> warning=$(mktemp -t readonly.XXX)
> cat >$warning <<EOF
> # This file...
> EOF
>
>> +
>> +# Generate debian/control from debian/control.in, substituting the soversion for both libs.
>> +# For wily onwards, we also add an entry for the vivid version to "Conflicts:" and "Replaces:".
>> +
>> +infile="${dir}"/control.in
>> +outfile="${dir}"/control
>> +echo "`warning_msg $infile`" >"$outfile"
>> +sed -e "s/@UNITY_SCOPES_SOVERSION@/${soversion}/" \
>> + -e "s/@UNITY_SCOPES_QT_SOVERSION@/${qt_soversion}/" "$infile" >>"$outfile"
>> +
>> +[ "$distro" != "vivid" ] && {
>> + sed -i -e "Replaces: libunity-scopes0,/a\
>> +\ libunity-scopes${so_version}," \
>> + -e "Conflicts: libunity-scopes0,/a\
>> +\ libunity-scopes${so_version}," \
>> + "$outfile"
>> +}
>> +
>> +# Generate the install files, naming them according to the soversion.
>> +
>> +# Install file for binary package
>> +infile="${dir}"/libunity-scopes.install.in
>> +outfile="${dir}"/libunity-scopes${soversion}.install
>> +echo "`warning_msg $infile`" >"$outfile"
>> +cat "$infile" >>"$outfile"
>
> ... and then here you'd do:
>
> cat $warning $infile >$outfile
>
>> +
>> +# Install file for dev package
>> +infile="${dir}"/libunity-scopes-dev.install.in
>> +outfile="${dir}"/libunity-scopes-dev.install
>> +echo "`warning_msg $infile`" >"$outfile"
>> +sed "s/@UNITY_SCOPES_SOVERSION@/${soversion}/" "$infile" >>"$outfile"
>
> cat $warning $infile | sed "s/@UNITY_SCOPES_SOVERSION@/${soversion}/" >$outfile
>
>> +
>> +# Install file for click hook
>> +infile="${dir}"/libunity-scopes.scope.click-hook.in
>> +outfile="${dir}"/libunity-scopes${soversion}.scope.click-hook
>> +echo "`warning_msg $infile`" >"$outfile"
>> +cat "$infile" >>"$outfile"
>
> cat $warning $infile >$outfile
>
>> +
>> +# Symbols file for vivid or shlibs file for wily and later
>> +if [ "$distro" = "vivid" ]
>> +then
>> + infile="${dir}"/libunity-scopes.symbols.in
>> + outfile="${dir}"/libunity-scopes${soversion}.symbols
>> + sed "s/@UNITY_SCOPES_SOVERSION@/${soversion}/g" "$infile" >>"$outfile"
>> +else
>> + infile="${dir}"/shlibs.in
>> + outfile="${dir}"/shlibs
>> + echo "`warning_msg $infile`" >"$outfile"
>> + sed -e "s/@UNITY_SCOPES_SOVERSION@/${soversion}/g" \
>> + -e "s/@UNITY_SCOPES_QT_SOVERSION@/${qt_soversion}/g" \
>> + -e "s/@UNITY_SCOPES_FULL_VERSION@/${full_version}/g" \
>> + -e "s/@UNITY_SCOPES_QT_FULL_VERSION@/${qt_full_version}/g" \
>> + -e "s/@UNITY_SCOPES_MAJOR_MINOR@/${major_minor}/g" \
>> + -e "s/@UNITY_SCOPES_QT_MAJOR_MINOR@/${qt_major_minor}/g" \
>> + "$infile" >>"$outfile"
>
> cat $warning $infile | sed ... >$outfile
>
>> +fi
>> +
>> +# Install file for qt binary package
>> +infile="${dir}"/libunity-scopes-qt.install.in
>> +outfile="${dir}"/libunity-scopes-qt${qt_soversion}.install
>> +echo "`warning_msg $infile`" >"$outfile"
>> +cat "$infile" >>"$outfile"
>
> etc
>
>> +
>> +# Install file for qt dev package
>> +infile="${dir}"/libunity-scopes-qt-dev.install.in
>> +outfile="${dir}"/libunity-scopes-qt-dev.install
>> +echo "`warning_msg $infile`" >"$outfile"
>> +sed "s/@UNITY_SCOPES_QT_SOVERSION@/${qt_soversion}/" "$infile" >>"$outfile"
>
> etc
>
>> +
>> +# Qt symbols file for vivid
>> +[ "$distro" = "vivid" ] && {
>> + infile="${dir}"/libunity-scopes-qt.symbols.in
>> + outfile="${dir}"/libunity-scopes-qt${qt_soversion}.symbols
>> + sed "s/@UNITY_SCOPES_QT_SOVERSION@/${qt_soversion}/g" "$infile" >"$outfile"
>
> etc
>
>> +}
>> +
>> +exit 0
>
>
> --
> https://code.launchpad.net/~michihenning/unity-scopes-api/single-tree/+merge/268433
> You are the owner of lp:~michihenning/unity-scopes-api/single-tree.