Enable deploy_chrome.py to deploy from a build output direct... [chromiumos/chromite : master]

[David James (Code Review)]

David James has posted comments on this change.

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


Patch Set 4: I would prefer that you didn't submit this

(13 inline comments)

....................................................
File lib/chrome_util.py
Line 1: #!/usr/bin/python
pylint errors:

************* Module lib.chrome_util
C0322: 24: Operator not preceded by a space
EXTRA_FLAGS=(_HIGHDPI_FLAG,)
^
W0613: 74:Conditions._ExtraFlagSet: Unused argument 'gyp_defines'
W6501:174:_CopyPaths: Specify string format arguments as logging function parameters
W0611: 20: Unused import sudo
W0611: 14: Unused import urlparse
W0611: 9: Unused import optparse
W0611: 13: Unused import time
W0611: 17: Unused import commandline

Line 53: return _NameValueListToDict(shlex.split(defines))
Can we assert upfront that defines is not None? shlex.split 'hangs' (waits on stdin) if defines is None, which is probably not expected here :)

Line 150: ('libnss3.so', '/usr/lib/libnss3.so.1d'),
Shouldn't we use the libdir? On 64 bit, for example, shouldn't it look in lib64? Noticed the ebuild doesn't do that, but it probably should...

Line 156: ('libnspr4.so', '/usr/lib/libnspr4.so.0d'),
This may be copied from the Chrome ebuild, but it looks very fragile... why can't Chrome just look in /usr/lib for so files? Every time someone updates any of these libraries they're going to update this script, and I doubt they're going to be looking in chromite for them...

Line 168: shutil.copytree(src, os.path.join(dest_base, path.name))
Guessing we'll need permissions on all the files / directories under here...

Line 176: cros_build_lib.Die('%s does not exist. Strict mode enabled.'
I don't see any references to strict mode -- do you mean that it's non-optional ?

Line 182: shutil.copy(f, dest)
Shouldn't we be setting permissions? I think Portage sets executables to 755, and non-executables to 644, but Mike would know for sure.

Line 213: os.chmod(sandbox, 04755)
Since we'll need to set permissions on all installed files, is there any reason the sandbox is special-cased? Might there be a general feature for installing setuid files?

....................................................
File scripts/deploy_chrome.py
Line 330: logging.info('GYP_DEFINES taken from environment: %s'
W6501:330:_PostParseCheck: Specify string format arguments as logging function parameters

Line 339: logging.info('Fetching gsutil.')
Should we do this even if gsutil is installed?

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

Line 361: cros_build_lib.logger.warning('Multiple chrome packages found. Using %s',
What's the reason for using this as opposed to cros_build_lib.Warning

Line 387: pkg_path = _FetchChromePackage(tempdir, options.gs_path)
E0602:387:_PrepareStagingDir: Undefined variable 'tempdir'

--
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: 4
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Ryan Cui <rc...@chromium.org>
Gerrit-Reviewer: Chris Sosa <so...@chromium.org>
Gerrit-Reviewer: David James <david...@chromium.org>
Gerrit-Reviewer: Mike Frysinger <vap...@chromium.org>
Gerrit-Reviewer: Peter Mayo <pete...@chromium.org>
Gerrit-Reviewer: Ryan Cui <rc...@chromium.org>

[Peter Mayo (Code Review)]

Peter Mayo has posted comments on this change.

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

Patch Set 4: (2 inline comments)

....................................................
File lib/chrome_util.py

Line 156: ('libnspr4.so', '/usr/lib/libnspr4.so.0d'),

I would tend to agree that unless we put a guard on this, like a regularly run unit test against ToT chrome, this is going to break.

Line 213: os.chmod(sandbox, 04755)
And if we're going to set setuid, make sure to set the uid.

[Mike Frysinger (Code Review)]

Mike Frysinger has posted comments on this change.

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

Patch Set 4: (5 inline comments)

....................................................
File lib/chrome_util.py
Line 156: ('libnspr4.so', '/usr/lib/libnspr4.so.0d'),

i don't know what this business is all about, but the list can be extracted from chrome-wrapper

at a higher level, this looks entirely messed up ... are we sure this is truly necessary ?

Line 182: shutil.copy(f, dest)
yes, executables (programs and shared libs) should be 755 while data files should be 644

shutil.copy() preserves mode bits, so if the source files are correct, we shouldn't have to mung them ...

Line 213: os.chmod(sandbox, 04755)
portage takes care of squashing uid's to root:root

....................................................
File scripts/deploy_chrome.py

Line 339: logging.info('Fetching gsutil.')

yeah, i don't think we should do this by default

Line 343: DebugRunCommand(['tar', '-xzf', gsutil_tar], cwd=tempdir)
this should use cros_build_lib.FindCompressor(cros_build_lib.COMP_GZIP) and then run tar with the -I flag. there are a bunch of examples in the chromite tree already.

[Chris Sosa (Code Review)]

Chris Sosa has posted comments on this change.

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

Patch Set 4: (1 inline comment)

....................................................
File scripts/deploy_chrome.py
Line 260: parser.add_option('--build-dir', type='path',
It seems like we have way too many options in this interface -- have you looked at ways of dumbing this down for the avg use case?

[Ryan Cui (Code Review)]

Ryan Cui has posted comments on this change.

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

Patch Set 4: (10 inline comments)

....................................................
File lib/chrome_util.py
Line 53: return _NameValueListToDict(shlex.split(defines))
ok

Line 156: ('libnspr4.so', '/usr/lib/libnspr4.so.0d'),

Ran trybots with symlinks removed, and tests passed. Doesn't look like this is needed. Will need to try on a daisy device to be sure.

Line 168: shutil.copytree(src, os.path.join(dest_base, path.name))

I think we can assume we have permissions to files in the build output directory. But once they are staged, the owners need to be set to root:root.

Line 176: cros_build_lib.Die('%s does not exist. Strict mode enabled.'

Will remove that reference.

Line 182: shutil.copy(f, dest)
Yea, that's my rationale - the source files should already be set to the correct mode.

Line 213: os.chmod(sandbox, 04755)
Good point, haven't actually tested deploying to device yet. Will update this function to set uid's to root:root.

....................................................
File scripts/deploy_chrome.py
Line 260: parser.add_option('--build-dir', type='path',

Two common usecases would be --build-dir (built locally) and --gs-path (built using trybots). I can move --local-pkg-path to advanced option section.

I am now thinking to make the advanced options (except --local-pkg-path) hidden...

In the future, --gyp-defines will be determined automatically from the board we are deploying to (unless the user has specified overrides).

The future normal usecase is through 'cros chrome deploy', where we'll have more context - i.e., we will know defaults for --build-dir, board, --gyp-defines, etc.

Line 339: logging.info('Fetching gsutil.')

This ensures we are running an updated version of gsutil regardless of what the user has installed. The correct way would be to cache it. Haven't implemented this yet. Would a TODO: suffice for now?

Line 360: files.sort()
Not sure if it's simpler - in the case of three chrome packages, with two not having -unstripped in it's name, you'd need to choose between the two. What I can do is loop through and use the first one (after sorting) that does not have 'unstripped'.

Line 361: cros_build_lib.logger.warning('Multiple chrome packages found. Using %s',

None...

[Chris Sosa (Code Review)]

Chris Sosa has posted comments on this change.

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

Patch Set 4: (1 inline comment)

....................................................
File scripts/deploy_chrome.py
Line 260: parser.add_option('--build-dir', type='path',

Sounds good!

Also, with ui-v2, we've switched away from --to.

Destinations are left as an arg so an invocation of this would be something like

deploy_chrome my_dut:22 -- ip/host should be the same rather than take separately.

More of an FYI than anything -- perhaps leave a todo here

[Ryan Cui (Code Review)]

Ryan Cui has posted comments on this change.

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

Patch Set 4: (4 inline comments)

feedback addressed. ptal.

....................................................
File scripts/deploy_chrome.py
Line 260: parser.add_option('--build-dir', type='path',

Yup, the interface will definitely need some rework once this gets merged into cros. Until then....Will add the TODO.

Line 330: logging.info('GYP_DEFINES taken from environment: %s'

Done

Line 343: DebugRunCommand(['tar', '-xzf', gsutil_tar], cwd=tempdir)

Will upload a separate CL for this.

Line 387: pkg_path = _FetchChromePackage(tempdir, options.gs_path)

Done

--
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: 4
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: Ryan Cui <rc...@chromium.org>
Gerrit-Reviewer: Chris Sosa <so...@chromium.org>
Gerrit-Reviewer: David James <david...@chromium.org>

Gerrit-Reviewer: Gerrit <chrom...@google.com>

[Ryan Cui (Code Review)]

Ryan Cui has posted comments on this change.

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

Patch Set 6:

Fixed bugs in chrome_util.CopyFile() and made GYP_DEFINES mandatory for --build-dir invocations (either through environment or --gyp-defines).

--
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: 6

[Mike Frysinger (Code Review)]

Mike Frysinger has posted comments on this change.

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

Patch Set 6: I would prefer that you didn't submit this

(16 inline comments)

....................................................
File lib/chrome_util.py
Line 5:
global docstring explaining purpose of this file ?

Line 16: _HIGHDPI_FLAG = 'highdpi'
shouldn't these flags live down below with all the other flags (i.e. _DISABLE_NACL and such) ?

Line 20: # Taken from gyp/pylib.
how about mirroring all of gyp's pylib into chromite/third_party/ rather than cherry-picking pieces ? or if possible, add the repo chromium is using to our manifest ...

Line 73: """Returns condition that tests a gyp flag is set (possibly to a value.)"""
period should be after the )

Line 87: class InvalidPathError(Exception):
seems like this is now unused

Line 98: Can be a glob pattern.
alignment is off by one space

Line 160: """Get the destination location of a source file.
shouldn't this be a method of the Path class ?

Line 190: if _CheckForGlobPatterns(path.src):
why bother with this logic ? a glob on a path that lacks wildcards will work the same and avoid having two divergent code paths.

alternatively, if you really want to keep this logic, you don't need to parse the globs. just do a len(files) > 1.

Line 208: STAGING_FLAGS.
alignment is slightly off here

Line 211: # Set the ownership of all files in staging dir to root:root
period at the end of the sentence

Line 213: [ 'chown', 'root:root', '-R', staging_dir])
indentation is incorrect here -- needs 4 spaces

also, delete the space right after the [

Line 215: # Setuid and rename the sandbox
period at the end of the sentence

Line 221: os.makedirs(os.path.join(dest_base, 'plugins'))
you can combine these two makedirs into a single osutils.SafeMakeDirs

....................................................
File lib/commandline.py
Line 43: """Validate and convert paths pointing to GoogleStorage."""
probably best to rename to NormalizeGSPath() ?

Line 52: if parsed.hostname.startswith('sandbox.google.com'):
parsed.hostname == 'sandbox.google.com' ?

i can't see any hostname that would have something *after* that ... sandbox.google.com.booga.foo.com would be weird

Line 54: # first component.
if the idea is to support copy & pasting from the address bar, it might be useful to also parse the query string if the URL portion is just /bucket/.

[Ryan Cui (Code Review)]

Ryan Cui has posted comments on this change.

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

Patch Set 6: (1 inline comment)

....................................................
File lib/chrome_util.py
Line 190: if _CheckForGlobPatterns(path.src):
The extra path is because for a glob pattern, we treat the user-specified 'dest' as a directory (since presumably there will be many results), whereas for a non-glob we treat dest as the actual path to rename the source to.

Using len(files)>1 doesn't work, because for a glob pattern that matches only one file, we still want 'dest' to be treated as a directory. I actually used len(files)>1 in the previous patchset but realized the error.

[Ryan Cui (Code Review)]

Ryan Cui has posted comments on this change.

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


Patch Set 6: (1 inline comment)

....................................................
File lib/chrome_util.py

Line 20: # Taken from gyp/pylib.

There is no separate repo for gyp's pylib - just the 'external/gyp.git' repository. I don't think we should be pulling the entire repo down just for this one function, which is not even gyp-specific (can be applied to any name value list...)

[Ryan Cui (Code Review)]

Ryan Cui has posted comments on this change.

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

Patch Set 6: (12 inline comments)

Comments addressed. Updated the Path Copy() logic. Ptal.

....................................................
File lib/chrome_util.py
Line 5:

Done

Line 16: _HIGHDPI_FLAG = 'highdpi'

Done

Line 73: """Returns condition that tests a gyp flag is set (possibly to a value.)"""

Done

Line 87: class InvalidPathError(Exception):
Done

Line 98: Can be a glob pattern.

Done

Line 208: STAGING_FLAGS.
Done

Line 211: # Set the ownership of all files in staging dir to root:root

Done

Line 213: [ 'chown', 'root:root', '-R', staging_dir])

Done

Line 215: # Setuid and rename the sandbox

Done

Line 221: os.makedirs(os.path.join(dest_base, 'plugins'))

Done

....................................................
File lib/commandline.py
Line 43: """Validate and convert paths pointing to GoogleStorage."""

Done

Line 52: if parsed.hostname.startswith('sandbox.google.com'):

Done

[Mike Frysinger (Code Review)]

Mike Frysinger has posted comments on this change.

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

Patch Set 6: (1 inline comment)

....................................................
File lib/chrome_util.py

Line 190: if _CheckForGlobPatterns(path.src):
i'm not sure entangling logic and globs like that is a good idea or even necessary

if you make dest less gooey (require a trailing slash), i think you can merge the logic into a single loop.

for path in paths:
files = glob.glob(os.path.join(src_base, path.src))
if not files:
... current warning/error ...

for file in files:
dest = os.path.join(
dest_base,
os.path.relpath(file, src_base) if path.dest is None else path.dest)
osutils.SafeMkdirs(os.path.dirname(dest))
if os.path.isdir(file):
shutil.copytree(file, dest)
else:
shutil.copy(file, dest)

if you do shutil.copytree('/some/dir', 'foo/'), then python knows to put it inside of foo/ rather than "foo" itself.

[Mike Frysinger (Code Review)]

Mike Frysinger has posted comments on this change.

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


Patch Set 6: (1 inline comment)

....................................................
File lib/chrome_util.py

Line 20: # Taken from gyp/pylib.

the gyp code comes from http://code.google.com/p/gyp/source/browse/

we can probably live with copying just one function, but if we want to do more, we should look at mirroring the repo from chromium or upstream

[Ryan Cui (Code Review)]

Ryan Cui has posted comments on this change.

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


Patch Set 6: (1 inline comment)

....................................................
File lib/chrome_util.py

Line 190: if _CheckForGlobPatterns(path.src):
As per our conversation, I'd still prefer checking to see if the src path is a glob pattern. It's not hard to implement, and avoids an error case where the trailing slash is not specified.

Ultimately, checking for glob pattern vs. looking at trailing slash of dest is to cover the case where a glob pattern is specified but matches only one file/directory, in which case we don't know whether to treat the custom 'dest' as the containing directory or the final target path.

It's funny that we are going back and forth on this, because there's actually no usage of the functionality (globbing to a containing dir) yet...

Please take a look at my latest patchset, where the refactored logic looks more like what you have here (minus the check for trailing slash).

[Ryan Cui (Code Review)]

Ryan Cui has posted comments on this change.

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


Patch Set 6: (1 inline comment)

....................................................
File lib/chrome_util.py
Line 190: if _CheckForGlobPatterns(path.src):

OK, talked to David about this. He's also against checking for glob patterns. So seeing how it's 2vs1, I'll go with vapier's solution of trailing slash.

[David James (Code Review)]

David James has posted comments on this change.

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

[David James (Code Review)]

David James has posted comments on this change.

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

Patch Set 11: I would prefer that you didn't submit this

(Looks like comments haven't been addressed yet, so marking -1 for tracking)

--
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: 11

[Ryan Cui (Code Review)]

Ryan Cui has posted comments on this change.

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

Patch Set 10: (10 inline comments)

....................................................
File lib/chrome_util.py
Line 113: def _SrcIsGlobPattern(self):

Done

Line 130: if os.path.isdir(src):
Done

Line 145: logging.warning('%s does not exist. Skipping.', src)

Done

Line 149:
Done

Line 211: def SetPermissions():
Done

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

Done

Line 277: help=optparse.SUPPRESS_HELP)
Done

Line 326: gsutil_tar = os.path.join(tempdir, 'gsutil.tar.gz')

From an earlier patchset: This ensures we are running an updated version of gsutil regardless of what the user has installed. The correct way would be to cache it. Haven't implemented this yet. Would a TODO: suffice for now?

Line 346: files.sort()
Done

Line 347: cros_build_lib.logger.warning('Multiple chrome packages found. Using %s',

Done

--
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

[David James (Code Review)]

David James has posted comments on this change.

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

Patch Set 12: Fails; I would prefer that you didn't submit this

Please run pylint, it's got some errors for you. I'll review once you've fixed those :)

--
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: 12

[Ryan Cui (Code Review)]

Ryan Cui has posted comments on this change.

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

Patch Set 13:

Passes pylint, and tests pass now too...

--
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: 13

[David James (Code Review)]

David James has posted comments on this change.

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

Patch Set 13: Looks good to me, approved

(3 inline comments)

....................................................
File lib/chrome_util.py
Line 140: shutil.copy(src, dest)
Why can't we use shutil.copytree regardless, even if it's not a dir? That'd cut out 2 lines of code.

....................................................
File lib/cros_test_lib.py
Line 31: if isinstance(obj, tuple):
I'd think the check would be for basestring, not tuple, since that way your function would work with lists as well.

i.e.

if isinstance(obj, basestring):
flattened.append(os.path.join(base_path, obj))
else:
...

Line 52: - ['file1', ('directory', ['deepfile1', 'deepfile2']), 'file2']
Looks like you're switching between tuples and lists here... why?

[Ryan Cui (Code Review)]

Ryan Cui has posted comments on this change.

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

Patch Set 13: (3 inline comments)

....................................................
File lib/chrome_util.py
Line 140: shutil.copy(src, dest)

copytree doesn't copy files...

....................................................
File lib/cros_test_lib.py
Line 31: if isinstance(obj, tuple):

In new patchset, the tuple becomes a Directory namedtuple.

Line 52: - ['file1', ('directory', ['deepfile1', 'deepfile2']), 'file2']

The tuple becomes a Directory namedtuple in the new patchset.

[Ryan Cui (Code Review)]

Ryan Cui has posted comments on this change.

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

Patch Set 15: Verified; Looks good to me, approved; Ready

Inheriting +2.

--
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: 15