Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2873082:
generate_update_payload script: script to generate full/delta update payloads (Closed)
Descriptiongenerate_update_payload script: script to generate full/delta update payloads
This script wraps the old memento udpate generator and the new delta
updater. It will also generate new-style full updates soon, which are
full updates that use the delta-update format, when that code is ready.
BUG=5248
TEST=ran local script to do full/delta update
Patch Set 1 #
Total comments: 33
Patch Set 2 : fixes for review #
Total comments: 1
Messages
Total messages: 7 (0 generated)
I think there's a relatively new convention to prefix new build related scripts with cros_ A bunch of style comments and optional suggestions. Just one correctness question about update.gz's location. http://codereview.chromium.org/2873082/diff/1/2 File generate_update_payload (right): http://codereview.chromium.org/2873082/diff/1/2#newcode3 generate_update_payload:3: # Copyright (c) 2009 The Chromium OS Authors. All rights reserved. 2010 http://codereview.chromium.org/2873082/diff/1/2#newcode7 generate_update_payload:7: # Script to generate an update between two image .bin files. If a source Maybe "Script to generate a Chrome OS update for use by the update engine. If a source..." http://codereview.chromium.org/2873082/diff/1/2#newcode17 generate_update_payload:17: # We need to be in the chroot to emerge test packages. Update comment. Or remove. http://codereview.chromium.org/2873082/diff/1/2#newcode35 generate_update_payload:35: if [ "$FLAGS_output" = "" ]; then Mostly in the interest of saving vertical space, I would: [ -n "$FLAGS_output" ] || die "You must specify..." http://codereview.chromium.org/2873082/diff/1/2#newcode40 generate_update_payload:40: if [ "$FLAGS_src_image" = "" ]; then -z "$FLAGS_src_image" http://codereview.chromium.org/2873082/diff/1/2#newcode45 generate_update_payload:45: echo "Generating a new-style full update" die "Generating a new style..." http://codereview.chromium.org/2873082/diff/1/2#newcode51 generate_update_payload:51: SRC_MNT="" Move these along with the function definitions before the flags processing. This is mixing code with functions (against shell style). http://codereview.chromium.org/2873082/diff/1/2#newcode62 generate_update_payload:62: if [ "$SRC_MNT" != "" ]; then -n "$SRC_MNT" Same for "$DST_MNT" below. http://codereview.chromium.org/2873082/diff/1/2#newcode64 generate_update_payload:64: if [ -d "$SRC_MNT" ]; then [ -d "$SRC_MNT" ] && rmdir "$SRC_MNT" Same for "$DST_MNT" below. http://codereview.chromium.org/2873082/diff/1/2#newcode80 generate_update_payload:80: if [ "$1" = "" ]; then [ -z "$1" ] || exit 1 http://codereview.chromium.org/2873082/diff/1/2#newcode86 generate_update_payload:86: FILENAME="$1" If you'd like to follow the shell style, local variables are lower case. http://codereview.chromium.org/2873082/diff/1/2#newcode94 generate_update_payload:94: if [ $(( ($OFFSET % $SECTORS_PER_TWO_MIB) + \ Does it make sense to split this into the -eq 0 expressions, anded together, for clarity? http://codereview.chromium.org/2873082/diff/1/2#newcode100 generate_update_payload:100: echo "warning: partition offset or length not at 2MiB boundary" There's a "warn" routine in common.sh. So, maybe: warn "Partition offset..." http://codereview.chromium.org/2873082/diff/1/2#newcode110 generate_update_payload:110: GENERATOR="$(dirname "$0")/../platform/update_engine/delta_generator" I suspect we'll need to fix this with the new cros workon flow -- i.e., you can't assume that another repo is checked out somewhere relative to the current repo. Maybe the delta_generator needs to be installed as part of hard host depends? http://codereview.chromium.org/2873082/diff/1/2#newcode111 generate_update_payload:111: if [ ! -x "$GENERATOR" ]; then [ -x "$GENERATOR" ] || die "$GENERATOR doesn't..." http://codereview.chromium.org/2873082/diff/1/2#newcode146 generate_update_payload:146: mv "$(dirname "$DST_KERNEL")/update.gz" "$FLAGS_output" I'm a bit confused. DST_KERNEL is a file right under /tmp generated by extract_partition_to_temp_file, right? So dirname $DST_KERNEL would return /tmp. Are you moving /tmp/update.gz here?
http://codereview.chromium.org/2873082/diff/1/2 File generate_update_payload (right): http://codereview.chromium.org/2873082/diff/1/2#newcode80 generate_update_payload:80: if [ "$1" = "" ]; then On 2010/07/30 05:02:11, petkov wrote: > [ -z "$1" ] || exit 1 > Actually, this should be: [ -n "$1" ] || exit 1
PTAL http://codereview.chromium.org/2873082/diff/1/2 File generate_update_payload (right): http://codereview.chromium.org/2873082/diff/1/2#newcode3 generate_update_payload:3: # Copyright (c) 2009 The Chromium OS Authors. All rights reserved. On 2010/07/30 05:02:11, petkov wrote: > 2010 > Done. http://codereview.chromium.org/2873082/diff/1/2#newcode7 generate_update_payload:7: # Script to generate an update between two image .bin files. If a source On 2010/07/30 05:02:11, petkov wrote: > Maybe "Script to generate a Chrome OS update for use by the update engine. If a > source..." > Done. http://codereview.chromium.org/2873082/diff/1/2#newcode17 generate_update_payload:17: # We need to be in the chroot to emerge test packages. On 2010/07/30 05:02:11, petkov wrote: > Update comment. Or remove. > Done. http://codereview.chromium.org/2873082/diff/1/2#newcode35 generate_update_payload:35: if [ "$FLAGS_output" = "" ]; then On 2010/07/30 05:02:11, petkov wrote: > Mostly in the interest of saving vertical space, I would: > > [ -n "$FLAGS_output" ] || die "You must specify..." > Done. http://codereview.chromium.org/2873082/diff/1/2#newcode40 generate_update_payload:40: if [ "$FLAGS_src_image" = "" ]; then On 2010/07/30 05:02:11, petkov wrote: > -z "$FLAGS_src_image" > Done. http://codereview.chromium.org/2873082/diff/1/2#newcode45 generate_update_payload:45: echo "Generating a new-style full update" On 2010/07/30 05:02:11, petkov wrote: > die "Generating a new style..." > Done. http://codereview.chromium.org/2873082/diff/1/2#newcode51 generate_update_payload:51: SRC_MNT="" On 2010/07/30 05:02:11, petkov wrote: > Move these along with the function definitions before the flags processing. This > is mixing code with functions (against shell style). > Done. http://codereview.chromium.org/2873082/diff/1/2#newcode62 generate_update_payload:62: if [ "$SRC_MNT" != "" ]; then On 2010/07/30 05:02:11, petkov wrote: > -n "$SRC_MNT" > > Same for "$DST_MNT" below. > Done. http://codereview.chromium.org/2873082/diff/1/2#newcode64 generate_update_payload:64: if [ -d "$SRC_MNT" ]; then On 2010/07/30 05:02:11, petkov wrote: > [ -d "$SRC_MNT" ] && rmdir "$SRC_MNT" > > Same for "$DST_MNT" below. > Done. http://codereview.chromium.org/2873082/diff/1/2#newcode80 generate_update_payload:80: if [ "$1" = "" ]; then On 2010/07/30 06:31:11, petkov wrote: > On 2010/07/30 05:02:11, petkov wrote: > > [ -z "$1" ] || exit 1 > > > > Actually, this should be: > > [ -n "$1" ] || exit 1 > Done. http://codereview.chromium.org/2873082/diff/1/2#newcode86 generate_update_payload:86: FILENAME="$1" On 2010/07/30 05:02:11, petkov wrote: > If you'd like to follow the shell style, local variables are lower case. > Done. http://codereview.chromium.org/2873082/diff/1/2#newcode94 generate_update_payload:94: if [ $(( ($OFFSET % $SECTORS_PER_TWO_MIB) + \ On 2010/07/30 05:02:11, petkov wrote: > Does it make sense to split this into the -eq 0 expressions, anded together, for > clarity? > Done. http://codereview.chromium.org/2873082/diff/1/2#newcode100 generate_update_payload:100: echo "warning: partition offset or length not at 2MiB boundary" On 2010/07/30 05:02:11, petkov wrote: > There's a "warn" routine in common.sh. So, maybe: > > warn "Partition offset..." > Done. http://codereview.chromium.org/2873082/diff/1/2#newcode110 generate_update_payload:110: GENERATOR="$(dirname "$0")/../platform/update_engine/delta_generator" On 2010/07/30 05:02:11, petkov wrote: > I suspect we'll need to fix this with the new cros workon flow -- i.e., you > can't assume that another repo is checked out somewhere relative to the current > repo. Maybe the delta_generator needs to be installed as part of hard host > depends? > > hm, yeah, we'll have to make a new ebuild for that. let's do it in another CL. http://codereview.chromium.org/2873082/diff/1/2#newcode111 generate_update_payload:111: if [ ! -x "$GENERATOR" ]; then On 2010/07/30 05:02:11, petkov wrote: > [ -x "$GENERATOR" ] || die "$GENERATOR doesn't..." > Done. http://codereview.chromium.org/2873082/diff/1/2#newcode146 generate_update_payload:146: mv "$(dirname "$DST_KERNEL")/update.gz" "$FLAGS_output" On 2010/07/30 05:02:11, petkov wrote: > I'm a bit confused. DST_KERNEL is a file right under /tmp generated by > extract_partition_to_temp_file, right? So dirname $DST_KERNEL would return /tmp. > Are you moving /tmp/update.gz here? > the generator here puts the output file in the same dir as the input kernel. So, i'm moving it out from there to the output file.
Thanks for the change to cros_* . Could we also put it in the crosutils.git:bin/ directory? The crosutils ebuild (pulled via hard host) installs the bin/ contents in the chroot /usr/bin/ so the script can be run from anywhere. (once we a few pending changes to clean up using common.sh paths etc)
LGTM w/ a nit. http://codereview.chromium.org/2873082/diff/6001/7001 File cros_generate_update_payload (right): http://codereview.chromium.org/2873082/diff/6001/7001#newcode82 cros_generate_update_payload:82: if [ -z "$STATE_LOOP_DEV" ]; then [ -n "$STATE_LOOP_DEV" ] || die "no free loop device"
thanks. fixed and pushed |
|||||||||||||||||||||||||||||||||||||||||||||||
