Add support for prebuilt uploading using rsync.

Add support for prebuilt uploading using rsync.

BUG=not yet
TEST=Try uploading prebuilts to codf30.jail. Also run unit tests.

Change-Id: Ib218e705633febeddbe5c104f67fa580e1fc7de4

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

[scottz, 2010-10-25 21:55:32]

http://codereview.chromium.org/4102001/diff/1/2
File prebuilt.py (right):

http://codereview.chromium.org/4102001/diff/1/2#newcode279
prebuilt.py:279: def _RetryRun(cmd, print_cmd=True, shell=False):
Thanks for breaking this out. Please put it above any functions that use it.
I.e. above _GsUpload

Also add a docstring please.

http://codereview.chromium.org/4102001/diff/1/2#newcode371
prebuilt.py:371: def UploadPrebuilt(build_path, upload_location, version,
board=None, git_sync=False):
over 80 characters

http://codereview.chromium.org/4102001/diff/1/2#newcode410
prebuilt.py:410: else:
How does this portion compensate for the host path? Above I set gs_path to
include _GS_HOST_PATH if the chroot files are being uploaded, something similar
needs to be done similar. Ideally above we would create a general variable that
can be used everywhere. 

relative_path = os.path.join(_GS_HOST_PATH, version)
or for board
relative_path = os.path.join(_GS_BOARD_PATH % % {'board': board, 'version':
version}

HOST_PATH and BOARD_PATH need to be made general (they pretty much are now. 

_REL_HOST_PATH
_REL_BOARD_PATH 

They should always be relative regardless of if it is gs or rsync. So with that
we should be able to use upload_location everywhere and just use your logic here
to call rsync or the gsutil method.

If it doesn't make sense lets just talk about it in person quickly.

http://codereview.chromium.org/4102001/diff/1/2#newcode411
prebuilt.py:411: hostname, path_prefix = upload_location.split(':', 2)
I am not sure I like hostname as the variable since we wil most likely be using
chrome-bot@hostname, ssh_server? something along those lines.

http://codereview.chromium.org/4102001/diff/1/2#newcode462
prebuilt.py:462: if not options.upload.startswith("gs://"):
Can you add a TODO here to expand filters to work across both mechanisms please

[davidjames, 2010-10-25 23:06:45]

PTAL

http://codereview.chromium.org/4102001/diff/1/2
File prebuilt.py (right):

http://codereview.chromium.org/4102001/diff/1/2#newcode279
prebuilt.py:279: def _RetryRun(cmd, print_cmd=True, shell=False):
On 2010/10/25 21:55:32, scottz wrote:
> Thanks for breaking this out. Please put it above any functions that use it.
> I.e. above _GsUpload
> 
> Also add a docstring please. 

Done.

http://codereview.chromium.org/4102001/diff/1/2#newcode371
prebuilt.py:371: def UploadPrebuilt(build_path, upload_location, version,
board=None, git_sync=False):
On 2010/10/25 21:55:32, scottz wrote:
> over 80 characters

Done.

http://codereview.chromium.org/4102001/diff/1/2#newcode410
prebuilt.py:410: else:
On 2010/10/25 21:55:32, scottz wrote:
> How does this portion compensate for the host path? Above I set gs_path to
> include _GS_HOST_PATH if the chroot files are being uploaded, something
similar
> needs to be done similar. Ideally above we would create a general variable
that
> can be used everywhere. 
> 
> relative_path = os.path.join(_GS_HOST_PATH, version)
> or for board
> relative_path = os.path.join(_GS_BOARD_PATH % % {'board': board, 'version':
> version}
> 
> HOST_PATH and BOARD_PATH need to be made general (they pretty much are now. 
> 
> _REL_HOST_PATH
> _REL_BOARD_PATH 
> 
> They should always be relative regardless of if it is gs or rsync. So with
that
> we should be able to use upload_location everywhere and just use your logic
here
> to call rsync or the gsutil method.
> 
> If it doesn't make sense lets just talk about it in person quickly.

It already accounts for this but with your suggestions it's a little more clear.
So I followed your suggestions.

http://codereview.chromium.org/4102001/diff/1/2#newcode411
prebuilt.py:411: hostname, path_prefix = upload_location.split(':', 2)
On 2010/10/25 21:55:32, scottz wrote:
> I am not sure I like hostname as the variable since we wil most likely be
using
> chrome-bot@hostname, ssh_server? something along those lines. 

Done.

http://codereview.chromium.org/4102001/diff/1/2#newcode462
prebuilt.py:462: if not options.upload.startswith("gs://"):
On 2010/10/25 21:55:32, scottz wrote:
> Can you add a TODO here to expand filters to work across both mechanisms
please

Done.

[scottz, 2010-10-25 23:17:59]

Just a few more nits, also please make sure to run the prebuilt_unittest before
submission.

http://codereview.chromium.org/4102001/diff/16001/17001
File prebuilt.py (right):

http://codereview.chromium.org/4102001/diff/16001/17001#newcode406
prebuilt.py:406: remote_location = os.path.join(upload_location, url_suffix)
Since this is now common between the two if pathways lets move it out of the if
statement just above git_file.

http://codereview.chromium.org/4102001/diff/16001/17001#newcode419
prebuilt.py:419: for cmd in ['ssh %s mkdir -p %s' % (ssh_server, remote_path),
for clarity can you create cmds = [ ]  and then just do for cmd in cmds:

http://codereview.chromium.org/4102001/diff/16001/17001#newcode438
prebuilt.py:438: parser.add_option('', '--binhost-base-url',
dest='binhost_base_url',
did you mean to leave this blank? If so you can just omit it entirely. It would
be nice to have a short hand though since this is so long.

[David James, 2010-10-25 23:36:35]

PTAL

http://codereview.chromium.org/4102001/diff/16001/17001
File prebuilt.py (right):

http://codereview.chromium.org/4102001/diff/16001/17001#newcode406
prebuilt.py:406: remote_location = os.path.join(upload_location, url_suffix)
On 2010/10/25 23:18:00, scottz wrote:
> Since this is now common between the two if pathways lets move it out of the
if
> statement just above git_file. 

Done.

http://codereview.chromium.org/4102001/diff/16001/17001#newcode419
prebuilt.py:419: for cmd in ['ssh %s mkdir -p %s' % (ssh_server, remote_path),
On 2010/10/25 23:18:00, scottz wrote:
> for clarity can you create cmds = [ ]  and then just do for cmd in cmds: 
> 

Done.

http://codereview.chromium.org/4102001/diff/16001/17001#newcode438
prebuilt.py:438: parser.add_option('', '--binhost-base-url',
dest='binhost_base_url',
On 2010/10/25 23:18:00, scottz wrote:
> did you mean to leave this blank? If so you can just omit it entirely. It
would
> be nice to have a short hand though since this is so long. 

Done.

[scottz, 2010-10-25 23:39:44]

LGTM