cros_utils: refine factory and imaging related scripts

cros_utils: refine factory and imaging related scripts

Major changes are:
 - prefix functions in cros_image_common with 'image' to avoid naming conflicts
 - use awk to replace grep+sed+cut
 - use case to handle board name list
 - factory_setup.sh should return error if applying patch failed
 - refine for shell script coding style guide:
   * replace `` with $()
   * quote variables
   * replace "! -z" by "-n"
   * no space between redirection symbol (>) and target name (eg, >>filename)
   * 1> should be simply >
   * no trailing \ for && and ||

BUG=chrome-os-partner:1583
TEST=manually verified:
 ./image_to_usb.sh --factory # calls mod_image_for_test --factory, and works fine
 ./make_factory_package.sh --factory PATH_TO_IMAGE --release PATH_TO_IMAGE ...
  # factory payloads were created successfully

Change-Id: I6bb10bdfb12cbdb14e9816b3ad72dfe4b7b0472f

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=0629479

[Tom Wai-Hong Tam, 2010-11-18 04:25:50]

LGTM. Thanks for fixing it.

http://codereview.chromium.org/5168001/diff/1/make_factory_package.sh
File make_factory_package.sh (right):

http://codereview.chromium.org/5168001/diff/1/make_factory_package.sh#newcode282
make_factory_package.sh:282: echo "The miniomaha server lives in
src/platform/dev
Better to add a period at the end.

http://codereview.chromium.org/5168001/diff/1/mod_for_factory_scripts/factory...
File mod_for_factory_scripts/factory_setup.sh (right):

http://codereview.chromium.org/5168001/diff/1/mod_for_factory_scripts/factory...
mod_for_factory_scripts/factory_setup.sh:11: for SCRIPT in
"${SCRIPT_BASE}"[0-9][0-9][0-9]*[!$~]
How about quote the whole pattern, like "${SCRIPT_BASE}[0-9][0-9][0-9]*[!$~]"?

[Nick Sanders, 2010-11-18 05:12:08]

http://codereview.chromium.org/5168001/diff/1/lib/cros_image_common.sh
File lib/cros_image_common.sh (right):

http://codereview.chromium.org/5168001/diff/1/lib/cros_image_common.sh#newcode55
lib/cros_image_common.sh:55: cgpt show -b -i "$partno" "$file"
Can you add parted to the chroot, and just use that in all cases, it's best to
minimize the number of paths through the code.

http://codereview.chromium.org/5168001/diff/1/make_factory_package.sh
File make_factory_package.sh (right):

http://codereview.chromium.org/5168001/diff/1/make_factory_package.sh#newcode284
make_factory_package.sh:284: python devserver.py --factory_config miniomaha.conf
--validate_factory_config
not all pythons can work, it needs 2.5+

http://codereview.chromium.org/5168001/diff/1/mod_for_factory_scripts/300modi...
File mod_for_factory_scripts/300modifyDescription (right):

http://codereview.chromium.org/5168001/diff/1/mod_for_factory_scripts/300modi...
mod_for_factory_scripts/300modifyDescription:9: sed -i 's/Test/Factory/' "$FILE"
${FILE} is more standard

http://codereview.chromium.org/5168001/diff/1/mod_for_factory_scripts/500popu...
File mod_for_factory_scripts/500populateQualDbs (right):

http://codereview.chromium.org/5168001/diff/1/mod_for_factory_scripts/500popu...
mod_for_factory_scripts/500populateQualDbs:9: pushd "${TEST_DIR}" >/dev/null
Can you check for existence of dir

http://codereview.chromium.org/5168001/diff/1/mod_for_factory_scripts/500popu...
mod_for_factory_scripts/500populateQualDbs:10: 
Can you also clean 600xxx in mod_for_factory

[Hung-Te, 2010-11-18 05:49:31]

Please take a look again, thanks!

http://codereview.chromium.org/5168001/diff/1/lib/cros_image_common.sh
File lib/cros_image_common.sh (right):

http://codereview.chromium.org/5168001/diff/1/lib/cros_image_common.sh#newcode55
lib/cros_image_common.sh:55: cgpt show -b -i "$partno" "$file"
cgpt is always in chroot. I made this because this script may be invoked inside
/ outside chroot, and even for some non-ubuntu Linux in the future, because a
"factory bundle" may not need entire chrome os source tree, so it'd be better to
have a portable library to work with images

http://codereview.chromium.org/5168001/diff/1/make_factory_package.sh
File make_factory_package.sh (right):

http://codereview.chromium.org/5168001/diff/1/make_factory_package.sh#newcode282
make_factory_package.sh:282: echo "The miniomaha server lives in
src/platform/dev
On 2010/11/18 04:25:50, Tom Wai-Hong Tam wrote:
> Better to add a period at the end.

Done.

http://codereview.chromium.org/5168001/diff/1/make_factory_package.sh#newcode284
make_factory_package.sh:284: python devserver.py --factory_config miniomaha.conf
--validate_factory_config
I thought version check should be done inside the script itself, instead of the
command to invoke it.

Anyway I've put back the 2.6.

http://codereview.chromium.org/5168001/diff/1/mod_for_factory_scripts/300modi...
File mod_for_factory_scripts/300modifyDescription (right):

http://codereview.chromium.org/5168001/diff/1/mod_for_factory_scripts/300modi...
mod_for_factory_scripts/300modifyDescription:9: sed -i 's/Test/Factory/' "$FILE"
oops, I missed this one. Done.

http://codereview.chromium.org/5168001/diff/1/mod_for_factory_scripts/500popu...
File mod_for_factory_scripts/500populateQualDbs (right):

http://codereview.chromium.org/5168001/diff/1/mod_for_factory_scripts/500popu...
mod_for_factory_scripts/500populateQualDbs:9: pushd "${TEST_DIR}" >/dev/null
Just saw you've created another refine for that - I'll wait for yours and merge
with it.

http://codereview.chromium.org/5168001/diff/1/mod_for_factory_scripts/500popu...
mod_for_factory_scripts/500populateQualDbs:10: 
Added in latest CL.

http://codereview.chromium.org/5168001/diff/1/mod_for_factory_scripts/factory...
File mod_for_factory_scripts/factory_setup.sh (right):

http://codereview.chromium.org/5168001/diff/1/mod_for_factory_scripts/factory...
mod_for_factory_scripts/factory_setup.sh:11: for SCRIPT in
"${SCRIPT_BASE}"[0-9][0-9][0-9]*[!$~]
You can't do that. * in quote won't be expanded.

[Nick Sanders, 2010-11-18 06:18:35]

lgtm. Comments below but no changes necessary for this cl

http://codereview.chromium.org/5168001/diff/1/lib/cros_image_common.sh
File lib/cros_image_common.sh (right):

http://codereview.chromium.org/5168001/diff/1/lib/cros_image_common.sh#newcode55
lib/cros_image_common.sh:55: cgpt show -b -i "$partno" "$file"
Err, what I was saying is that we can easily ensure that parted is always in the
chroot, and therefore always use parted. That way it's potable, with less code
duplication.

Since there are many codepaths here, it's likely that the ones run least will
bitrot. Anyways this can be a fix for a later CL.

http://codereview.chromium.org/5168001/diff/1/make_factory_package.sh
File make_factory_package.sh (right):

http://codereview.chromium.org/5168001/diff/1/make_factory_package.sh#newcode284
make_factory_package.sh:284: python devserver.py --factory_config miniomaha.conf
--validate_factory_config
Agree that the check should be in devserver.py, but currently it will just barf
with a cryptic message if you use an older python, so best to specify here until
it's fixed there.

[Hung-Te, 2010-11-18 06:46:24]

http://codereview.chromium.org/5168001/diff/1/lib/cros_image_common.sh
File lib/cros_image_common.sh (right):

http://codereview.chromium.org/5168001/diff/1/lib/cros_image_common.sh#newcode55
lib/cros_image_common.sh:55: cgpt show -b -i "$partno" "$file"
Thanks for the suggestion. It's a good idea to have parted in chroot so we can
always use parted when writing portable scripts to work inside/outside chroot.