Re-write of autoupdate.py to merge changes to be more cohesive and allow a forced_image option.

Re-write of autoupdate.py to merge changes to be more cohesive and allow a forced_image option.

BUG=
TEST=More tests pending.  Ran unittests, ran manual for latest and ran for ForcedBuild option all successfully.

[sosa, 2010-09-14 17:12:56]

Doing more testing.  If you can suggest more test methods, let me know.

[seano, 2010-09-14 21:08:07]

http://codereview.chromium.org/3296022/diff/6001/7001
File autoupdate.py (right):

http://codereview.chromium.org/3296022/diff/6001/7001#newcode286
autoupdate.py:286: cached_file_path = os.path.join(static_image_dir,
'update.gz')
Is update.gz still the only generated file that we care about? (I think so, just
checking... stateful.image.gz, other stuff?)

http://codereview.chromium.org/3296022/diff/6001/7001#newcode464
autoupdate.py:464: url = 'http://%s/static/update.gz' % hostname
static/archive?

http://codereview.chromium.org/3296022/diff/6001/7003
File devserver.py (left):

http://codereview.chromium.org/3296022/diff/6001/7003#oldcode66
devserver.py:66: default='MementoSoftwareUpdate')
Can we officially change this to the update engine client prefix?

http://codereview.chromium.org/3296022/diff/6001/7003#oldcode69
devserver.py:69: parser.add_option('-t', action='store_true', dest='test_image')
I've always wanted to get rid of this, and serve the test/regular image based on
the ping. Is this any more possible after your changes?

http://codereview.chromium.org/3296022/diff/6001/7003
File devserver.py (right):

http://codereview.chromium.org/3296022/diff/6001/7003#newcode91
devserver.py:91: sys.argv.remove(options.image)
Maybe we should just clean argv in a loop over options after processing all
args, vs hard-coding removal in each case.

[sosa, 2010-09-14 23:30:33]

http://codereview.chromium.org/3296022/diff/6001/7001
File autoupdate.py (right):

http://codereview.chromium.org/3296022/diff/6001/7001#newcode286
autoupdate.py:286: cached_file_path = os.path.join(static_image_dir,
'update.gz')
Stateful should be 1:1 with image so this should be sufficient.

On 2010/09/14 21:08:07, seano wrote:
> Is update.gz still the only generated file that we care about? (I think so,
just
> checking... stateful.image.gz, other stuff?)

http://codereview.chromium.org/3296022/diff/6001/7001#newcode464
autoupdate.py:464: url = 'http://%s/static/update.gz' % hostname
On 2010/09/14 21:08:07, seano wrote:
> static/archive?

Fixed to make this more general

http://codereview.chromium.org/3296022/diff/6001/7003
File devserver.py (left):

http://codereview.chromium.org/3296022/diff/6001/7003#oldcode66
devserver.py:66: default='MementoSoftwareUpdate')
Not yet, factory still heavily using this.

On 2010/09/14 21:08:07, seano wrote:
> Can we officially change this to the update engine client prefix?

http://codereview.chromium.org/3296022/diff/6001/7003#oldcode69
devserver.py:69: parser.add_option('-t', action='store_true', dest='test_image')
We could do it, but often is the case that test images are
chromiumos_image.bin's (default for mod_image_for_test).  Maybe we should change
mod_image_for_test to create something else but that's a different debate.

On 2010/09/14 21:08:07, seano wrote:
> I've always wanted to get rid of this, and serve the test/regular image based
on
> the ping. Is this any more possible after your changes?

http://codereview.chromium.org/3296022/diff/6001/7003
File devserver.py (right):

http://codereview.chromium.org/3296022/diff/6001/7003#newcode91
devserver.py:91: sys.argv.remove(options.image)
On 2010/09/14 21:08:07, seano wrote:
> Maybe we should just clean argv in a loop over options after processing all
> args, vs hard-coding removal in each case.

Done.  This was really easy once I figured out you can just pass in sys.argv and
OptionParser already does the magic

[sosa, 2010-09-15 00:31:45]

I also refactored __init__ a bit.  Calling functions and doing complex stuff in
__init__ is generally bad :)

[seano, 2010-09-15 14:23:59]

LGTM

Yay, unit tests!

http://codereview.chromium.org/3296022/diff/22001/23003
File devserver.py (right):

http://codereview.chromium.org/3296022/diff/22001/23003#newcode100
devserver.py:100: options, sys.argv = parser.parse_args(sys.argv)
Yeah, that's much nicer :)

[sosa, 2010-09-15 17:55:11]

Thanks seano!

ping adlr / rtc, can either of you take a look at this since this re-writes most
of autoupdate.py?

[adlr, 2010-09-15 18:17:40]

http://codereview.chromium.org/3296022/diff/22001/23001
File autoupdate.py (right):

http://codereview.chromium.org/3296022/diff/22001/23001#newcode113
autoupdate.py:113: if not os.path.exists(image_path) and
os.path.exists(cached_file_path):
should you maybe throw an exception if either doesn't exist?

http://codereview.chromium.org/3296022/diff/22001/23001#newcode174
autoupdate.py:174: def GenerateUpdateGz(self, image_path):
It might be nice to avoid using "Gz" here, as the delta update file format is
not a valid gzip archive. I think the extension we'll be moving to is CRAU
(ChRome os Auto Update). For now, the gz extension is appropriate, tho.

http://codereview.chromium.org/3296022/diff/22001/23001#newcode315
autoupdate.py:315: """Imports a factory - floor server configuration file. The
file should
imho, the - was better w/o the spaces around it. You might also change it to
just "Imports a factory server configuration file. ..."

http://codereview.chromium.org/3296022/diff/22001/23001#newcode400
autoupdate.py:400: url = 'http: //% s / static /% s' % (hostname, filename)
why all the spaces in this string?

http://codereview.chromium.org/3296022/diff/22001/23002
File autoupdate_unittest.py (right):

http://codereview.chromium.org/3296022/diff/22001/23002#newcode62
autoupdate_unittest.py:62: '%s/%s/%s/chromiumos_image.bin' % (self.build_root,
self.test_board,
will this work if the user running the test doesn't have any images built?

[sosa, 2010-09-15 18:28:58]

Thanks, PTAL

http://codereview.chromium.org/3296022/diff/22001/23001
File autoupdate.py (right):

http://codereview.chromium.org/3296022/diff/22001/23001#newcode113
autoupdate.py:113: if not os.path.exists(image_path) and
os.path.exists(cached_file_path):
I'm ok with either way.  The use case in defense of this is that I've seen some
users of the devserver create their own update file and put it in dev/static. 
If they'd never built an image before then this would always throw an exception
even though theoretically they do have an image to serve.

On 2010/09/15 18:17:41, adlr wrote:
> should you maybe throw an exception if either doesn't exist?

http://codereview.chromium.org/3296022/diff/22001/23001#newcode174
autoupdate.py:174: def GenerateUpdateGz(self, image_path):
Agreed. Done

On 2010/09/15 18:17:41, adlr wrote:
> It might be nice to avoid using "Gz" here, as the delta update file format is
> not a valid gzip archive. I think the extension we'll be moving to is CRAU
> (ChRome os Auto Update). For now, the gz extension is appropriate, tho.

http://codereview.chromium.org/3296022/diff/22001/23001#newcode315
autoupdate.py:315: """Imports a factory - floor server configuration file. The
file should
Sorry my IDE auto-formatted.  Changing my defaults.  Good catch.

On 2010/09/15 18:17:41, adlr wrote:
> imho, the - was better w/o the spaces around it. You might also change it to
> just "Imports a factory server configuration file. ..."

http://codereview.chromium.org/3296022/diff/22001/23001#newcode400
autoupdate.py:400: url = 'http: //% s / static /% s' % (hostname, filename)
On 2010/09/15 18:17:41, adlr wrote:
> why all the spaces in this string?

Good catch, IDE again.

[adlr, 2010-09-15 18:59:27]

LGTM

On Wed, Sep 15, 2010 at 11:28 AM, <sosa@chromium.org> wrote:

> Thanks, PTAL
>
>
>
> http://codereview.chromium.org/3296022/diff/22001/23001
> File autoupdate.py (right):
>
> http://codereview.chromium.org/3296022/diff/22001/23001#newcode113
> autoupdate.py:113: if not os.path.exists(image_path) and
> os.path.exists(cached_file_path):
> I'm ok with either way.  The use case in defense of this is that I've
> seen some users of the devserver create their own update file and put it
> in dev/static.  If they'd never built an image before then this would
> always throw an exception even though theoretically they do have an
> image to serve.


I'm fine if you keep your behavior.


>
>
> On 2010/09/15 18:17:41, adlr wrote:
>
>> should you maybe throw an exception if either doesn't exist?
>>
>
> http://codereview.chromium.org/3296022/diff/22001/23001#newcode174
> autoupdate.py:174: def GenerateUpdateGz(self, image_path):
> Agreed. Done
>
>
> On 2010/09/15 18:17:41, adlr wrote:
>
>> It might be nice to avoid using "Gz" here, as the delta update file
>>
> format is
>
>> not a valid gzip archive. I think the extension we'll be moving to is
>>
> CRAU
>
>> (ChRome os Auto Update). For now, the gz extension is appropriate,
>>
> tho.
>
> http://codereview.chromium.org/3296022/diff/22001/23001#newcode315
> autoupdate.py:315: """Imports a factory - floor server configuration
> file. The file should
> Sorry my IDE auto-formatted.  Changing my defaults.  Good catch.
>
>
> On 2010/09/15 18:17:41, adlr wrote:
>
>> imho, the - was better w/o the spaces around it. You might also change
>>
> it to
>
>> just "Imports a factory server configuration file. ..."
>>
>
> http://codereview.chromium.org/3296022/diff/22001/23001#newcode400
> autoupdate.py:400: url = 'http: //% s / static /% s' % (hostname,
> filename)
> On 2010/09/15 18:17:41, adlr wrote:
>
>> why all the spaces in this string?
>>
>
> Good catch, IDE again.
>
>
> http://codereview.chromium.org/3296022/show
>

[sosa, 2010-09-15 23:29:14]

Hey Adlr, based on our discussion, I have updated this CL (see diffs 11 & 12 for
autoupdate.py).  Can you PTAL

[adlr, 2010-09-15 23:34:43]

LGTM

[Nick Sanders, 2010-09-16 11:31:21]

Please revert this change ASAP, it has broken factory install. 

Please do include me on any changed to devserver in the future, and test against
factory install usage.

[Tammo Spalink, 2010-09-16 11:33:44]

Breakage detals:

(cros-chroot) tammo@tammo ~/trunk/src/platform/dev $ python2.6
devserver.py --factory_config miniomaha.conf --validate_factory_config
'dev root is /home/tammo/trunk/src'
'Serving from /home/tammo/trunk/src/platform/dev/static'
Traceback (most recent call last):
  File "devserver.py", line 130, in <module>
    updater.ImportFactoryConfigFile(factory_config_path,
NameError: name 'factory_config_path' is not defined

Tammo

On Thu, Sep 16, 2010 at 19:31,  <nsanders@chromium.org> wrote:
> Please revert this change ASAP, it has broken factory install.
>
> Please do include me on any changed to devserver in the future, and test
> against
> factory install usage.
>
> http://codereview.chromium.org/3296022/show
>

[Tammo Spalink, 2010-09-16 12:05:27]

lgtm

Tammo

On Thu, Sep 16, 2010 at 19:33, Tammo Spalink <tammo@chromium.org> wrote:
> Breakage detals:
>
> (cros-chroot) tammo@tammo ~/trunk/src/platform/dev $ python2.6
> devserver.py --factory_config miniomaha.conf --validate_factory_config
> 'dev root is /home/tammo/trunk/src'
> 'Serving from /home/tammo/trunk/src/platform/dev/static'
> Traceback (most recent call last):
>  File "devserver.py", line 130, in <module>
>    updater.ImportFactoryConfigFile(factory_config_path,
> NameError: name 'factory_config_path' is not defined
>
> Tammo
>
> On Thu, Sep 16, 2010 at 19:31,  <nsanders@chromium.org> wrote:
>> Please revert this change ASAP, it has broken factory install.
>>
>> Please do include me on any changed to devserver in the future, and test
>> against
>> factory install usage.
>>
>> http://codereview.chromium.org/3296022/show
>>
>

[sosa, 2010-09-16 16:33:08]

Is there documentation describing the factory install usage?  Do you have tests?

On Thu, Sep 16, 2010 at 4:31 AM,  <nsanders@chromium.org> wrote:
> Please revert this change ASAP, it has broken factory install.
>
> Please do include me on any changed to devserver in the future, and test
> against
> factory install usage.
>
> http://codereview.chromium.org/3296022/show
>