Use gclient's git_cache setting in bot_update/api.py

Use gclient's git_cache setting in bot_update/api.py

BUG=589140
R=agable@chromium.org

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

[szager1, 2016-03-03 01:11:13]

Description was changed from

==========
Use gclient's git_cache setting in bot_update/api.py

BUG=589140
R=agable@chromium.org
==========

to

==========
Use gclient's git_cache setting in bot_update/api.py

BUG=589140
R=agable@chromium.org
==========

[szager1, 2016-03-03 01:11:39]

This has test failures and less than 100% coverage.  I don't know how to fix
those things.  Please advise.

[agable, 2016-03-03 01:13:47]

On 2016/03/03 at 01:11:39, szager wrote:
> This has test failures and less than 100% coverage.  I don't know how to fix
those things.  Please advise.

For posterity:
$ cd depot_tools
$ ./recipes.py simulation_test train

[wkorman, 2016-03-03 02:19:21]

wkorman@chromium.org changed reviewers:
+ wkorman@chromium.org

[wkorman, 2016-03-03 02:19:22]

https://codereview.chromium.org/1762513002/diff/1/recipe_modules/bot_update/a...
File recipe_modules/bot_update/api.py (right):

https://codereview.chromium.org/1762513002/diff/1/recipe_modules/bot_update/a...
recipe_modules/bot_update/api.py:161: # See chromium_no_git_cache config in
gclient/api.py.
s/api/config/ ?

[szager1, 2016-03-03 02:26:50]

PTAL

[agable, 2016-03-03 17:42:30]

agable@chromium.org changed reviewers:
+ iannucci@chromium.org

[agable, 2016-03-03 17:42:31]

LGTM, +iannucci to sanity check since he's way more in-touch with recipes today
than I am.

https://codereview.chromium.org/1762513002/diff/40001/recipe_modules/bot_upda...
File recipe_modules/bot_update/api.py (right):

https://codereview.chromium.org/1762513002/diff/40001/recipe_modules/bot_upda...
recipe_modules/bot_update/api.py:161: # See chromium_no_git_cache config in
gclient/api.py.
gclient/config.py

https://codereview.chromium.org/1762513002/diff/40001/recipe_modules/gclient/...
File recipe_modules/gclient/example.expected/basic.json (right):

https://codereview.chromium.org/1762513002/diff/40001/recipe_modules/gclient/...
recipe_modules/gclient/example.expected/basic.json:92: "cache_dir =
''\nsolutions = [{'deps_file': 'DEPS', 'managed': True, 'name': 'WebKit',
'safesync_url': 'https://blink-status.appspot.com/lkgr', 'url':
'svn://svn.chromium.org/blink/trunk'}]"
I'm not sure I like cache_dir='' here.

[szager1, 2016-03-03 18:03:02]

https://codereview.chromium.org/1762513002/diff/40001/recipe_modules/bot_upda...
File recipe_modules/bot_update/api.py (right):

https://codereview.chromium.org/1762513002/diff/40001/recipe_modules/bot_upda...
recipe_modules/bot_update/api.py:161: # See chromium_no_git_cache config in
gclient/api.py.
On 2016/03/03 17:42:31, agable wrote:
> gclient/config.py

Done.

https://codereview.chromium.org/1762513002/diff/40001/recipe_modules/gclient/...
File recipe_modules/gclient/example.expected/basic.json (right):

https://codereview.chromium.org/1762513002/diff/40001/recipe_modules/gclient/...
recipe_modules/gclient/example.expected/basic.json:92: "cache_dir =
''\nsolutions = [{'deps_file': 'DEPS', 'managed': True, 'name': 'WebKit',
'safesync_url': 'https://blink-status.appspot.com/lkgr', 'url':
'svn://svn.chromium.org/blink/trunk'}]"
On 2016/03/03 17:42:31, agable wrote:
> I'm not sure I like cache_dir='' here.

This is dictated by recipe_engine.config.Single, which has strict rules about
types.  Here are a couple of ways to work around that, neither of which I think
is a good idea:

- sub-class Single
- Tweak the result of glient.c.as_jsonish() before the spec string is produced
(gclient/api.py line 245)

I did a quick survey of the use of cache_dir in gclient and git_cache; in all
cases, that code does a normal bool check rather than comparing to None.

I will let you make the call, but IMO the depot_tools code should be able to
handle cache_dir=''; insisting that it be None is fragile and invites future
unimagined-as-yet bugs.

[agable, 2016-03-03 18:11:50]

https://codereview.chromium.org/1762513002/diff/40001/recipe_modules/gclient/...
File recipe_modules/gclient/example.expected/basic.json (right):

https://codereview.chromium.org/1762513002/diff/40001/recipe_modules/gclient/...
recipe_modules/gclient/example.expected/basic.json:92: "cache_dir =
''\nsolutions = [{'deps_file': 'DEPS', 'managed': True, 'name': 'WebKit',
'safesync_url': 'https://blink-status.appspot.com/lkgr', 'url':
'svn://svn.chromium.org/blink/trunk'}]"
On 2016/03/03 at 18:03:02, szager1 wrote:
> On 2016/03/03 17:42:31, agable wrote:
> > I'm not sure I like cache_dir='' here.
> 
> This is dictated by recipe_engine.config.Single, which has strict rules about
types.  Here are a couple of ways to work around that, neither of which I think
is a good idea:
> 
> - sub-class Single
> - Tweak the result of glient.c.as_jsonish() before the spec string is produced
(gclient/api.py line 245)
> 
> I did a quick survey of the use of cache_dir in gclient and git_cache; in all
cases, that code does a normal bool check rather than comparing to None.
> 
> I will let you make the call, but IMO the depot_tools code should be able to
handle cache_dir=''; insisting that it be None is fragile and invites future
unimagined-as-yet bugs.

Actually, in my infinite cynicism, I expect depot_tools to break equally with ''
and None, and to only operate correctly if the key is missing entirely :P

But honestly I think you're right and this is probably totally fine.

[wkorman, 2016-03-03 18:59:47]

lgtm

[iannucci, 2016-03-03 20:00:16]

iannucci@chromium.org changed reviewers:
+ hinoka@chromium.org

[iannucci, 2016-03-03 20:00:17]

+hinoka

[Ryan Tseng, 2016-03-03 20:06:18]

hinoka@google.com changed reviewers:
+ hinoka@google.com, martiniss@chromium.org

[Ryan Tseng, 2016-03-03 20:06:19]

+Martiniss (who added this flag)

--git-cache-dir defaults to the default one (/b/build/slave/cache_dir) if it's
not specified.

[szager1, 2016-03-03 20:47:59]

On 2016/03/03 20:06:19, Ryan Tseng wrote:
> +Martiniss (who added this flag)
> 
> --git-cache-dir defaults to the default one (/b/build/slave/cache_dir) if it's
> not specified.

I don't see that in gclient.py, where does it happen?

[chromium-reviews, 2016-03-03 20:48:46]

https://code.google.com/p/chromium/codesearch#chromium/tools/depot_tools/reci...

On Thu, Mar 3, 2016 at 12:47 PM, <szager@chromium.org> wrote:

> On 2016/03/03 20:06:19, Ryan Tseng wrote:
> > +Martiniss (who added this flag)
> >
> > --git-cache-dir defaults to the default one (/b/build/slave/cache_dir)
> if it's
> > not specified.
>
> I don't see that in gclient.py, where does it happen?
>
> https://codereview.chromium.org/1762513002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[szager1, 2016-03-03 21:01:19]

> > > --git-cache-dir defaults to the default one (/b/build/slave/cache_dir)
> > if it's
> > > not specified.

Huh.  Looks like bot_update.py cannot function at all without a git cache. 
Probably worth it to add 'assert os.path.isdir(git_cache_dir)' somewhere.

In the meantime, though, this patch is silly.

[Ryan Tseng, 2016-03-03 21:02:34]

Not really, and we don't want to change that either, because of the fear of
taking down the git service

[szager1, 2016-03-03 21:07:17]

On 2016/03/03 21:02:34, Ryan Tseng wrote:
> Not really, and we don't want to change that either, because of the fear of
> taking down the git service

OK, then, I think the only viable way forward here is for gclient to use a
"push-able" git config along these lines:

[remote "origin"]
    url = /path/to/git_cache/repository.git
    fetch = +refs/heads/*:refs/heads/*
    pushurl = https://canonical.googlesource.com/repository.git

... and teach 'git cl land' how to fetch refs/pending/* from the pushurl.

Or something along those lines.  The good news is: this will go a long way
towards making git-cache usable by developers.

Does anyone want to volunteer for this work?  Or should I go buttonhole mal and
abw to get it some attention?

[martiniss, 2016-03-03 21:10:23]

I actually have a CL tentatively out at
https://codereview.chromium.org/1741983002 which changes a lot of the bot_update
API, including allowing you to pass this in as an argument. But maybe it's
better to just use the current gclient config. Comment attached to show how I
would do this (to allow manually passing in the gclient config).

https://codereview.chromium.org/1762513002/diff/60001/recipe_modules/bot_upda...
File recipe_modules/bot_update/api.py (right):

https://codereview.chromium.org/1762513002/diff/60001/recipe_modules/bot_upda...
recipe_modules/bot_update/api.py:163: flags.append(['--git-cache-dir',
self.m.gclient.c.cache_dir])
instead of 'self.m.gclient.c.cache_dir', use 'cfg.cache_dir'. See line 80

[szager1, 2016-03-03 22:08:24]

On 2016/03/03 21:10:23, martiniss wrote:
> I actually have a CL tentatively out at
> https://codereview.chromium.org/1741983002 which changes a lot of the
bot_update
> API, including allowing you to pass this in as an argument. But maybe it's
> better to just use the current gclient config. Comment attached to show how I
> would do this (to allow manually passing in the gclient config).

That's not the point.  bot_update.py itself is 1758 lines of code that cannot
whatsoever cope with cache_dir not being set.  For example:

def git_checkout(...):
  for sln in solutions:
    ...
    while not done:
      populate_cmd = (['cache', 'populate', ...])
      git(*populate_cmd)


It calls git-cache unconditionally without checking whether cache_dir is valid. 
bot_update.py is chock full of code like that.

[szager1, 2016-03-04 21:59:55]

PTAL

This gets rid of the doomed chromium_no_git_cache config, but it still does what
the description says: bot_update honors the git_cache setting from gclient.  And
that's still worth doing for basic sanity.

[agable, 2016-03-04 22:52:07]

lgtm

[szager1, 2016-03-04 22:57:12]

The CQ bit was checked by szager@chromium.org

[szager1, 2016-03-04 22:57:12]

The patchset sent to the CQ was uploaded after l-g-t-m from wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/1762513002/#ps100001
(title: "Make gclient cache_dir config Static again")

[commit-bot: I haz the power, 2016-03-04 22:57:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1762513002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1762513002/100001

[commit-bot: I haz the power, 2016-03-04 23:00:00]

Description was changed from

==========
Use gclient's git_cache setting in bot_update/api.py

BUG=589140
R=agable@chromium.org
==========

to

==========
Use gclient's git_cache setting in bot_update/api.py

BUG=589140
R=agable@chromium.org

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

[commit-bot: I haz the power, 2016-03-04 23:00:01]

Committed patchset #6 (id:100001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=299120

[martiniss, 2016-03-08 22:17:06]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1776183002/ by martiniss@chromium.org.

The reason for reverting is: Roll into build breaks infra crons..