cros_image_to_target handles test images, less verbose

cros_image_to_target handles test images, less verbose

added --test option
removed superfluous output (e.g. from dd) unless --verbose given
added --debug option for when you really want that output
added use of pv for a nice progress bar

TEST=(run script several times)
BUG=chromium-os:9563
Change-Id: Ib9d89ad5649b6cb6d5db35f66f6a3fda1ada0c53

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

[adlr, 2010-11-17 20:43:25]

LGTM if you address all the nits

http://codereview.chromium.org/5176002/diff/1/bin/cros_image_to_target.py
File bin/cros_image_to_target.py (right):

http://codereview.chromium.org/5176002/diff/1/bin/cros_image_to_target.py#new...
bin/cros_image_to_target.py:283: verbose=self.verbose, have_pv=self.have_pv))
80 cols here, and a couple lines down as well

http://codereview.chromium.org/5176002/diff/1/bin/cros_image_to_target.py#new...
bin/cros_image_to_target.py:325: self.Info ("Starting client...")
i think style says no spaces between method name and (. Also, I think single
quotes are preferred in python

http://codereview.chromium.org/5176002/diff/1/bin/cros_image_to_target.py#new...
bin/cros_image_to_target.py:357: self.Info ('Client update completed
successfully!')
remove space before (

http://codereview.chromium.org/5176002/diff/1/bin/cros_image_to_target.py#new...
bin/cros_image_to_target.py:604: maybe_build_test = False
it would be nice to have a variable name that more accurately describes its use
(the comment doesn't seem to). would build_test be a better name?

http://codereview.chromium.org/5176002/diff/1/bin/cros_image_to_target.py#new...
bin/cros_image_to_target.py:645: test_output =
cros_env.cmd.Output(cros_env.CrosUtilsPath('enter_chroot.sh'),
a few lines around here (this, 646, 649) are over 80 cols

http://codereview.chromium.org/5176002/diff/1/cros_generate_update_payload
File cros_generate_update_payload (right):

http://codereview.chromium.org/5176002/diff/1/cros_generate_update_payload#ne...
cros_generate_update_payload:29: #echo "Cleaning up"
delete the line or the '#'

http://codereview.chromium.org/5176002/diff/1/cros_generate_update_payload#ne...
cros_generate_update_payload:77: dd if="$filename" of="$temp_file" bs=$bs
count="$length" skip="$offset" 2>/dev/null
wrap to 80 cols

http://codereview.chromium.org/5176002/diff/1/cros_generate_update_payload#ne...
cros_generate_update_payload:85: echo  "   into" $IMAGE
delete extra after echo and add one after 'into'?

[Paul Stewart, 2010-11-17 21:37:36]

I'm glad someone else is getting use out of this.  I have a CL in progress that
uses the stateful image builder (http://codereview.chromium.org/5149002/)
instead of cgpt and friends directly from this script.  Are you successfully
doing updates without that change?

http://codereview.chromium.org/5176002/diff/1/bin/cros_image_to_target.py
File bin/cros_image_to_target.py (right):

http://codereview.chromium.org/5176002/diff/1/bin/cros_image_to_target.py#new...
bin/cros_image_to_target.py:124: '-o', 'ConnectTimeout=3',
Look 2 lines above this one.  I even used a constant. :-)

http://codereview.chromium.org/5176002/diff/1/bin/cros_image_to_target.py#new...
bin/cros_image_to_target.py:160: self.have_pv = (os.system('pv --help >/dev/null
2>&1') == 0)
I'd much prefer if you used self.cmd.Run() instead

http://codereview.chromium.org/5176002/diff/1/bin/cros_image_to_target.py#new...
bin/cros_image_to_target.py:423: fp.close()
If you can't get it working, please revert this change.

[sjg, 2010-11-23 18:49:14]

http://codereview.chromium.org/5176002/diff/1/bin/cros_image_to_target.py
File bin/cros_image_to_target.py (right):

http://codereview.chromium.org/5176002/diff/1/bin/cros_image_to_target.py#new...
bin/cros_image_to_target.py:124: '-o', 'ConnectTimeout=3',
On 2010/11/17 21:37:36, Paul Stewart wrote:
> Look 2 lines above this one.  I even used a constant. :-)

Sorry! It was hanging at some point and I was looking for why...

[sjg, 2010-11-23 21:05:25]

Hi Paul,

Have uploaded some corrections. My main question is the stateful.tgz file -
I altered the script to bring this in, but perhaps this is something you are
doing another way.

Also it seems that we should have a standard library in python for:

- finding the chroot and associated paths, settings
- displaying messages in color
- running programs, pipes, etc.

Does that exist anywhere? It not I could volunteer to start something by
grabbing bits from the scripts.

Regards,
Simon

On Tue, Nov 23, 2010 at 10:49 AM, <sjg@chromium.org> wrote:

>
> http://codereview.chromium.org/5176002/diff/1/bin/cros_image_to_target.py
> File bin/cros_image_to_target.py (right):
>
>
>
http://codereview.chromium.org/5176002/diff/1/bin/cros_image_to_target.py#new...
> bin/cros_image_to_target.py:124: '-o', 'ConnectTimeout=3',
> On 2010/11/17 21:37:36, Paul Stewart wrote:
>
>> Look 2 lines above this one.  I even used a constant. :-)
>>
>
> Sorry! It was hanging at some point and I was looking for why...
>
>
> http://codereview.chromium.org/5176002/
>

[Paul Stewart, 2010-11-23 21:21:33]

I have comments. :-)  I don't think the old stateful is valuable anymore, and
tip-of-trunk cros_image_to_target has removed references to it.  I'd like you to
rebase your work on that.  Otherwise, it's just a matter of a few cleanups.

You might want to chat with Sosa, who also believes we should have a nice
centralized python lib for cros/chroot activities.  I wrote these because there
was none, but it would make sense to move it out so others can share.

http://codereview.chromium.org/5176002/diff/7001/bin/cros_image_to_target.py
File bin/cros_image_to_target.py (right):

http://codereview.chromium.org/5176002/diff/7001/bin/cros_image_to_target.py#...
bin/cros_image_to_target.py:37: STATEFUL_FILENAME = 'stateful.gz'
The old stateful.gz is dead, I believe, and removed in tip of trunk.

http://codereview.chromium.org/5176002/diff/7001/bin/cros_image_to_target.py#...
bin/cros_image_to_target.py:293: verbose=self.verbose, have_pv=self.have_pv))
Please rebase these changes on what's currently in tip of trunk.  I think all
you'll need to do is add your have_pv flag here.  The old stateful file is gone.
 I'd suggest you follow style and newline-indent after commas instead of
de-indenting the line above to get it to fit in 80 cols.

http://codereview.chromium.org/5176002/diff/7001/bin/cros_image_to_target.py#...
bin/cros_image_to_target.py:410: #FIXME: this should use pv also
Use # TODO(sgj): ...

[sjg, 2010-11-23 22:42:38]

Hi Paul,

I have rebased this - your changes to stateful make it quiet a bit cleaner and
answer the question I had. Please can you review once more?

Thanks,
Simon

[Paul Stewart, 2010-11-23 22:49:07]

Almost there!  Just a couple tidy-up comments left.

http://codereview.chromium.org/5176002/diff/12001/bin/cros_image_to_target.py
File bin/cros_image_to_target.py (right):

http://codereview.chromium.org/5176002/diff/12001/bin/cros_image_to_target.py...
bin/cros_image_to_target.py:120: # allow up to 3 seconds to connect
Drop this comment.  The associated code is gone.

http://codereview.chromium.org/5176002/diff/12001/bin/cros_image_to_target.py...
bin/cros_image_to_target.py:150: def __init__(self, verbose=0):
Please make CrossEnv constants as above for SILENT, INFO and DEBUG verbosity
levels, and use throughout.

[sjg, 2010-11-23 23:05:30]

Changed to use constants instead of numbers for verbosity

[Paul Stewart, 2010-11-23 23:09:58]

LGTM.  Thanks for bearing with me!

[sjg, 2010-11-23 23:23:45]

Thanks!

On Tue, Nov 23, 2010 at 3:09 PM, <pstew@chromium.org> wrote:

> LGTM.  Thanks for bearing with me!
>
>
> http://codereview.chromium.org/5176002/
>