Convenience tool for rolling git-style deps with an svn revision.

Convenience tool for rolling git-style deps with an svn revision.

Usage: roll-dep third_party/WebKit 12345

That will update DEPS with the new revision, and leave the DEPS file
dirty.  It's the up to the user to 'git add; git commit;
git cl upload'.

This script won't handle arbitrary python syntax, but it will handle
variable references like Var("webkit_revision") correctly.

R=mmoss@chromium.org, iannucci@chromium.org
BUG=341098
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275875

[Michael Moss, 2014-06-09 16:01:34]

lgtm

https://codereview.chromium.org/318153003/diff/40001/roll_dep.py
File roll_dep.py (right):

https://codereview.chromium.org/318153003/diff/40001/roll_dep.py#newcode71
roll_dep.py:71: log_p = Popen(['git', 'log', 'origin/master'],
What about branch revisions? Are we not worried about that? Could it fallback to
scanning all branches if a match isn't found?

https://codereview.chromium.org/318153003/diff/40001/roll_dep.py#newcode97
roll_dep.py:97: '"git fetch origin" to get the latest commits.' % svn_rev)
Any reason not to make this automatic?

https://codereview.chromium.org/318153003/diff/40001/roll_dep.py#newcode212
roll_dep.py:212: for os_node in deps_os_node.values:
Not sure it makes sense to always update os deps, since they could intentionally
be different than the main deps, but I guess that's easy enough for someone to
undo before they commit. If you wanted to be fancy, maybe you could detect if
they started out at different revisions and then prompt the user if they want to
update them all to the same revision, but not a big deal.

https://codereview.chromium.org/318153003/diff/40001/roll_dep.py#newcode232
roll_dep.py:232: if len(argv) !=2 :
Nit: space after !=

[iannucci, 2014-06-09 19:00:21]

Couple questions:

What's the rationale for not doing the commit? In particular, this would let us
add a link to the gitiles +log/old..new for the rolled dep.

Also, this doesn't appear to require/use strict mode, which I think would help
us catch arbitrary python syntax early, instead of letting a hand-rolled DEPS
file quietly break the tool.

[szager1, 2014-06-09 19:05:26]

On 2014/06/09 19:00:21, iannucci wrote:
> Couple questions:
> 
> What's the rationale for not doing the commit? In particular, this would let
us
> add a link to the gitiles +log/old..new for the rolled dep.

I think the tool should allow additional edits to be added before commit.  I can
create a .git/COMMIT_MSG with a canned message and the gitiles links.


> Also, this doesn't appear to require/use strict mode, which I think would help
> us catch arbitrary python syntax early, instead of letting a hand-rolled DEPS
> file quietly break the tool.

It does require strict mode, in the sense that the DEPS file must be parse-able
python, and it only supports a limited syntax (e.g, no custom_vars).  Is there
something else you had in mind?

[szager1, 2014-06-09 19:05:32]

https://codereview.chromium.org/318153003/diff/40001/roll_dep.py
File roll_dep.py (right):

https://codereview.chromium.org/318153003/diff/40001/roll_dep.py#newcode71
roll_dep.py:71: log_p = Popen(['git', 'log', 'origin/master'],
On 2014/06/09 16:01:34, Michael Moss wrote:
> What about branch revisions? Are we not worried about that? Could it fallback
to
> scanning all branches if a match isn't found?

I actually had an earlier version of this that scanned all remote branches, but
then I realized that it was dangerous, because gclient isn't set up to pull
anything other than origin/master.  Someone could innocently roll deps to a
non-master branch revision, and break gclient sync.

I'm not against adding it back, but only once there's support in gclient.

https://codereview.chromium.org/318153003/diff/40001/roll_dep.py#newcode97
roll_dep.py:97: '"git fetch origin" to get the latest commits.' % svn_rev)
On 2014/06/09 16:01:34, Michael Moss wrote:
> Any reason not to make this automatic?

Not a strong reason.  I think it's kind of weird to try and roll deps to a
revision that's not in your working copy, so I guess I'd like to call that out
rather than quietly fetch the missing revision.  Also, I want this to be a
light-weight tool that runs quickly and doesn't connect to the internet.  I
don't think it's too burdensome to expect devs to 'git fetch' themselves.

https://codereview.chromium.org/318153003/diff/40001/roll_dep.py#newcode212
roll_dep.py:212: for os_node in deps_os_node.values:
On 2014/06/09 16:01:34, Michael Moss wrote:
> Not sure it makes sense to always update os deps, since they could
intentionally
> be different than the main deps, but I guess that's easy enough for someone to
> undo before they commit. If you wanted to be fancy, maybe you could detect if
> they started out at different revisions and then prompt the user if they want
to
> update them all to the same revision, but not a big deal.

I don't think gclient can even handle a deps_os entry that conflicts with a deps
entry.  AFAIK, there are only two legitimate use cases for deps_os: to add a
distinct dependency, and to disable the checkout of something in deps that isn't
needed for the target platform.

https://codereview.chromium.org/318153003/diff/40001/roll_dep.py#newcode232
roll_dep.py:232: if len(argv) !=2 :
On 2014/06/09 16:01:34, Michael Moss wrote:
> Nit: space after !=

Done.

[iannucci, 2014-06-09 19:10:21]

On 2014/06/09 19:05:26, szager1 wrote:
> On 2014/06/09 19:00:21, iannucci wrote:
> > Couple questions:
> > 
> > What's the rationale for not doing the commit? In particular, this would let
> us
> > add a link to the gitiles +log/old..new for the rolled dep.
> 
> I think the tool should allow additional edits to be added before commit.  I
can
> create a .git/COMMIT_MSG with a canned message and the gitiles links.

That would be great :)

> 
> 
> > Also, this doesn't appear to require/use strict mode, which I think would
help
> > us catch arbitrary python syntax early, instead of letting a hand-rolled
DEPS
> > file quietly break the tool.
> 
> It does require strict mode, in the sense that the DEPS file must be
parse-able
> python, and it only supports a limited syntax (e.g, no custom_vars).  Is there
> something else you had in mind?

I was meaning asserting that 'use strict' is in the first line of the file :)
(not currently the case, though I've verified that the chromium DEPS file is
compatible in this mode).
https://code.google.com/p/chromium/codesearch#chromium/tools/depot_tools/gcli...

Maybe we could add this in later, but I'd like to get all of our
tools-which-assume-limited-DEPS-syntax to rely on a DEPS file which is only
parsable under that limited syntax.

[Michael Moss, 2014-06-09 21:13:17]

lgtm

https://codereview.chromium.org/318153003/diff/40001/roll_dep.py
File roll_dep.py (right):

https://codereview.chromium.org/318153003/diff/40001/roll_dep.py#newcode71
roll_dep.py:71: log_p = Popen(['git', 'log', 'origin/master'],
On 2014/06/09 19:05:32, szager1 wrote:
> On 2014/06/09 16:01:34, Michael Moss wrote:
> > What about branch revisions? Are we not worried about that? Could it
fallback
> to
> > scanning all branches if a match isn't found?
> 
> I actually had an earlier version of this that scanned all remote branches,
but
> then I realized that it was dangerous, because gclient isn't set up to pull
> anything other than origin/master.  Someone could innocently roll deps to a
> non-master branch revision, and break gclient sync.
> 
> I'm not against adding it back, but only once there's support in gclient.

What about defaulting to 'HEAD' and then falling back to 'origin/master'? That
way if they're on a release branch, intending to roll DEPS for that branch, then
it might actually work, but without too much risk of accidentally rolling trunk
to something insane?

[szager1, 2014-06-09 21:22:05]

On 2014/06/09 21:13:17, Michael Moss wrote:
> lgtm
> 
> https://codereview.chromium.org/318153003/diff/40001/roll_dep.py
> File roll_dep.py (right):
> 
> https://codereview.chromium.org/318153003/diff/40001/roll_dep.py#newcode71
> roll_dep.py:71: log_p = Popen(['git', 'log', 'origin/master'],
> On 2014/06/09 19:05:32, szager1 wrote:
> > On 2014/06/09 16:01:34, Michael Moss wrote:
> > > What about branch revisions? Are we not worried about that? Could it
> fallback
> > to
> > > scanning all branches if a match isn't found?
> > 
> > I actually had an earlier version of this that scanned all remote branches,
> but
> > then I realized that it was dangerous, because gclient isn't set up to pull
> > anything other than origin/master.  Someone could innocently roll deps to a
> > non-master branch revision, and break gclient sync.
> > 
> > I'm not against adding it back, but only once there's support in gclient.
> 
> What about defaulting to 'HEAD' and then falling back to 'origin/master'? That
> way if they're on a release branch, intending to roll DEPS for that branch,
then
> it might actually work, but without too much risk of accidentally rolling
trunk
> to something insane?

Done.

[szager1, 2014-06-09 22:44:40]

On 2014/06/09 19:10:21, iannucci wrote:
> On 2014/06/09 19:05:26, szager1 wrote:

> > I think the tool should allow additional edits to be added before commit.  I
> can
> > create a .git/COMMIT_MSG with a canned message and the gitiles links.
> 
> That would be great :)

Done.


> > It does require strict mode, in the sense that the DEPS file must be
> parse-able
> > python, and it only supports a limited syntax (e.g, no custom_vars).  Is
there
> > something else you had in mind?
> 
> I was meaning asserting that 'use strict' is in the first line of the file :)
> (not currently the case, though I've verified that the chromium DEPS file is
> compatible in this mode).
>
https://code.google.com/p/chromium/codesearch#chromium/tools/depot_tools/gcli...
> 
> Maybe we could add this in later, but I'd like to get all of our
> tools-which-assume-limited-DEPS-syntax to rely on a DEPS file which is only
> parsable under that limited syntax.

Fine with me, but let's not put the cart before the horse: first, we need to put
the magic string in the DEPS files, and *then* update our tools.

[iannucci, 2014-06-09 23:01:58]

On 2014/06/09 22:44:40, szager1 wrote:
> On 2014/06/09 19:10:21, iannucci wrote:
> > On 2014/06/09 19:05:26, szager1 wrote:
> 
> > > I think the tool should allow additional edits to be added before commit. 
I
> > can
> > > create a .git/COMMIT_MSG with a canned message and the gitiles links.
> > 
> > That would be great :)
> 
> Done.
> 
> 
> > > It does require strict mode, in the sense that the DEPS file must be
> > parse-able
> > > python, and it only supports a limited syntax (e.g, no custom_vars).  Is
> there
> > > something else you had in mind?
> > 
> > I was meaning asserting that 'use strict' is in the first line of the file
:)
> > (not currently the case, though I've verified that the chromium DEPS file is
> > compatible in this mode).
> >
>
https://code.google.com/p/chromium/codesearch#chromium/tools/depot_tools/gcli...
> > 
> > Maybe we could add this in later, but I'd like to get all of our
> > tools-which-assume-limited-DEPS-syntax to rely on a DEPS file which is only
> > parsable under that limited syntax.
> 
> Fine with me, but let's not put the cart before the horse: first, we need to
put
> the magic string in the DEPS files, and *then* update our tools.

Sure... the string needs to land at >= the landing of the tools.

[szager1, 2014-06-09 23:04:55]

The CQ bit was checked by szager@chromium.org

[commit-bot: I haz the power, 2014-06-09 23:33:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/szager@chromium.org/318153003/80001

[commit-bot: I haz the power, 2014-06-09 23:33:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-09 23:33:45]

Failed to apply patch for depot_tools/roll-dep.bat:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  A         depot_tools/roll-dep.bat
  Copied depot_tools/apply_issue.bat -> depot_tools/roll-dep.bat
  patching file depot_tools/roll-dep.bat
  Hunk #1 FAILED at 2.
  1 out of 1 hunk FAILED -- saving rejects to file depot_tools/roll-dep.bat.rej

Patch:   NR  depot_tools/apply_issue.bat->depot_tools/roll-dep.bat
Index: roll-dep.bat
diff --git depot_tools/apply_issue.bat depot_tools/roll-dep.bat
similarity index 81%
copy from depot_tools/apply_issue.bat
copy to depot_tools/roll-dep.bat
index
7e052ee3454fe62be82602f0e2ad4b8f849a39aa..d8b69c27c2efb6c0e806f2fa8aad225a66c01291
100755
--- a/depot_tools/apply_issue.bat
+++ b/depot_tools/roll-dep.bat
@@ -2,10 +2,9 @@
 :: Copyright (c) 2012 The Chromium Authors. All rights reserved.
 :: Use of this source code is governed by a BSD-style license that can be
 :: found in the LICENSE file.
-setlocal
 
 :: This is required with cygwin only.
 PATH=%~dp0;%PATH%
 
 :: Defer control.
-%~dp0python "%~dp0\apply_issue.py" %*
+%~dp0python "%~dp0\roll_dep.py" %*

[szager1, 2014-06-09 23:43:36]

Committed patchset #7 manually as r275875.