Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
5271010:
Factored out the code to copy an image and modify it for test (Closed)
|
Created:
10 years ago by sjg Modified:
9 years, 7 months ago CC:
chromium-os-reviews_chromium.org, Mandeep Singh Baines, anush, sosa Visibility:
Public. |
DescriptionFactored out the code to copy an image and modify it for test
into mod_image_for_test.sh rather than slightly different versions
of the same in image_to_usb.sh and image_to_vm.sh
Added a function to get a test image into common.sh
Added --inplace option to mod_image_for_test, which is the default,
and preserves the original behaviour. But using --noinplace it will
now do the copy for you.
Found that chromiumos_image.bin appears throughout the scripts, so added it and the test variant to common.sh
BUG=chromiumos-10126
TEST=run mod_image_for_test.sh with and without --noinplace
run image_to_usb.sh and image_to_vm.sh with both options
test on Seaboard that correct image is provided
Really we should have automated testing for these scripts
Change-Id: I5cfa91792c7fded35e7f4ca8f8f27c6b270817fb
Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=4fc5227
Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=142ca06
Patch Set 1 #
Total comments: 26
Patch Set 2 : Changes as suggested #
Total comments: 10
Patch Set 3 : Minor changes #
Total comments: 6
Patch Set 4 : Changed to use a temp variable #
Total comments: 2
Patch Set 5 : Update to try to fix buildbot issue #Patch Set 6 : Rebased to ToT #
Total comments: 2
Patch Set 7 : Default force_copy to TRUE in image_to_vm.sh #Patch Set 8 : Final upload to check before push #
Messages
Total messages: 18 (0 generated)
http://codereview.chromium.org/5271010/diff/1/image_to_usb.sh File image_to_usb.sh (right): http://codereview.chromium.org/5271010/diff/1/image_to_usb.sh#newcode150 image_to_usb.sh:150: SRC_IMAGE="${FLAGS_from}/${FLAGS_image_name}" This was moved to make the script more similar to image_to_vm.sh http://codereview.chromium.org/5271010/diff/1/mod_image_for_test.sh File mod_image_for_test.sh (right): http://codereview.chromium.org/5271010/diff/1/mod_image_for_test.sh#newcode36 mod_image_for_test.sh:36: DEFINE_boolean inplace $FLAGS_TRUE \ I used 'inplace' since possibly this could become default 'false' in future.
http://codereview.chromium.org/5271010/diff/1/common.sh File common.sh (right): http://codereview.chromium.org/5271010/diff/1/common.sh#newcode18 common.sh:18: PV_OK=1 These vars are pretty short, watch out for name conflicts. Might be better to prepend with something more unique .. i.e. COMMON_ http://codereview.chromium.org/5271010/diff/1/common.sh#newcode139 common.sh:139: # Standard filenames Check for name collisions in other files http://codereview.chromium.org/5271010/diff/1/common.sh#newcode553 common.sh:553: # On exit, SRC_IMAGE contains the pathname of the resulting test image Treat this more like a function ... i.e. don't modify SRC_IMAGE in place rather take it as an arg ... i.e. arg 1 ... and echo the new path at the end ... that way callees can do SRC_IMAGE="$(prepare_test_image)" Rather than rely on some underlying mechanism to change a global var http://codereview.chromium.org/5271010/diff/1/common.sh#newcode555 common.sh:555: prepare_test_image() { Indent http://codereview.chromium.org/5271010/diff/1/common.sh#newcode560 common.sh:560: EXTRA_ARGS="--factory" use locals http://codereview.chromium.org/5271010/diff/1/common.sh#newcode564 common.sh:564: if [ ${FLAGS_factory_install} -eq ${FLAGS_TRUE} ] ; then style is no space between ] and ;. This has changed so some other places might be wrong http://codereview.chromium.org/5271010/diff/1/common.sh#newcode570 common.sh:570: EXTRA_ARGS+="--force_copy" Does this work? +=? Is that a bashism? Why not EXTRA_ARGS="${EXTRA_ARGS} --force_copy" http://codereview.chromium.org/5271010/diff/1/common.sh#newcode573 common.sh:573: # modified the image for test, creating a new test image s/m/M http://codereview.chromium.org/5271010/diff/1/common.sh#newcode574 common.sh:574: "${SCRIPTS_DIR}/mod_image_for_test.sh" --board=${FLAGS_board} --image \ Use = consistently in options http://codereview.chromium.org/5271010/diff/1/common.sh#newcode577 common.sh:577: # from now on we use the just-created test image s/f/F http://codereview.chromium.org/5271010/diff/1/image_to_usb.sh File image_to_usb.sh (right): http://codereview.chromium.org/5271010/diff/1/image_to_usb.sh#newcode3 image_to_usb.sh:3: # Copyright (c) 2010 The Chromium OS Authors. All rights reserved. 2009-2010 http://codereview.chromium.org/5271010/diff/1/image_to_usb.sh#newcode85 image_to_usb.sh:85: # get latest image directory s/get/Get http://codereview.chromium.org/5271010/diff/1/image_to_usb.sh#newcode153 image_to_usb.sh:153: # make a test image as requested, this will modify SRC_IMAGE s/make/Make also why are we modifying the src image? http://codereview.chromium.org/5271010/diff/1/image_to_vm.sh File image_to_vm.sh (right): http://codereview.chromium.org/5271010/diff/1/image_to_vm.sh#newcode93 image_to_vm.sh:93: # make a test image as requested, this will modify SRC_IMAGE again same question http://codereview.chromium.org/5271010/diff/1/mod_image_for_test.sh File mod_image_for_test.sh (right): http://codereview.chromium.org/5271010/diff/1/mod_image_for_test.sh#newcode39 mod_image_for_test.sh:39: if needed, and modified there" o Short var doesn't really help here? http://codereview.chromium.org/5271010/diff/1/mod_image_for_test.sh#newcode40 mod_image_for_test.sh:40: DEFINE_boolean force_copy ${FLAGS_FALSE} \ Use exact def ... i.e. always rebuild test image if --noinplace http://codereview.chromium.org/5271010/diff/1/mod_image_for_test.sh#newcode164 mod_image_for_test.sh:164: IMAGE_DIR="$(dirname "$FLAGS_image")" {} http://codereview.chromium.org/5271010/diff/1/mod_image_for_test.sh#newcode167 mod_image_for_test.sh:167: if [ $FLAGS_inplace -eq $FLAGS_FALSE ]; then be consistent with use of {}
Also please file a cleanup issue to fill in the bug! Thanks for looking into this On Tue, Nov 30, 2010 at 2:01 PM, <sosa@chromium.org> wrote: > > http://codereview.chromium.org/5271010/diff/1/common.sh > File common.sh (right): > > http://codereview.chromium.org/5271010/diff/1/common.sh#newcode18 > common.sh:18: PV_OK=1 > These vars are pretty short, watch out for name conflicts. Might be > better to prepend with something more unique .. i.e. COMMON_ > > http://codereview.chromium.org/5271010/diff/1/common.sh#newcode139 > common.sh:139: # Standard filenames > Check for name collisions in other files > > http://codereview.chromium.org/5271010/diff/1/common.sh#newcode553 > common.sh:553: # On exit, SRC_IMAGE contains the pathname of the > resulting test image > Treat this more like a function ... i.e. don't modify SRC_IMAGE in > place rather take it as an arg ... i.e. arg 1 ... and echo the new path > at the end ... that way callees can do > > SRC_IMAGE="$(prepare_test_image)" > > Rather than rely on some underlying mechanism to change a global var > > http://codereview.chromium.org/5271010/diff/1/common.sh#newcode555 > common.sh:555: prepare_test_image() { > Indent > > http://codereview.chromium.org/5271010/diff/1/common.sh#newcode560 > common.sh:560: EXTRA_ARGS="--factory" > use locals > > http://codereview.chromium.org/5271010/diff/1/common.sh#newcode564 > common.sh:564: if [ ${FLAGS_factory_install} -eq ${FLAGS_TRUE} ] ; then > style is no space between ] and ;. This has changed so some other > places might be wrong > > http://codereview.chromium.org/5271010/diff/1/common.sh#newcode570 > common.sh:570: EXTRA_ARGS+="--force_copy" > Does this work? +=? Is that a bashism? Why not > EXTRA_ARGS="${EXTRA_ARGS} --force_copy" > > http://codereview.chromium.org/5271010/diff/1/common.sh#newcode573 > common.sh:573: # modified the image for test, creating a new test image > s/m/M > > http://codereview.chromium.org/5271010/diff/1/common.sh#newcode574 > common.sh:574: "${SCRIPTS_DIR}/mod_image_for_test.sh" > --board=${FLAGS_board} --image \ > Use = consistently in options > > http://codereview.chromium.org/5271010/diff/1/common.sh#newcode577 > common.sh:577: # from now on we use the just-created test image > s/f/F > > http://codereview.chromium.org/5271010/diff/1/image_to_usb.sh > File image_to_usb.sh (right): > > http://codereview.chromium.org/5271010/diff/1/image_to_usb.sh#newcode3 > image_to_usb.sh:3: # Copyright (c) 2010 The Chromium OS Authors. All > rights reserved. > 2009-2010 > > http://codereview.chromium.org/5271010/diff/1/image_to_usb.sh#newcode85 > image_to_usb.sh:85: # get latest image directory > s/get/Get > > http://codereview.chromium.org/5271010/diff/1/image_to_usb.sh#newcode153 > image_to_usb.sh:153: # make a test image as requested, this will modify > SRC_IMAGE > s/make/Make also why are we modifying the src image? > > http://codereview.chromium.org/5271010/diff/1/image_to_vm.sh > File image_to_vm.sh (right): > > http://codereview.chromium.org/5271010/diff/1/image_to_vm.sh#newcode93 > image_to_vm.sh:93: # make a test image as requested, this will modify > SRC_IMAGE > again same question > > http://codereview.chromium.org/5271010/diff/1/mod_image_for_test.sh > File mod_image_for_test.sh (right): > > http://codereview.chromium.org/5271010/diff/1/mod_image_for_test.sh#newcode39 > mod_image_for_test.sh:39: if needed, and modified there" o > Short var doesn't really help here? > > http://codereview.chromium.org/5271010/diff/1/mod_image_for_test.sh#newcode40 > mod_image_for_test.sh:40: DEFINE_boolean force_copy ${FLAGS_FALSE} \ > Use exact def ... i.e. always rebuild test image if --noinplace > > http://codereview.chromium.org/5271010/diff/1/mod_image_for_test.sh#newcode164 > mod_image_for_test.sh:164: IMAGE_DIR="$(dirname "$FLAGS_image")" > {} > > http://codereview.chromium.org/5271010/diff/1/mod_image_for_test.sh#newcode167 > mod_image_for_test.sh:167: if [ $FLAGS_inplace -eq $FLAGS_FALSE ]; then > be consistent with use of {} > > http://codereview.chromium.org/5271010/ >
Hi, Have cleaned up everything except this... Re the function request, this is hard, because mod_image_for_test generates lots of output. It means that I need to redirect it and then perhaps print it afterwards on failure. Are you sure this is the best approach? Perhaps there is another way to return a pathname from prepare_test_image()? Regards, Simon http://codereview.chromium.org/5271010/diff/1/common.sh File common.sh (right): http://codereview.chromium.org/5271010/diff/1/common.sh#newcode18 common.sh:18: PV_OK=1 On 2010/11/30 22:01:43, sosa wrote: > These vars are pretty short, watch out for name conflicts. Might be better to > prepend with something more unique .. i.e. COMMON_ Done http://codereview.chromium.org/5271010/diff/1/common.sh#newcode142 common.sh:142: There are none. cd .../src/scripts grep CHROMEOS_IMAGE grep CHROMEOS_TEST * http://codereview.chromium.org/5271010/diff/1/common.sh#newcode553 common.sh:553: # On exit, SRC_IMAGE contains the pathname of the resulting test image On 2010/11/30 22:01:43, sosa wrote: > Treat this more like a function ... i.e. don't modify SRC_IMAGE in place rather > take it as an arg ... i.e. arg 1 ... and echo the new path at the end ... that > way callees can do > > SRC_IMAGE="$(prepare_test_image)" > > Rather than rely on some underlying mechanism to change a global var Actually this is hard, because mod_image_for_test generates lots of output. It means that I need to redirect it and then perhaps print it afterwards on failure. Are you sure this is the best approach? http://codereview.chromium.org/5271010/diff/1/image_to_usb.sh File image_to_usb.sh (right): http://codereview.chromium.org/5271010/diff/1/image_to_usb.sh#newcode153 image_to_usb.sh:153: # make a test image as requested, this will modify SRC_IMAGE On 2010/11/30 22:01:43, sosa wrote: > s/make/Make also why are we modifying the src image? We will change it to point to the test image.
Why not just redirect to stderr i.e. ./mod_image_for_test.sh > /dev/stderr On Tue, Nov 30, 2010 at 4:57 PM, <sjg@chromium.org> wrote: > Hi, > > Have cleaned up everything except this... > > Re the function request, this is hard, because mod_image_for_test generates > lots > of output. It means that I need to redirect it and then perhaps print it > afterwards on failure. Are you sure this is the best approach? > > Perhaps there is another way to return a pathname from prepare_test_image()? > > Regards, > Simon > > > > http://codereview.chromium.org/5271010/diff/1/common.sh > File common.sh (right): > > http://codereview.chromium.org/5271010/diff/1/common.sh#newcode18 > common.sh:18: PV_OK=1 > On 2010/11/30 22:01:43, sosa wrote: >> >> These vars are pretty short, watch out for name conflicts. Might be > > better to >> >> prepend with something more unique .. i.e. COMMON_ > > Done > > http://codereview.chromium.org/5271010/diff/1/common.sh#newcode142 > common.sh:142: > There are none. > > cd .../src/scripts > grep CHROMEOS_IMAGE > grep CHROMEOS_TEST * > > http://codereview.chromium.org/5271010/diff/1/common.sh#newcode553 > common.sh:553: # On exit, SRC_IMAGE contains the pathname of the > resulting test image > On 2010/11/30 22:01:43, sosa wrote: >> >> Treat this more like a function ... i.e. don't modify SRC_IMAGE in > > place rather >> >> take it as an arg ... i.e. arg 1 ... and echo the new path at the end > > ... that >> >> way callees can do > >> SRC_IMAGE="$(prepare_test_image)" > >> Rather than rely on some underlying mechanism to change a global var > > Actually this is hard, because mod_image_for_test generates lots of > output. It means that I need to redirect it and then perhaps print it > afterwards on failure. Are you sure this is the best approach? > > http://codereview.chromium.org/5271010/diff/1/image_to_usb.sh > File image_to_usb.sh (right): > > http://codereview.chromium.org/5271010/diff/1/image_to_usb.sh#newcode153 > image_to_usb.sh:153: # make a test image as requested, this will modify > SRC_IMAGE > On 2010/11/30 22:01:43, sosa wrote: >> >> s/make/Make also why are we modifying the src image? > > We will change it to point to the test image. > > http://codereview.chromium.org/5271010/ >
Hi, Have made all these changes. Also found another use of pv. I hope it is OK to replace cp -f a b with cat a >b - Simon http://codereview.chromium.org/5271010/diff/1/common.sh File common.sh (right): http://codereview.chromium.org/5271010/diff/1/common.sh#newcode553 common.sh:553: # On exit, SRC_IMAGE contains the pathname of the resulting test image On 2010/12/01 00:57:05, sjg wrote: > On 2010/11/30 22:01:43, sosa wrote: > > Treat this more like a function ... i.e. don't modify SRC_IMAGE in place > rather > > take it as an arg ... i.e. arg 1 ... and echo the new path at the end ... that > > way callees can do > > > > SRC_IMAGE="$(prepare_test_image)" > > > > Rather than rely on some underlying mechanism to change a global var > > Actually this is hard, because mod_image_for_test generates lots of output. It > means that I need to redirect it and then perhaps print it afterwards on > failure. Are you sure this is the best approach? OK have piped the script to stderr as you suggest. It seems fine. http://codereview.chromium.org/5271010/diff/1/mod_image_for_test.sh File mod_image_for_test.sh (right): http://codereview.chromium.org/5271010/diff/1/mod_image_for_test.sh#newcode39 mod_image_for_test.sh:39: if needed, and modified there" o On 2010/11/30 22:01:43, sosa wrote: > Short var doesn't really help here? Yes have r3emoved it.
http://codereview.chromium.org/5271010/diff/10001/common.sh File common.sh (right): http://codereview.chromium.org/5271010/diff/10001/common.sh#newcode556 common.sh:556: # SRC_IMAGE=$(prepare_test_image "/path/to/image") Dcoument both $1 and $2 ... i only see $1 specified in the usage http://codereview.chromium.org/5271010/diff/10001/common.sh#newcode557 common.sh:557: Remove extra line here http://codereview.chromium.org/5271010/diff/10001/common.sh#newcode569 common.sh:569: if [ ${FLAGS_factory_install} -eq ${FLAGS_TRUE} ]; then Just checking but these two can't be set at the same time? http://codereview.chromium.org/5271010/diff/10001/common.sh#newcode575 common.sh:575: args="$args --force_copy" ${args} http://codereview.chromium.org/5271010/diff/10001/common.sh#newcode579 common.sh:579: "${SCRIPTS_DIR}/mod_image_for_test.sh" --board=${FLAGS_board} --image=\ I believe having the = at the break point will fail. Move --image down to the next line (cause this turns into --image= "image_path" http://codereview.chromium.org/5271010/diff/10001/image_to_usb.sh File image_to_usb.sh (right): http://codereview.chromium.org/5271010/diff/10001/image_to_usb.sh#newcode236 image_to_usb.sh:236: ${COMMON_PV_CAT} "${SRC_IMAGE}" >"${FLAGS_to}" Was just looking at this ... ummm why do we even have a install flag???? You can dd from a file to another file. Seems kind of like a lot of extra logic for no gain. Arrrrgh ... anyway was just a side comment http://codereview.chromium.org/5271010/diff/10001/mod_image_for_test.sh File mod_image_for_test.sh (right): http://codereview.chromium.org/5271010/diff/10001/mod_image_for_test.sh#newco... mod_image_for_test.sh:176: echo "Using cached test image" Don't you have to set FLAGS_image here too? Might want to set unconditionally
http://codereview.chromium.org/5271010/diff/10001/common.sh File common.sh (right): http://codereview.chromium.org/5271010/diff/10001/common.sh#newcode569 common.sh:569: if [ ${FLAGS_factory_install} -eq ${FLAGS_TRUE} ]; then On 2010/12/01 20:37:45, sosa wrote: > Just checking but these two can't be set at the same time? Yes I noticed this - I copied the code as before. In mod_image_for_test they have separate code paths so I think this is ok. http://codereview.chromium.org/5271010/diff/10001/common.sh#newcode579 common.sh:579: "${SCRIPTS_DIR}/mod_image_for_test.sh" --board=${FLAGS_board} --image=\ On 2010/12/01 20:37:45, sosa wrote: > I believe having the = at the break point will fail. Move --image down to the > next line (cause this turns into --image= "image_path" It seems to work, but I have changed it http://codereview.chromium.org/5271010/diff/10001/mod_image_for_test.sh File mod_image_for_test.sh (right): http://codereview.chromium.org/5271010/diff/10001/mod_image_for_test.sh#newco... mod_image_for_test.sh:176: echo "Using cached test image" On 2010/12/01 20:37:45, sosa wrote: > Don't you have to set FLAGS_image here too? Might want to set unconditionally Actually it is a flag, and is set further up the file. I change it only if we don't want inplace behavior.
http://codereview.chromium.org/5271010/diff/16001/common.sh File common.sh (right): http://codereview.chromium.org/5271010/diff/16001/common.sh#newcode141 common.sh:141: CHROMEOS_TEST_IMAGE_NAME="chromiumos_test_image.bin" Can you modify the output name based on test/factory/factory_install http://codereview.chromium.org/5271010/diff/16001/mod_image_for_test.sh File mod_image_for_test.sh (right): http://codereview.chromium.org/5271010/diff/16001/mod_image_for_test.sh#newco... mod_image_for_test.sh:176: echo "Using cached test image" This particular option is targeted to more quickly write a test image to usb, is there a reason to pull it into mod_image_for_test? I can't think of any situation why someone would call mod_image_for_test directly, and want to receive stale output. Maybe just have image_to_usb not call mod_image_for _test at all if it has a cached image?
http://codereview.chromium.org/5271010/diff/16001/common.sh File common.sh (right): http://codereview.chromium.org/5271010/diff/16001/common.sh#newcode579 common.sh:579: --image="$1/$2" --noinplace ${args} >/dev/stderr Can you get this output on stdout? It doesn't seem like a good idea to change the nonerror message output stream to stderr. This will make is difficult and annoying to grep, redirect to a file, or silence.
http://codereview.chromium.org/5271010/diff/16001/common.sh File common.sh (right): http://codereview.chromium.org/5271010/diff/16001/common.sh#newcode141 common.sh:141: CHROMEOS_TEST_IMAGE_NAME="chromiumos_test_image.bin" On 2010/12/02 00:01:48, Nick Sanders wrote: > Can you modify the output name based on test/factory/factory_install I haven't added constants for these also at this stage. Nor have I used my constants in all the other scripts. http://codereview.chromium.org/5271010/diff/16001/common.sh#newcode579 common.sh:579: --image="$1/$2" --noinplace ${args} >/dev/stderr On 2010/12/02 00:07:51, Nick Sanders wrote: > Can you get this output on stdout? It doesn't seem like a good idea to change > the nonerror message output stream to stderr. This will make is difficult and > annoying to grep, redirect to a file, or silence. Chris suggested making this a function which takes arguments and returns a result on stdout. See the first patch set for how it was originally done. I don't know of a way of doing this without creating a lot of mess. I do want the output to appear as it goes, but I agree it is changing behavior to put it on stderr. Which option is best? http://codereview.chromium.org/5271010/diff/16001/mod_image_for_test.sh File mod_image_for_test.sh (right): http://codereview.chromium.org/5271010/diff/16001/mod_image_for_test.sh#newco... mod_image_for_test.sh:176: echo "Using cached test image" On 2010/12/02 00:01:48, Nick Sanders wrote: > This particular option is targeted to more quickly write a test image to usb, is > there a reason to pull it into mod_image_for_test? > > I can't think of any situation why someone would call mod_image_for_test > directly, and want to receive stale output. Maybe just have image_to_usb not > call mod_image_for _test at all if it has a cached image? It's just that it starts with the existing previously-cached test image. It still updates it and makes sure it has all the latest test packages. So I don't think this is useless behavior. But I might be wrong.
Hi, I have made the changes as suggested - did either of you see it come through? Regards, Simon
LGTM w/ quote nits http://codereview.chromium.org/5271010/diff/23001/image_to_usb.sh File image_to_usb.sh (right): http://codereview.chromium.org/5271010/diff/23001/image_to_usb.sh#newcode152 image_to_usb.sh:152: SRC_IMAGE=${CHROMEOS_RETURN_VAL} Quotes around val http://codereview.chromium.org/5271010/diff/23001/image_to_vm.sh File image_to_vm.sh (right): http://codereview.chromium.org/5271010/diff/23001/image_to_vm.sh#newcode92 image_to_vm.sh:92: SRC_IMAGE=${CHROMEOS_RETURN_VAL} Quotes around val
Tested ... added comments with why the pfq failed http://codereview.chromium.org/5271010/diff/31002/image_to_vm.sh File image_to_vm.sh (right): http://codereview.chromium.org/5271010/diff/31002/image_to_vm.sh#newcode113 image_to_vm.sh:113: prepare_test_image "${FLAGS_from}" "${CHROMEOS_IMAGE_NAME}" Need to add FLAGS_force_copy=$FLAGS_TRUE here or add as a flag that defaults to true to this flags list. This is what killed the pfq (i think) http://codereview.chromium.org/5271010/diff/31002/image_to_vm.sh#newcode285 image_to_vm.sh:285: echo "sudo kvm -m ${FLAGS_mem} -vga std -pidfile /tmp/kvm.pid -net nic,model=e1000 " \ This got accidentally reverted
actually force_copy is just set to false here but true otherwise ... prob just need to flip that.
Ok here it is with the fix - thanks, Simon
LGTM again! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
