Change subject: Enable deploy_chrome.py to deploy from a build output directory.
......................................................................

Patch Set 10: (13 inline comments)

....................................................
File lib/chrome_util.py
Line 113:   def _SrcIsGlobPattern(self):
This is unused now isn't it? With that, GLOB_PATTERNS would be unused too...

Note that if you want to check whether a path has magic characters in it you can use glob.has_magic

Line 130:     if os.path.isdir(src):
This can be simplified:

  src_is_dir = os.path.isdir(src)
  dir_suffix = ' [d]' if src_is_dir else ''
  logging.debug('%s%s -> %s', src, dir_suffix, dest)
  if src_is_dir and os.path.isdir(dest):
    dest = os.path.join(dest, os.path.basename(src))
  shutil.copytree(src, dest)

Line 145:         logging.warning('%s does not exist.  Skipping.', src)
Why is this a warning if the file is truly optional? This is more informational, is it not?

Line 149:
if len(paths) > 1 and not os.path.isdir(dest), we should probably raise an error, no?

Line 211:   def SetPermissions():
Any reason why this is a nested function? Nested functions are hard to test because you can't reference them from tests...

A private function would be more appropriate, I think.

....................................................
File lib/commandline.py
Line 48:   parsed = urlparse.urlparse(value)
Should you be checking parsed.scheme?

Line 52:   if parsed.hostname == 'sandbox.google.com':
This should be a constant and should be shared with all the other places it is used.

Line 56:     assert storage == 'storage', 'GS URL %s not in expected format.' % value
'storage' should also be a constant and shared.

....................................................
File scripts/deploy_chrome.py
Line 246:                           'When this option is used, The GYP_DEFINES '
s/used, The/used, the/

Line 277:                    help=optparse.SUPPRESS_HELP)
indentation is off here.

Line 326:   gsutil_tar = os.path.join(tempdir, 'gsutil.tar.gz')
Why do we fetch gsutil every time?

Line 346:     files.sort()
From earlier comment: Why don't we just search for the one that doesn't contain 'unstripped' in it? Simpler and not depending on alphabetical sorting...

The case you mentioned where there are multiple versions of Chrome uploaded for a given buildbot run sounds like an error case (and we should fail on that case, right?)

sorting and picking the oldest version of Chrome that was uploaded doesn't sound correct anyway, and sort() won't sort version numbers correctly anyhow

Line 347:     cros_build_lib.logger.warning('Multiple chrome packages found.  Using %s',
Any reason why we're using cros_build_lib.logger.warning intead of cros_build_lib.Warning ?

--
To view, visit https://gerrit.chromium.org/gerrit/36020
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I25bc3f5afc0fd80fd5e664c75cbd3524b9716831
Gerrit-PatchSet: 10
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Ryan Cui <r...@chromium.org>
Gerrit-Reviewer: Chris Sosa <s...@chromium.org>
Gerrit-Reviewer: David James <davidja...@chromium.org>
Gerrit-Reviewer: Gerrit <chrome-...@google.com>
Gerrit-Reviewer: Mike Frysinger <vap...@chromium.org>
Gerrit-Reviewer: Peter Mayo <peterm...@chromium.org>
Gerrit-Reviewer: Ryan Cui <r...@chromium.org>