Re: [PATCH] bb-matrix.sh: check for the existence of time

[Darren Hart <dvhart@linux.intel.com>]

Hi Jeff,

Thanks for the patch. A couple points of feedback below:

On 07/26/2011 02:57 PM, Jeff Mitchell wrote:
> What an existential subject. The patch is quite self-explanatory.

Well, there is almost always a need for commit log beyond just the
subject. The only exception might be "whitespace fixes only" or
"spelling fixes only".

Something like:

"If time is missing, bb-matrix will fail to run and report misleading
 errors about tmp/buildstats not existing. Check and abort with an
 appropriate error message."

Also, while it is tempting to squeeze in typo fixes and such with a
functional patch, unless they are in the code you are changing anyway,
they should be sent as separate patches. If it were necessary to revert
the functional patch, we'd rather not have to also lose the typo or
whitespace cleanup, for example.

> -# This script runs BB_CMD (typically building core-image-sato) for all
> +# This script runs BB_CMD (typically building core-image-minimal) for all
>  # combincations of BB_RANGE and PM_RANGE values. It saves off all the console

           ^ heh, another typo! bad dvhart.

>  # logs, the buildstats directories, and creates a bb-pm-runtime.dat file which
>  # can be used to postprocess the results with a plotting tool, spreadsheet, etc.
> @@ -32,6 +32,11 @@
>  # Darren Hart <dvhart@linux.intel.com>
>  #
>  
> +if [ ! -f /usr/bin/time ]; then

This should probably be a bit more generic:

TIME_CMD=$(which time)
if [ -z "$TIME_CMD" ]; then
	echo 'The "time" binary was not found in your path, \
              please install it.'
	exit 1
fi

Then replace /usr/bin/time below with TIME_CMD.

-- 
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel