Add support in gclient for pre-DEPS hooks

Add support in gclient for pre-DEPS hooks

These are run for a given dependency after it has been synced but before its
DEPS have been synced. This will help to switch Chromium to depend on Skia's
git repository (skia:1638).

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=228651

[borenet, 2013-09-30 18:23:14]

Uploading a first pass.  This will allow us to switch and sync Skia from
skia.googlesource.com/skia.git instead of skia.googlecode.com/svn in one commit
which does the following:

1. Removes src/third_party/skia/LICENSE, src/third_party/skia/OWNERS, and
src/third_party/README.chromium

2. Changes Skia DEPS from:
  "src/third_party/skia/src":
    (Var("googlecode_url") % "skia") + "/trunk/src@" + Var("skia_revision"),

  "src/third_party/skia/gyp":
    (Var("googlecode_url") % "skia") + "/trunk/gyp@" + Var("skia_revision"),

  "src/third_party/skia/include":
    (Var("googlecode_url") % "skia") + "/trunk/include@" + Var("skia_revision"),

to:
'src/third_party/skia': Var('skia_git) + '/skia.git@' + Var("skia_hash"),

3. Adds a "remove_dep_if_needed" script. This script determines whether a given
directory (eg src/third_party/skia) contains a checkout of a desired repo (eg
https://skia.googlesource.com/skia.git) and if not, moves the directory into a
"deleteme" directory.

4. Adds a pre_deps_hook for src which runs "remove_dep_if_needed" for
src/third_party/skia to remove it if it isn't a checkout from
https://skia.googlesource.com/skia.git.  Per this CL, this hook runs after src
is synced (so that the new DEPS file is picked up) but before any of the DEPS
are synced, so src/third_party/skia will be empty before we try to sync Skia
from the git repo.

[borenet, 2013-10-01 19:55:47]

On 2013/09/30 18:23:14, borenet wrote:
> Uploading a first pass.  This will allow us to switch and sync Skia from
> skia.googlesource.com/skia.git instead of skia.googlecode.com/svn in one
commit
> which does the following:
> 
> 1. Removes src/third_party/skia/LICENSE, src/third_party/skia/OWNERS, and
> src/third_party/README.chromium
> 
> 2. Changes Skia DEPS from:
>   "src/third_party/skia/src":
>     (Var("googlecode_url") % "skia") + "/trunk/src@" + Var("skia_revision"),
> 
>   "src/third_party/skia/gyp":
>     (Var("googlecode_url") % "skia") + "/trunk/gyp@" + Var("skia_revision"),
> 
>   "src/third_party/skia/include":
>     (Var("googlecode_url") % "skia") + "/trunk/include@" +
Var("skia_revision"),
> 
> to:
> 'src/third_party/skia': Var('skia_git) + '/skia.git@' + Var("skia_hash"),
> 
> 3. Adds a "remove_dep_if_needed" script. This script determines whether a
given
> directory (eg src/third_party/skia) contains a checkout of a desired repo (eg
> https://skia.googlesource.com/skia.git) and if not, moves the directory into a
> "deleteme" directory.
> 
> 4. Adds a pre_deps_hook for src which runs "remove_dep_if_needed" for
> src/third_party/skia to remove it if it isn't a checkout from
> https://skia.googlesource.com/skia.git.  Per this CL, this hook runs after src
> is synced (so that the new DEPS file is picked up) but before any of the DEPS
> are synced, so src/third_party/skia will be empty before we try to sync Skia
> from the git repo.

Ping.  What are chrome-infra's thoughts on this?

[cmp, 2013-10-01 20:01:23]

> Ping.  What are chrome-infra's thoughts on this?

Anytime you add a new hook to DEPS or modify gclient you will elicit interesting
responses. :)

First, I'm open to this, especially since it simplifies switching bot checkouts
around in synchronization with the waterfalls and builds.  Next, your CL doesn't
add any tests, so please take a pass at two or three.  Once the tests are in
place and working, you'll get traction on a review and landing it.

[iannucci, 2013-10-02 21:44:14]

This change generally seems good to me, though it does create a rather large
affordance for breaking the whole fleet at once :D

I would almost be inclined to swallow the error from the pre-deps hooks rather
than aborting gclient. I believe if gclient fails then the slaves will currently
take that as an opportunity to clobber All The Things.

[borenet, 2013-10-03 19:06:30]

Thanks guys.  I expected a lot of push back on this one, so I wanted to get the
fighting out of the way first.  PTAL at patch set 2, which adds a couple of
tests.

[borenet, 2013-10-07 19:19:22]

On 2013/10/03 19:06:30, borenet wrote:
> Thanks guys.  I expected a lot of push back on this one, so I wanted to get
the
> fighting out of the way first.  PTAL at patch set 2, which adds a couple of
> tests.

ping

[iannucci, 2013-10-07 22:40:08]

wdyt about an alternate flag? I don't think gclient has enough options...

https://codereview.chromium.org/25322002/diff/6001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/25322002/diff/6001/gclient.py#newcode660
gclient.py:660: self.RunPreDepsHooks()
So, the bots generally run with --nohooks. If we want to use this for the bots
then maybe we should have a --noprehooks flag?

[borenet, 2013-10-08 13:53:18]

https://codereview.chromium.org/25322002/diff/6001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/25322002/diff/6001/gclient.py#newcode660
gclient.py:660: self.RunPreDepsHooks()
On 2013/10/07 22:40:09, iannucci wrote:
> So, the bots generally run with --nohooks. If we want to use this for the bots
> then maybe we should have a --noprehooks flag?

This sounds fine to me, if adding another flag is alright.

[iannucci, 2013-10-08 17:50:10]

On 2013/10/08 13:53:18, borenet wrote:
> https://codereview.chromium.org/25322002/diff/6001/gclient.py
> File gclient.py (right):
> 
> https://codereview.chromium.org/25322002/diff/6001/gclient.py#newcode660
> gclient.py:660: self.RunPreDepsHooks()
> On 2013/10/07 22:40:09, iannucci wrote:
> > So, the bots generally run with --nohooks. If we want to use this for the
bots
> > then maybe we should have a --noprehooks flag?
> 
> This sounds fine to me, if adding another flag is alright.

Yeah, I would be inclined to do this, since I think the pre-deps hooks are
intended for a different purpose (i.e. doing SCM conversions, moving checkouts,
etc.).

One thing that's a little concerning to me is that the pre-deps hooks will have
to basically live forever. Would it make sense to add an 'expiration date' to
them so they automatically become inert after, say 3 months, and then someone
else could remove them with the full knowledge that nothing is using them any
more? Otherwise they turn into "I'm not sure if anyone is still using this code,
so I'll leave it in..." where the original developer could know that after some
time frame the code won't be useful any more.

[borenet, 2013-10-09 15:27:55]

On 2013/10/08 17:50:10, iannucci wrote:
> On 2013/10/08 13:53:18, borenet wrote:
> > https://codereview.chromium.org/25322002/diff/6001/gclient.py
> > File gclient.py (right):
> > 
> > https://codereview.chromium.org/25322002/diff/6001/gclient.py#newcode660
> > gclient.py:660: self.RunPreDepsHooks()
> > On 2013/10/07 22:40:09, iannucci wrote:
> > > So, the bots generally run with --nohooks. If we want to use this for the
> bots
> > > then maybe we should have a --noprehooks flag?
> > 
> > This sounds fine to me, if adding another flag is alright.
> 
> Yeah, I would be inclined to do this, since I think the pre-deps hooks are
> intended for a different purpose (i.e. doing SCM conversions, moving
checkouts,
> etc.).
> 
> One thing that's a little concerning to me is that the pre-deps hooks will
have
> to basically live forever. Would it make sense to add an 'expiration date' to
> them so they automatically become inert after, say 3 months, and then someone
> else could remove them with the full knowledge that nothing is using them any
> more? Otherwise they turn into "I'm not sure if anyone is still using this
code,
> so I'll leave it in..." where the original developer could know that after
some
> time frame the code won't be useful any more.

Uploaded patch set 3 to add the --noprehooks flag.  Regarding expiration dates,
the primary reason we're doing this instead of just changing Skia's DEP outright
is so that the bots will stay happy.  If the pre-DEPS hook which (re)moves the
svn checkout of Skia expires or is removed after a couple of days/weeks, the
bots will be fine, but there will probably be a handful of developers who
haven't synced in a while and need to manually remove src/third_party/skia.  I
think we've decided that this is okay, and my plan is to just remove the
pre-DEPS hook after things seem stable.  I can add a comment to that effect when
I submit the change to add the prehook.

[iannucci, 2013-10-09 16:41:23]

What about just setting it to something like 3 months? Or 6 months? or a year?
Just to put an upper bound on it.

A comment is better than nothing, but they tend to be untrustworthy (i.e.
someone looking at it may not be able to make the determination that it's
/really/ not needed any more, even though the comment pinky-swears that
absolutely nothing uses it after such-and-such). I'm not saying I doubt that
you'll remove it, but Things Happen :)

Other than that, lgtm % comment about default value.

https://codereview.chromium.org/25322002/diff/17001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/25322002/diff/17001/gclient.py#newcode1850
gclient.py:1850: options.noprehooks = False
Hm... I don't think any of these are really necessary, since you can set
default=... on the parser entry? Or am I missing something?

[borenet, 2013-10-10 18:24:29]

Uploaded patch set 4.  I'm hesitant to mess with expiration dates in automation,
unless we're really sure about it.  The phrase "time bomb" comes to mind.

I'm actually not sure that I have the default behavior right for all of the
commands.  For example, I'm pretty sure that for "runhooks" we don't want to run
the pre-DEPS hooks, but as it is, they will run.  I don't know what the correct
set of commands should be...

https://codereview.chromium.org/25322002/diff/17001/gclient.py
File gclient.py (right):

https://codereview.chromium.org/25322002/diff/17001/gclient.py#newcode1850
gclient.py:1850: options.noprehooks = False
On 2013/10/09 16:41:24, iannucci wrote:
> Hm... I don't think any of these are really necessary, since you can set
> default=... on the parser entry? Or am I missing something?

If I understand correctly, these are defaults for commands  which don't expose
these as optional.  I get a bunch of test failures (eg. testRevertAndStatus or
testRunHooks) when I don't set a default here.

[iannucci, 2013-10-10 18:47:15]

On 2013/10/10 18:24:29, borenet wrote:
> Uploaded patch set 4.  I'm hesitant to mess with expiration dates in
automation,
> unless we're really sure about it.  The phrase "time bomb" comes to mind.

Fair enough, a comment should suffice then :)

> 
> I'm actually not sure that I have the default behavior right for all of the
> commands.  For example, I'm pretty sure that for "runhooks" we don't want to
run
> the pre-DEPS hooks, but as it is, they will run.  I don't know what the
correct
> set of commands should be...

Hm.. I would expect that they're independent of one another. For example if you
wanted to move a repo, you'd still want to run gyp when everything finished,
right?

> 
> https://codereview.chromium.org/25322002/diff/17001/gclient.py
> File gclient.py (right):
> 
> https://codereview.chromium.org/25322002/diff/17001/gclient.py#newcode1850
> gclient.py:1850: options.noprehooks = False
> On 2013/10/09 16:41:24, iannucci wrote:
> > Hm... I don't think any of these are really necessary, since you can set
> > default=... on the parser entry? Or am I missing something?
> 
> If I understand correctly, these are defaults for commands  which don't expose
> these as optional.  I get a bunch of test failures (eg. testRevertAndStatus or
> testRunHooks) when I don't set a default here.

Bah, I think you're right.

[borenet, 2013-10-10 18:55:14]

> > 
> > I'm actually not sure that I have the default behavior right for all of the
> > commands.  For example, I'm pretty sure that for "runhooks" we don't want to
> run
> > the pre-DEPS hooks, but as it is, they will run.  I don't know what the
> correct
> > set of commands should be...
> 
> Hm.. I would expect that they're independent of one another. For example if
you
> wanted to move a repo, you'd still want to run gyp when everything finished,
> right?
> 

Right.  I don't *think* we want to be running pre-DEPS hooks when we run
"gclient status" for example.  In fact, maybe the only time we want them to run
is "gclient sync"?

[iannucci, 2013-10-10 20:00:31]

On 2013/10/10 18:55:14, borenet wrote:
> > > 
> > > I'm actually not sure that I have the default behavior right for all of
the
> > > commands.  For example, I'm pretty sure that for "runhooks" we don't want
to
> > run
> > > the pre-DEPS hooks, but as it is, they will run.  I don't know what the
> > correct
> > > set of commands should be...
> > 
> > Hm.. I would expect that they're independent of one another. For example if
> you
> > wanted to move a repo, you'd still want to run gyp when everything finished,
> > right?
> > 
> 
> Right.  I don't *think* we want to be running pre-DEPS hooks when we run
> "gclient status" for example.  In fact, maybe the only time we want them to
run
> is "gclient sync"?

Ah, yes.... I think you're right, we'll only want to run it when we do sync.

[iannucci, 2013-10-10 20:01:12]

On 2013/10/10 20:00:31, iannucci wrote:
> On 2013/10/10 18:55:14, borenet wrote:
> > > > 
> > > > I'm actually not sure that I have the default behavior right for all of
> the
> > > > commands.  For example, I'm pretty sure that for "runhooks" we don't
want
> to
> > > run
> > > > the pre-DEPS hooks, but as it is, they will run.  I don't know what the
> > > correct
> > > > set of commands should be...
> > > 
> > > Hm.. I would expect that they're independent of one another. For example
if
> > you
> > > wanted to move a repo, you'd still want to run gyp when everything
finished,
> > > right?
> > > 
> > 
> > Right.  I don't *think* we want to be running pre-DEPS hooks when we run
> > "gclient status" for example.  In fact, maybe the only time we want them to
> run
> > is "gclient sync"?
> 
> Ah, yes.... I think you're right, we'll only want to run it when we do sync.

Maybe not even for "runhooks".

[borenet, 2013-10-10 20:42:28]

On 2013/10/10 20:01:12, iannucci wrote:
> On 2013/10/10 20:00:31, iannucci wrote:
> > On 2013/10/10 18:55:14, borenet wrote:
> > > > > 
> > > > > I'm actually not sure that I have the default behavior right for all
of
> > the
> > > > > commands.  For example, I'm pretty sure that for "runhooks" we don't
> want
> > to
> > > > run
> > > > > the pre-DEPS hooks, but as it is, they will run.  I don't know what
the
> > > > correct
> > > > > set of commands should be...
> > > > 
> > > > Hm.. I would expect that they're independent of one another. For example
> if
> > > you
> > > > wanted to move a repo, you'd still want to run gyp when everything
> finished,
> > > > right?
> > > > 
> > > 
> > > Right.  I don't *think* we want to be running pre-DEPS hooks when we run
> > > "gclient status" for example.  In fact, maybe the only time we want them
to
> > run
> > > is "gclient sync"?
> > 
> > Ah, yes.... I think you're right, we'll only want to run it when we do sync.
> 
> Maybe not even for "runhooks".

Patch set 5: Only run pre-DEPS hooks for "sync" and "revert"

[iannucci, 2013-10-10 21:16:44]

On 2013/10/10 20:42:28, borenet wrote:
> On 2013/10/10 20:01:12, iannucci wrote:
> > On 2013/10/10 20:00:31, iannucci wrote:
> > > On 2013/10/10 18:55:14, borenet wrote:
> > > > > > 
> > > > > > I'm actually not sure that I have the default behavior right for all
> of
> > > the
> > > > > > commands.  For example, I'm pretty sure that for "runhooks" we don't
> > want
> > > to
> > > > > run
> > > > > > the pre-DEPS hooks, but as it is, they will run.  I don't know what
> the
> > > > > correct
> > > > > > set of commands should be...
> > > > > 
> > > > > Hm.. I would expect that they're independent of one another. For
example
> > if
> > > > you
> > > > > wanted to move a repo, you'd still want to run gyp when everything
> > finished,
> > > > > right?
> > > > > 
> > > > 
> > > > Right.  I don't *think* we want to be running pre-DEPS hooks when we run
> > > > "gclient status" for example.  In fact, maybe the only time we want them
> to
> > > run
> > > > is "gclient sync"?
> > > 
> > > Ah, yes.... I think you're right, we'll only want to run it when we do
sync.
> > 
> > Maybe not even for "runhooks".
> 
> Patch set 5: Only run pre-DEPS hooks for "sync" and "revert"

lgtm. Please coordinate with IRC/sheriffs when you land (this shouldn't change
any behavior, but just to be safe)

[commit-bot: I haz the power, 2013-10-15 08:11:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/borenet@google.com/25322002/27001

[commit-bot: I haz the power, 2013-10-15 08:13:17]

Change committed as 228651