Add script for syncing Chrome to a specified directory. [chromiumos/chromite : master]

[David James (Code Review)]

David James has uploaded a new change for review.

Change subject: Add script for syncing Chrome to a specified directory.
......................................................................

Add script for syncing Chrome to a specified directory.

BUG=chromium-os:36324
TEST=Run it.

Change-Id: I8a1e8c2c9191495cd641c62d43938195ed621e1f
---
A bin/sync_chrome
A lib/gclient.py
M scripts/cros_mark_chrome_as_stable.py
A scripts/sync_chrome.py
4 files changed, 176 insertions(+), 7 deletions(-)


git pull ssh://gerrit.chromium.org:29418/chromiumos/chromite refs/changes/23/37923/1
--
To view, visit https://gerrit.chromium.org/gerrit/37923
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I8a1e8c2c9191495cd641c62d43938195ed621e1f
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: David James <david...@chromium.org>

[Peter Mayo (Code Review)]

Peter Mayo has posted comments on this change.

Change subject: Add script for syncing Chrome to a specified directory.
......................................................................

Patch Set 1: Looks good to me, approved

(4 inline comments)

a nit, 2 suggestions and a lurking bug.
Please test an old version of a chrome internal.

....................................................
File lib/gclient.py
Line 53: auxiliary_url = '%s/trunk/src-internal%s' % (internal_url, rev_str)
Auxilliary URL shouldn't have the same @rev as main, in general.

Line 57: primary_url = '%s/trunk/tools/buildspec/releases/%s' % (internal_url, rev)
Suggest: Add a TODO to handle the archived case.

Line 88: # out the right revision to pull from the internal DEPS.
nit: "a" right revision - despite assertions to the contrary it is not a well-defined function.

....................................................
File scripts/sync_chrome.py
Line 19: type=int, dest='version')
Consider blank lines around version & its use to make it quicker to read the group.

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

Gerrit-MessageType: comment

Gerrit-Change-Id: I8a1e8c2c9191495cd641c62d43938195ed621e1f
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: David James <david...@chromium.org>

Gerrit-Reviewer: Gerrit <chrom...@google.com>
Gerrit-Reviewer: Peter Mayo <pete...@chromium.org>
Gerrit-Reviewer: Ryan Cui <rc...@chromium.org>

[Anonymous Coward (Code Review)]

so...@google.com has posted comments on this change.

Change subject: Add script for syncing Chrome to a specified directory.
......................................................................

Patch Set 1:

Am out sick, but can you put together a quick plan about your implementation for this -- this seems like a lot of work to pull out the Chrome sync components of the ebuild -- which you should and it would be nice to see the whole plan before the little bits go in.

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

Gerrit-MessageType: comment

Gerrit-Change-Id: I8a1e8c2c9191495cd641c62d43938195ed621e1f
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: David James <david...@chromium.org>

Gerrit-Reviewer: Gerrit <chrom...@google.com>
Gerrit-Reviewer: Peter Mayo <pete...@chromium.org>
Gerrit-Reviewer: Ryan Cui <rc...@chromium.org>

Gerrit-Reviewer: so...@google.com

[Peter Mayo (Code Review)]

Peter Mayo has posted comments on this change.

Change subject: Add script for syncing Chrome to a specified directory.
......................................................................

Patch Set 1: (1 inline comment)

I see so many times where using the logic, or similar logic, from inside the ebuild would be inappropriate that I don't see a problem with the duplication. It already exists in different forms in workflows, chrome build scripts, etc.

I could see an argument that this is not specific to chromeos and so should be upstreamed to chrome. It could find a home in tools/build. But that would add complexity to the indirection.

....................................................
File lib/gclient.py
Line 1: # Copyright (c) 2012 The Chromium OS Authors. All rights reserved.
Should this file be called gclient.py or gclient_utils.py?

It doesn't implement gclient, nor do a bunch of other interface libraries, like git, svn, reitveld, to name a few from different contexts.

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

Gerrit-MessageType: comment

Gerrit-Change-Id: I8a1e8c2c9191495cd641c62d43938195ed621e1f
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: David James <david...@chromium.org>

[Chris Sosa (Code Review)]

Chris Sosa has posted comments on this change.

Change subject: Add script for syncing Chrome to a specified directory.
......................................................................

Patch Set 1:

I'm just arguing that they entire flow etc should be documented before we start adding code for it. I'm not actually -1 or -2ing it but just asking that we spend a little time up front before we start checking in code -- if only just to have docs to point to later :)

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

Gerrit-MessageType: comment

Gerrit-Change-Id: I8a1e8c2c9191495cd641c62d43938195ed621e1f
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: David James <david...@chromium.org>

Gerrit-Reviewer: Chris Sosa <so...@chromium.org>

[David James (Code Review)]

David James has posted comments on this change.

Change subject: Add script for syncing Chrome to a specified directory.
......................................................................

Patch Set 1:

Here's the plan for updating cbuildbot to support rietveld patches:

1. Add sync_chrome script for syncing Chrome source code to a specified directory. (This CL.)
2. Update Chrome ebuild to use sync_chrome script for SERVER_SOURCE builds.
3. Add SyncChrome stage to cbuildbot which runs sync_chrome script.
4. Update cbuildbot to use the LOCAL_SOURCE workflow and the source code sync’d by the SyncChrome stage.
5. Update cbuildbot to support patching in Rietveld patches supplied on the command line.

This now lives at http://goto.google.com/cbuildbot-rietveld in case you want a Google Doc to point to :)

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

Gerrit-MessageType: comment

Gerrit-Change-Id: I8a1e8c2c9191495cd641c62d43938195ed621e1f
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: David James <david...@chromium.org>

Gerrit-Reviewer: Chris Sosa <so...@chromium.org>
Gerrit-Reviewer: David James <david...@chromium.org>

[Ryan Cui (Code Review)]

Ryan Cui has posted comments on this change.

Change subject: Add script for syncing Chrome to a specified directory.
......................................................................

Patch Set 1: (4 inline comments)

....................................................
File lib/gclient.py
Line 40: internal: Whether you want an internal checkout.
Instead of copying and pasting the docstrings, can you reference a central source(i.e, WriteConfigFile)?

Line 85: 'custom_deps': custom_deps}]
'custom_var' section missing.

Line 119: cmd = ['gclient', 'config', '--spec', spec]
from the gclient help for --spec - "create a gclient file containing the provided string. Due to Cygwin/Python brokenness, it probably can't contain any newlines." Have you tested in the case of newlines generated by pprint?

Line 136: '--delete_unversioned_trees']
What does --manually_grab_svn_rev do - the --help description is not very clear.

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

Gerrit-MessageType: comment

Gerrit-Change-Id: I8a1e8c2c9191495cd641c62d43938195ed621e1f
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: David James <david...@chromium.org>

[Ryan Cui (Code Review)]

Ryan Cui has posted comments on this change.

Change subject: Add script for syncing Chrome to a specified directory.
......................................................................

Patch Set 1:

as an FYI, the main usecase for developers will be git. Currently the script only supports svn.

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

Gerrit-MessageType: comment

Gerrit-Change-Id: I8a1e8c2c9191495cd641c62d43938195ed621e1f
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: David James <david...@chromium.org>

[David James (Code Review)]

David James has posted comments on this change.

Change subject: Add script for syncing Chrome to a specified directory.
......................................................................

Patch Set 1: (9 inline comments)

....................................................
File lib/gclient.py

Line 1: # Copyright (c) 2012 The Chromium OS Authors. All rights reserved.

Personally, I prefer just gclient.py, since gclient_utils.py is longer and a little redundant. This is just called gclient because it interacts with gclient, similar to 'gs.py' which doesn't reimplement Google Storage or gsutil but interacts with Google Storage.

Line 40: internal: Whether you want an internal checkout.

Done

Line 53: auxiliary_url = '%s/trunk/src-internal%s' % (internal_url, rev_str)

Good catch! Done.

Line 57: primary_url = '%s/trunk/tools/buildspec/releases/%s' % (internal_url, rev)

Done

Line 85: 'custom_deps': custom_deps}]
Good catch. Fixed.

Line 88: # out the right revision to pull from the internal DEPS.

Done. Replaced "the right revision" with "the approximate revision" since it's not an exact science.

I would guess that gclient is consistent about what revision it uses even if the revision it picks might be wrong.

Line 119: cmd = ['gclient', 'config', '--spec', spec]

Yes it works. It might be busted on cygwin but it works great on Linux :)

Line 136: '--delete_unversioned_trees']
Looking at the code, --manually_grab_svn_rev speeds up updates by checking the revision the repo is at prior to running 'svn up'. This skips the whole need for an update if we are already up to date.

Based on that description, I've made it a default option since it doesn't have anything to do with reset.

....................................................
File scripts/sync_chrome.py
Line 19: type=int, dest='version')

Done

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

Gerrit-MessageType: comment

Gerrit-Change-Id: I8a1e8c2c9191495cd641c62d43938195ed621e1f
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: David James <david...@chromium.org>

[Brian Harring (Code Review)]

Brian Harring has posted comments on this change.

Change subject: Add script for syncing Chrome to a specified directory.
......................................................................

Patch Set 2: (12 inline comments)

....................................................
File lib/gclient.py
Line 5: """Common functions used for syncing Chrome."""
suggest sticking a link to gclient documentation in here, since this code has assumptions re: gclient.

Line 15: def _UseGoloMirror():
likely should memoize this.

Line 19: golo.chromium.org.
expand the docstring explaining why external can't access golo (and if they can, why we don't have non golo hardware access it).

I assume those mirrors aren't accessible outside the golo- that said the commit message doesn't make it clear or not.

Line 46: rev_str = '@%s' % rev if rev else ''
rev or ''

Line 58: """Get the solutions array to write to the gclient file.
a docstring of the form for "GetFozzilBom" that is "Get the FozillBom" isn't much fun for people who don't already know what it is.

Clarify the solutions array (aka, the fact it's a project definition).

Line 94: return 'solutions = %s\n' % pprint.pformat(solutions)
things would be a helluva lot simpler if gclient files were json rather than quasi...

Line 97: def WriteConfigFile(gclient, cwd, internal, use_pdf, rev):
suspect this should be an object w/ an appropriate write; else this code is purely usable only for chrome (meaning lib/gclient is a misnomer).

Reason I mention implementing this as a mapping is that I suspect sooner or later we're going to have to deal w/ a gclient config that isn't a single project- or we'll have to pull values out of it; the api's you're laying down here won't be particularly conducive to that.

Line 123: cros_build_lib.RunCommand([gclient, 'revert', '--nohooks'], cwd=cwd)
a comment clarifying why this is ran in addition to the reset logic below would be wise.

Line 129: cros_build_lib.RunCommand(cmd, cwd=cwd)
RunCommandCapture strikes me as warranted, or at least the ability to keep gclient quiet.

Also, stdin should be suppressed unless there is a good reason to allow it to go interactive if it deigns to.

....................................................
File scripts/sync_chrome.py
Line 13: """Creates the argparse parser."""
no reason to store this w/in a function; you can put it in global scope; I mention this since our wrapper design will eventually migrate towards such a thing.

Line 23: action='store_true', default=False)
in the absence of a given default, it should be None; reason you care about False vs None?

Line 41: gclient.Sync(options.gclient, options.chrome_root, options.reset)
have suspect if this fails, we want to pass that error code back out to the term- this doesn't. Counter argument?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8a1e8c2c9191495cd641c62d43938195ed621e1f
Gerrit-PatchSet: 2

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: David James <david...@chromium.org>

Gerrit-Reviewer: Brian Harring <ferr...@chromium.org>

[Chris Sosa (Code Review)]

Chris Sosa has posted comments on this change.

Change subject: Add script for syncing Chrome to a specified directory.
......................................................................

Patch Set 2: (3 inline comments)

....................................................
File lib/gclient.py
Line 5: """Common functions used for syncing Chrome."""

To play devil's advocate, gclient is in depot_tools and which is already a requirement for the build system. Not sure how much value this would add.

Line 68: 'svn_url': SVN_MIRROR_URL,
There's a bunch of layout tests that are None'd in the Chrome ebuild's gclient spec file -- these seem to be missing from here.

Line 71: 'sourceforge_url': SVN_MIRROR_URL + '/%(repo)s'
What updates %(repo)?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8a1e8c2c9191495cd641c62d43938195ed621e1f
Gerrit-PatchSet: 2

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: David James <david...@chromium.org>

[David James (Code Review)]

David James has posted comments on this change.

Change subject: Add script for syncing Chrome to a specified directory.
......................................................................

Patch Set 2: (14 inline comments)

....................................................
File lib/gclient.py
Line 5: """Common functions used for syncing Chrome."""

I've added a link to the documentation to the WriteConfigFile function, which is the function all the docstrings point at.

Line 15: def _UseGoloMirror():
Why? This function takes 0.026ms to run. We could optimize it to run faster, but this seems like unnecessary complexity given how fast it runs already.

Line 19: golo.chromium.org.
Done

Line 46: rev_str = '@%s' % rev if rev else ''

Does not work.

>>> rev = None
>>> '@%s' % rev or ''
'@None'

>>> '@%s' % rev if rev else ''

''

Line 58: """Get the solutions array to write to the gclient file.

I don't think this is really necessary given that gclient has its own documentation describing how the gclient file format works. I've updated the WriteConfigFile function to link to the gclient documentation.

Line 68: 'svn_url': SVN_MIRROR_URL,
Yeah, that's intentional -- those tests are fairly small (with the exception of src/third_party/WebKit/LayoutTests which is 5.1G) and the list of DEPS to exclude is prone to getting out of date (it's already out of date). So just excluding the 1 big thing is more maintainable. Long term we want to move that logic into the DEPS file itself.

Line 71: 'sourceforge_url': SVN_MIRROR_URL + '/%(repo)s'

This is used in the DEPS file itself -- they substitute in the repo.

Line 94: return 'solutions = %s\n' % pprint.pformat(solutions)

Solution files are documented as being Python format, not JSON

Line 97: def WriteConfigFile(gclient, cwd, internal, use_pdf, rev):

I don't think we're going to be using gclient for anything else -- if anything, Chrome is moving away from gclient so we don't want to start adding gclient APIs for doing other things. The same is true for the few other projects that use gclient.

The main reason I called this gclient.py as opposed to chrome.py, is that I don't want this file growing to contain everything related to Chrome. gclient.py gives it a clear scope (i.e. just for gclient-related things.)

Line 123: cros_build_lib.RunCommand([gclient, 'revert', '--nohooks'], cwd=cwd)

Done

Line 129: cros_build_lib.RunCommand(cmd, cwd=cwd)
Some problems with this: 1) Hiding the input or output prevents users from being able to enter in their password if they were using this script. 2) gclient is prone to take a long time, so hiding the output means that buildbot may timeout while waiting for output from gclient. And if buildbot times out while we were waiting for output, we get no debugging information at all as to what the problem was. So I think that just switching to CaptureOutput here would be a bad idea.

We currently don't limit the output of gclient at all inside the Chrome ebuild, so how about we table this discussion until sync_chrome is added into cbuildbot? At that point we can look at output limiting and/or progress indicators.

....................................................
File scripts/sync_chrome.py
Line 13: """Creates the argparse parser."""

I'm going to disagree here because all of our existing code has GetParser in a function, so it's a convention. If you want to change the convention, I'd suggest we discuss it on chromeos-build@ or submit a big CL rather than just update it in 1 place and not in the rest.

Line 23: action='store_true', default=False)

Nope, but I think it's cleaner to be True or False rather than True or None.

Line 41: gclient.Sync(options.gclient, options.chrome_root, options.reset)

If the sync fails, we throw an exception so failure code is returned. As for the specific error code, I don't think that matters here.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8a1e8c2c9191495cd641c62d43938195ed621e1f
Gerrit-PatchSet: 2

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: David James <david...@chromium.org>

[David James (Code Review)]

David James has posted comments on this change.

Change subject: Add script for syncing Chrome to a specified directory.
......................................................................

Patch Set 3: Verified

Verified with many trybot runs.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8a1e8c2c9191495cd641c62d43938195ed621e1f
Gerrit-PatchSet: 3

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: David James <david...@chromium.org>

[Peter Mayo (Code Review)]

Peter Mayo has posted comments on this change.

Change subject: Add script for syncing Chrome to a specified directory.
......................................................................

Patch Set 3: Looks good to me, approved

(2 inline comments)

My remaining suggestions may be actionable, and may lead to handling a few more unusual situations more gracefully, but are certainly incremental to the validated functionality.

....................................................
File lib/gclient.py
Line 135: cros_build_lib.RunCommand(revert_cmd, cwd=cwd)
The chrome trybots do a revert before they sync to undo local changes first. (in fact they [revert, config, sync] to correctly roll from one DEPS + changes to the next)

....................................................
File scripts/sync_chrome.py
Line 38: osutils.SafeMakedirs(options.chrome_root)
Might want to just split the reset code out and invoke it here.
In particular, if you have just created the dir, there i nothing to reset, whereas if the directory is present and looks at least semi-valid, try revert.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8a1e8c2c9191495cd641c62d43938195ed621e1f
Gerrit-PatchSet: 3

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: David James <david...@chromium.org>

[Chris Sosa (Code Review)]

Chris Sosa has posted comments on this change.

Change subject: Add script for syncing Chrome to a specified directory.
......................................................................

Patch Set 3: Looks good to me, approved

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8a1e8c2c9191495cd641c62d43938195ed621e1f
Gerrit-PatchSet: 3

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: David James <david...@chromium.org>

[Brian Harring (Code Review)]

Brian Harring has posted comments on this change.

Change subject: Add script for syncing Chrome to a specified directory.
......................................................................

Patch Set 2: (5 inline comments)

....................................................
File lib/gclient.py
Line 15: def _UseGoloMirror():
eh, being paranoid- it wasn't for speed, more was related to the fact fqdn isn't guaranteed stable; that said, for golo it always is, thus as said- being paranoid.

Ignore it.

Line 46: rev_str = '@%s' % rev if rev else ''

yeah, overlooked the interpolation.

Line 58: """Get the solutions array to write to the gclient file.

I meant moreso that someone (think of a newbie/intern) looking at this the first time through isn't going to have a clue what a solutions array is.

If you know gclient files, it's obvious- just thinking there has to be a better description for GetClientSolutions then "Get the solutions array to write the gclient file".

Not sure what the 'better' text would be there, but I suspect you now grok what I meant when I said having a docstring for a method GetFozzilBom that is "Get the FozzilBom" is a bit unfriendly. ;)

Line 97: def WriteConfigFile(gclient, cwd, internal, use_pdf, rev):

disagree a bit as to the naming, but I'm not going to push that point particularly hard.

Line 129: cros_build_lib.RunCommand(cmd, cwd=cwd)
#1 is more my concern; the docstring should be updated that this may go interactive if we're not planning on suppressing that behaviour.

For cbuildbot usage (ie, builders), I suspect we want to to thread a --non-interactive into the script that threads down to here, suppressing stdin.

gclient may have a tty check for stdin, and hinge it's interactive behaviour on that- I just don't trust it and am concerned about potential scenarios where code above this, gets hung waiting on user input.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8a1e8c2c9191495cd641c62d43938195ed621e1f
Gerrit-PatchSet: 2

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: David James <david...@chromium.org>

[Brian Harring (Code Review)]

Brian Harring has posted comments on this change.

Change subject: Add script for syncing Chrome to a specified directory.
......................................................................

Patch Set 3: (2 inline comments)

....................................................
File lib/gclient.py
Line 135: cros_build_lib.RunCommand(revert_cmd, cwd=cwd)
who's handling the retries for network flakeyness here? gclient, or is this code supposed to?

This is part of why I mentioned passing the correct exit code on failure for the sync script; and code above this that wraps the run in a retry (if this code doesn't) would need to know the exit code to decipher when to retry or not.

....................................................
File scripts/sync_chrome.py
Line 38: osutils.SafeMakedirs(options.chrome_root)

or thread a hint down that tells it to skip revert; SafeMakeDirs returns a boolean indicating whether or not the directory had to be created.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8a1e8c2c9191495cd641c62d43938195ed621e1f
Gerrit-PatchSet: 3

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: David James <david...@chromium.org>

[David James (Code Review)]

David James has posted comments on this change.

Change subject: Add script for syncing Chrome to a specified directory.
......................................................................

Patch Set 3: (2 inline comments)

....................................................
File lib/gclient.py
Line 135: cros_build_lib.RunCommand(revert_cmd, cwd=cwd)

Done.

gclient is supposed to handle network flakiness, and doesn't return any useful return codes that'd really allow you to differentiate flakiness from a hard failure. So in this case I don't think passing down the return code is that useful.

....................................................
File scripts/sync_chrome.py
Line 38: osutils.SafeMakedirs(options.chrome_root)

Done

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8a1e8c2c9191495cd641c62d43938195ed621e1f
Gerrit-PatchSet: 3

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: David James <david...@chromium.org>

[Peter Mayo (Code Review)]

Peter Mayo has posted comments on this change.

Change subject: Add script for syncing Chrome to a specified directory.
......................................................................

Patch Set 4: Looks good to me, approved

(1 inline comment)

....................................................
File scripts/sync_chrome.py
Line 43: gclient.Revert(options.gclient, options.chrome_root)
The gclient revert can't necessarily revert the correct stuff if it runs after you change the config, right?

It does have the _entries file, but this is slightly different from the chrome workflow, and that makes me worry it will have subtle consequences someday.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8a1e8c2c9191495cd641c62d43938195ed621e1f
Gerrit-PatchSet: 4

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: David James <david...@chromium.org>

[David James (Code Review)]

David James has posted comments on this change.

Change subject: Add script for syncing Chrome to a specified directory.
......................................................................

Patch Set 4: (1 inline comment)

....................................................
File scripts/sync_chrome.py
Line 43: gclient.Revert(options.gclient, options.chrome_root)

Done. I also added logic to recreate the chrome root if the revert fails (e.g. in cases where the checkout is corrupted, such as the .gclient file being missing.)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8a1e8c2c9191495cd641c62d43938195ed621e1f
Gerrit-PatchSet: 4

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: David James <david...@chromium.org>

[David James (Code Review)]

David James has posted comments on this change.

Change subject: Add script for syncing Chrome to a specified directory.
......................................................................

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

All trybot runs are passing. Inheriting LGTM.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8a1e8c2c9191495cd641c62d43938195ed621e1f
Gerrit-PatchSet: 6

Gerrit-Project: chromiumos/chromite
Gerrit-Branch: master
Gerrit-Owner: David James <david...@chromium.org>