| Submitter | Jeff Mitchell |
|---|---|
| Date | July 26, 2011, 9:57 p.m. |
| Message ID | <4E2F384F.6060907@ll.mit.edu> |
| Download | mbox | patch |
| Permalink | /patch/8615/ |
| State | New, archived |
| Headers | show |
Comments
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.
On 07/26/2011 06:41 PM, Darren Hart wrote: > > 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". I did write a commit log, but git-format-patch turned it into the subject of the attached patch. No, I haven't used git-format-patch before :-) (I've been fortunate to have been able to avoid it for many years now.) > 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. Given the triviality of both changes I didn't think this would be an issue. --Jeff
Patch
From 62972e71cd952a890068a1f6b6e2234608747a1c Mon Sep 17 00:00:00 2001 From: Jeff Mitchell <jmitchell@ll.mit.edu> Date: Tue, 26 Jul 2011 17:45:58 -0400 Subject: [PATCH] Check that the time binary is installed in /usr/bin/time when running bb-matrix.sh. Also fix a typo. --- scripts/contrib/bb-perf/bb-matrix.sh | 9 +++++++-- 1 files changed, 7 insertions(+), 2 deletions(-) diff --git a/scripts/contrib/bb-perf/bb-matrix.sh b/scripts/contrib/bb-perf/bb-matrix.sh index b9edd5f..66bd567 100755 --- a/scripts/contrib/bb-perf/bb-matrix.sh +++ b/scripts/contrib/bb-perf/bb-matrix.sh @@ -18,7 +18,7 @@ # Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. # # DESCRIPTION -# 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 # 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 + echo "The \"time\" binary was not found at /usr/bin/time -- please install it!" + exit 1 +fi + # The following ranges are appropriate for a 4 core system with 8 logical units BB_RANGE="04 05 06 07 08 09 10 11 12 13 14 15 16" PM_RANGE="04 05 06 07 08 09 10 11 12 13 14 15 16" @@ -48,7 +53,7 @@ TIME_STR="%e %S %U %P %c %w %R %F %M %x" mkdir $DATADIR if [ $? -ne 0 ]; then echo "Failed to create $DATADIR." - exit 1 + exit 2 fi # Add a simple header -- 1.7.3.4