Make git_cache.py import-able.

Make git_cache.py import-able.

Evidence indicates that running non-builtin git commands is very
slow in msysgit, slow enough to dominate the running time of
gclient sync.  With this change, gclient never shells out to
git-cache; it import the lib directly instead.

R=agable@chromium.org,hinoka@chromium.org
BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=262759

[Ryan Tseng, 2014-04-09 00:39:43]

some comments.

https://codereview.chromium.org/229653002/diff/1/gclient_scm.py
File gclient_scm.py (right):

https://codereview.chromium.org/229653002/diff/1/gclient_scm.py#newcode741
gclient_scm.py:741: mirror.unlock()
We actually don't want to do this.  The lock is present to avoid two or more
gclient tasklets from calling populate() at the same time.

Moreover, bot_update takes care of all unlocking(), so if the repo is locked at
this point, then something is probably wrong.

https://codereview.chromium.org/229653002/diff/1/gclient_scm.py#newcode742
gclient_scm.py:742: mirror.populate(noisy=options.verbose)
bootstrap=True?

https://codereview.chromium.org/229653002/diff/1/git_cache.py
File git_cache.py (right):

https://codereview.chromium.org/229653002/diff/1/git_cache.py#newcode120
git_cache.py:120: pass
if os.exists(self.lockfile):
  raise

If the lock file is still around, we probably still want to fail.

https://codereview.chromium.org/229653002/diff/1/git_cache.py#newcode165
git_cache.py:165: def GetCachePath(cls):
This is rather convoluted, and I feel like it'll cause a bit of confusion for
the next person reading this code.
Imo it makes more sense to set this as an instance variable during __init__(). 
We already do that with the url anyways.

https://codereview.chromium.org/229653002/diff/1/git_cache.py#newcode212
git_cache.py:212: Cannot find 7z in the path. If you want git cache to be able
to bootstrap from
nit: 2 spaces here, or 1 space down on line 221/116.

https://codereview.chromium.org/229653002/diff/1/git_cache.py#newcode273
git_cache.py:273: if noisy:
why not just call it verbose all the way down the chain?

https://codereview.chromium.org/229653002/diff/1/git_cache.py#newcode288
git_cache.py:288: bootstrapped = bootstrap and self.bootstrap_repo(tempdir)
Actually we probably want
not depth and bootstrap and self.bootstrap_repo(tempdir)

So it doesn't try to bootstrap a full repo when we actually want a shallow repo

https://codereview.chromium.org/229653002/diff/1/git_cache.py#newcode382
git_cache.py:382: options.depth = 10000
no longer needed.

[szager1, 2014-04-09 05:22:29]

https://codereview.chromium.org/229653002/diff/1/gclient_scm.py
File gclient_scm.py (right):

https://codereview.chromium.org/229653002/diff/1/gclient_scm.py#newcode741
gclient_scm.py:741: mirror.unlock()
On 2014/04/09 00:39:44, Ryan T. wrote:
> We actually don't want to do this.  The lock is present to avoid two or more
> gclient tasklets from calling populate() at the same time.
> 
> Moreover, bot_update takes care of all unlocking(), so if the repo is locked
at
> this point, then something is probably wrong.

OK.  I may have gone a bit overboard in adding calls to unlock().  However, I
have seen the locking code fail sporadically on Windows.

https://codereview.chromium.org/229653002/diff/1/gclient_scm.py#newcode742
gclient_scm.py:742: mirror.populate(noisy=options.verbose)
On 2014/04/09 00:39:44, Ryan T. wrote:
> bootstrap=True?

Done.

https://codereview.chromium.org/229653002/diff/1/git_cache.py
File git_cache.py (right):

https://codereview.chromium.org/229653002/diff/1/git_cache.py#newcode120
git_cache.py:120: pass
On 2014/04/09 00:39:44, Ryan T. wrote:
> if os.exists(self.lockfile):
>   raise
> 
> If the lock file is still around, we probably still want to fail.

Below is sporadic error I see on Windows.  There's no reason why this would fail
other than Windows weirdness.  That's why I have 'pass' there.  I am open to
suggestions.

Traceback (most recent call last):
  File "E:\b\depot_tools\\gclient.py", line 1973, in <module>
    sys.exit(Main(sys.argv[1:]))
  File "E:\b\depot_tools\\gclient.py", line 1961, in Main
    return dispatcher.execute(OptionParser(), argv)
  File "E:\b\depot_tools\subcommand.py", line 245, in execute
    return command(parser, args[1:])
  File "E:\b\depot_tools\\gclient.py", line 1740, in CMDsync
    ret = client.RunOnDeps('update', args)
  File "E:\b\depot_tools\\gclient.py", line 1279, in RunOnDeps
    work_queue.flush(revision_overrides, command, args, options=self._options)
  File "E:\b\depot_tools\gclient_utils.py", line 923, in run
    self.item.run(*self.args, **self.kwargs)
  File "E:\b\depot_tools\\gclient.py", line 676, in run
    file_list)
  File "E:\b\depot_tools\gclient_scm.py", line 156, in RunCommand
    return getattr(self, command)(options, args, file_list)
  File "E:\b\depot_tools\gclient_scm.py", line 337, in update
    url = self._CreateOrUpdateCache(url, options)
  File "E:\b\depot_tools\gclient_scm.py", line 746, in _CreateOrUpdateCache
    mirror.populate(noisy=options.verbose)
  File "E:\b\depot_tools\git_cache.py", line 298, in populate
    os.rename(tempdir, self.mirror_path)
  File "E:\b\depot_tools\git_cache.py", line 110, in __exit__
    self.unlock()
  File "E:\b\depot_tools\git_cache.py", line 80, in unlock
    self._remove_lockfile()
  File "E:\b\depot_tools\git_cache.py", line 58, in _remove_lockfile
    os.remove(self.lockfile)
WindowsError: [Error 32] The process cannot access the file because it is being
used by another process:
'E:\\b\\build\\slave\\cache_dir\\chrome--internal.googlesource.com-chrome-data-dom_perf.lock'

https://codereview.chromium.org/229653002/diff/1/git_cache.py#newcode165
git_cache.py:165: def GetCachePath(cls):
On 2014/04/09 00:39:44, Ryan T. wrote:
> This is rather convoluted, and I feel like it'll cause a bit of confusion for
> the next person reading this code.
> Imo it makes more sense to set this as an instance variable during __init__().

> We already do that with the url anyways.

Unlike url, cachepath is a global setting.  It doesn't make sense to have
different cachepath for different instances of Mirror.  And it doesn't make
sense to call 'git config cache.cachepath' more than once per process.  This
arrangement guarantees that 'git config cache.cachepath' will be called at most
once, and only if SetCachePath() hasn't been called.

I'm open to other ways to implement this, as long as it preserves that behavior.

https://codereview.chromium.org/229653002/diff/1/git_cache.py#newcode212
git_cache.py:212: Cannot find 7z in the path. If you want git cache to be able
to bootstrap from
On 2014/04/09 00:39:44, Ryan T. wrote:
> nit: 2 spaces here, or 1 space down on line 221/116.

Done.

https://codereview.chromium.org/229653002/diff/1/git_cache.py#newcode273
git_cache.py:273: if noisy:
On 2014/04/09 00:39:44, Ryan T. wrote:
> why not just call it verbose all the way down the chain?

Done.

https://codereview.chromium.org/229653002/diff/1/git_cache.py#newcode288
git_cache.py:288: bootstrapped = bootstrap and self.bootstrap_repo(tempdir)
On 2014/04/09 00:39:44, Ryan T. wrote:
> Actually we probably want
> not depth and bootstrap and self.bootstrap_repo(tempdir)
> 
> So it doesn't try to bootstrap a full repo when we actually want a shallow
repo

Done.

https://codereview.chromium.org/229653002/diff/1/git_cache.py#newcode382
git_cache.py:382: options.depth = 10000
On 2014/04/09 00:39:44, Ryan T. wrote:
> no longer needed.

Done.

[szager1, 2014-04-09 17:33:48]

In the interest of making progress on the git migration stuff, I'm going to TBR
this.

[szager1, 2014-04-09 19:06:46]

Committed patchset #2 manually as r262759.

[agable, 2014-04-11 20:44:54]

Retroactive LGTM with some minor comments/nits.

https://codereview.chromium.org/229653002/diff/1/git_cache.py
File git_cache.py (right):

https://codereview.chromium.org/229653002/diff/1/git_cache.py#newcode148
git_cache.py:148: def FindExecutable(executable):
Why is this a method on Mirror?

https://codereview.chromium.org/229653002/diff/1/git_cache.py#newcode161
git_cache.py:161: def SetCachePath(cls, cachepath):
Could make cache_path a property with a @property and a @property.setter

https://codereview.chromium.org/229653002/diff/1/git_cache.py#newcode165
git_cache.py:165: def GetCachePath(cls):
On 2014/04/09 00:39:44, Ryan T. wrote:
> This is rather convoluted, and I feel like it'll cause a bit of confusion for
> the next person reading this code.
> Imo it makes more sense to set this as an instance variable during __init__().

> We already do that with the url anyways.

Can't do this at __init__ time because it's a class attribute, not an instance
attribute.

https://codereview.chromium.org/229653002/diff/1/git_cache.py#newcode211
git_cache.py:211: self.print('''
use textwrap.dedent.

[agable, 2014-04-11 20:44:58]

Retroactive LGTM with some minor comments/nits.

[iannucci, 2014-04-24 01:31:13]

On 2014/04/11 20:44:58, agable wrote:
> Retroactive LGTM with some minor comments/nits.

BTW, this is breaking tryslaves the world over:
http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...

```
Traceback (most recent call last):
  File "/mnt/scratch0/b_used/depot_tools/gclient.py", line 1974, in <module>
    sys.exit(Main(sys.argv[1:]))
  File "/mnt/scratch0/b_used/depot_tools/gclient.py", line 1962, in Main
    return dispatcher.execute(OptionParser(), argv)
  File "/mnt/scratch0/b_used/depot_tools/subcommand.py", line 245, in execute
    return command(parser, args[1:])
  File "/mnt/scratch0/b_used/depot_tools/gclient.py", line 1741, in CMDsync
    ret = client.RunOnDeps('update', args)
  File "/mnt/scratch0/b_used/depot_tools/gclient.py", line 1261, in RunOnDeps
    self._CheckConfig()
  File "/mnt/scratch0/b_used/depot_tools/gclient.py", line 1060, in _CheckConfig
    actual_url = scm.GetActualRemoteURL(self._options)
  File "/mnt/scratch0/b_used/depot_tools/gclient_scm.py", line 164, in
GetActualRemoteURL
    actual_remote_url = shlex.split(self._Capture(
AttributeError: 'SVNWrapper' object has no attribute '_Capture'
```

:(

[agable, 2014-05-01 17:07:26]

On 2014/04/24 01:31:13, iannucci wrote:
> On 2014/04/11 20:44:58, agable wrote:
> > Retroactive LGTM with some minor comments/nits.
> 
> BTW, this is breaking tryslaves the world over:
>
http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...
> 
> ```
> Traceback (most recent call last):
>   File "/mnt/scratch0/b_used/depot_tools/gclient.py", line 1974, in <module>
>     sys.exit(Main(sys.argv[1:]))
>   File "/mnt/scratch0/b_used/depot_tools/gclient.py", line 1962, in Main
>     return dispatcher.execute(OptionParser(), argv)
>   File "/mnt/scratch0/b_used/depot_tools/subcommand.py", line 245, in execute
>     return command(parser, args[1:])
>   File "/mnt/scratch0/b_used/depot_tools/gclient.py", line 1741, in CMDsync
>     ret = client.RunOnDeps('update', args)
>   File "/mnt/scratch0/b_used/depot_tools/gclient.py", line 1261, in RunOnDeps
>     self._CheckConfig()
>   File "/mnt/scratch0/b_used/depot_tools/gclient.py", line 1060, in
_CheckConfig
>     actual_url = scm.GetActualRemoteURL(self._options)
>   File "/mnt/scratch0/b_used/depot_tools/gclient_scm.py", line 164, in
> GetActualRemoteURL
>     actual_remote_url = shlex.split(self._Capture(
> AttributeError: 'SVNWrapper' object has no attribute '_Capture'
> ```
> 
> :(

This still hasn't been fixed. I'm putting together a fix right now.

[chromium-reviews, 2014-05-01 17:10:09]

You linked me to a build from 7 days ago, I'm not surprised its broken...


On Thu, May 1, 2014 at 10:07 AM, <agable@chromium.org> wrote:

> On 2014/04/24 01:31:13, iannucci wrote:
>
>> On 2014/04/11 20:44:58, agable wrote:
>> > Retroactive LGTM with some minor comments/nits.
>>
>
>  BTW, this is breaking tryslaves the world over:
>>
>
> http://build.chromium.org/p/tryserver.chromium/builders/
> chromium_presubmit/builds/63134/steps/gclient%20sync/logs/stdio
>
>  ```
>> Traceback (most recent call last):
>>    File "/mnt/scratch0/b_used/depot_tools/gclient.py", line 1974, in
>> <module>
>>      sys.exit(Main(sys.argv[1:]))
>>    File "/mnt/scratch0/b_used/depot_tools/gclient.py", line 1962, in Main
>>      return dispatcher.execute(OptionParser(), argv)
>>    File "/mnt/scratch0/b_used/depot_tools/subcommand.py", line 245, in
>> execute
>>      return command(parser, args[1:])
>>    File "/mnt/scratch0/b_used/depot_tools/gclient.py", line 1741, in
>> CMDsync
>>      ret = client.RunOnDeps('update', args)
>>    File "/mnt/scratch0/b_used/depot_tools/gclient.py", line 1261, in
>> RunOnDeps
>>      self._CheckConfig()
>>    File "/mnt/scratch0/b_used/depot_tools/gclient.py", line 1060, in
>>
> _CheckConfig
>
>>      actual_url = scm.GetActualRemoteURL(self._options)
>>    File "/mnt/scratch0/b_used/depot_tools/gclient_scm.py", line 164, in
>> GetActualRemoteURL
>>      actual_remote_url = shlex.split(self._Capture(
>> AttributeError: 'SVNWrapper' object has no attribute '_Capture'
>> ```
>>
>
>  :(
>>
>
> This still hasn't been fixed. I'm putting together a fix right now.
>
> https://codereview.chromium.org/229653002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kustermann, 2014-05-01 23:35:14]

https://codereview.chromium.org/229653002/diff/20001/gclient_scm.py
File gclient_scm.py (right):

https://codereview.chromium.org/229653002/diff/20001/gclient_scm.py#newcode163
gclient_scm.py:163: actual_remote_url = shlex.split(self._Capture(
This line raises an exception on a "gclient sync" on my machine:

~/mycheckout $ gclient sync
Traceback (most recent call last):
  File "/..../bin/depot_tools/gclient.py", line 1974, in <module>
    sys.exit(Main(sys.argv[1:]))
  File "/..../bin/depot_tools/gclient.py", line 1962, in Main
    return dispatcher.execute(OptionParser(), argv)
  File "/..../bin/depot_tools/subcommand.py", line 245, in execute
    return command(parser, args[1:])
  File "/..../bin/depot_tools/gclient.py", line 1741, in CMDsync
    ret = client.RunOnDeps('update', args)
  File "/..../bin/depot_tools/gclient.py", line 1261, in RunOnDeps
    self._CheckConfig()
  File "/..../bin/depot_tools/gclient.py", line 1060, in _CheckConfig
    actual_url = scm.GetActualRemoteURL(self._options)
  File "/..../bin/depot_tools/gclient_scm.py", line 164, in GetActualRemoteURL
    actual_remote_url = shlex.split(self._Capture(
AttributeError: 'SVNWrapper' object has no attribute '_Capture'
Sending crash report ...
  args: ['/..../bin/depot_tools/gclient.py', 'sync']
  cwd: /..../mycheckout
  exception: 'SVNWrapper' object has no attribute '_Capture'
  host: kustermann
  stack:   File "/..../bin/depo
  user: kustermann
  version: 2.7.3 (default, Feb 27 2014, 19:58:35) 
[GCC 4.6.3
A stack trace has been sent to the maintainers.


Git blame says that this CL was the last one touching this line.

Could you take a look?
Let me know if you need more information.

[chromium-reviews, 2014-05-01 23:42:15]

Does it happen if you do a clean sync?  This error message comes up when
some DEPS get switched from git <-> svn.


On Thu, May 1, 2014 at 4:35 PM, <kustermann@google.com> wrote:

>
> https://codereview.chromium.org/229653002/diff/20001/gclient_scm.py
> File gclient_scm.py (right):
>
> https://codereview.chromium.org/229653002/diff/20001/
> gclient_scm.py#newcode163
> gclient_scm.py:163: actual_remote_url = shlex.split(self._Capture(
> This line raises an exception on a "gclient sync" on my machine:
>
> ~/mycheckout $ gclient sync
>
> Traceback (most recent call last):
>   File "/..../bin/depot_tools/gclient.py", line 1974, in <module>
>     sys.exit(Main(sys.argv[1:]))
>   File "/..../bin/depot_tools/gclient.py", line 1962, in Main
>     return dispatcher.execute(OptionParser(), argv)
>   File "/..../bin/depot_tools/subcommand.py", line 245, in execute
>
>     return command(parser, args[1:])
>   File "/..../bin/depot_tools/gclient.py", line 1741, in CMDsync
>
>     ret = client.RunOnDeps('update', args)
>   File "/..../bin/depot_tools/gclient.py", line 1261, in RunOnDeps
>     self._CheckConfig()
>   File "/..../bin/depot_tools/gclient.py", line 1060, in _CheckConfig
>     actual_url = scm.GetActualRemoteURL(self._options)
>   File "/..../bin/depot_tools/gclient_scm.py", line 164, in
>
> GetActualRemoteURL
>     actual_remote_url = shlex.split(self._Capture(
> AttributeError: 'SVNWrapper' object has no attribute '_Capture'
> Sending crash report ...
>   args: ['/..../bin/depot_tools/gclient.py', 'sync']
>   cwd: /..../mycheckout
>   exception: 'SVNWrapper' object has no attribute '_Capture'
>   host: kustermann
>   stack:   File "/..../bin/depo
>   user: kustermann
>   version: 2.7.3 (default, Feb 27 2014, 19:58:35)
> [GCC 4.6.3
> A stack trace has been sent to the maintainers.
>
>
> Git blame says that this CL was the last one touching this line.
>
> Could you take a look?
> Let me know if you need more information.
>
> https://codereview.chromium.org/229653002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.