gclient: Fix cache_dir functionality

gclient: Fix cache_dir functionality

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

[borenet, 2014-04-08 15:30:43]

Is this a reasonable fix for this issue?

[iannucci, 2014-04-08 17:29:36]

Good catch. lgtm

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

https://codereview.chromium.org/228793002/diff/1/gclient_scm.py#newcode152
gclient_scm.py:152: if hasattr(self, 'cache_dir') and self.cache_dir:
if getattr(self, 'cache_dir', None):

https://codereview.chromium.org/228793002/diff/1/gclient_scm.py#newcode154
gclient_scm.py:154: full_cache_dir = self._Run(['cache', 'exists',
'--cache-dir',
Note that there seems to be flakiness when invoking git subcommands this way on
windows. We're looking into it.

https://codereview.chromium.org/228793002/diff/1/gclient_scm.py#newcode159
gclient_scm.py:159: if full_cache_dir == actual_remote_url:
I'm not sure if this will work well on windows.

[iannucci, 2014-04-08 17:30:00]

+hinoka, agable for fyi

[borenet, 2014-04-08 18:42:13]

Uploaded patch set 2.

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

https://codereview.chromium.org/228793002/diff/1/gclient_scm.py#newcode152
gclient_scm.py:152: if hasattr(self, 'cache_dir') and self.cache_dir:
On 2014/04/08 17:29:36, iannucci wrote:
> if getattr(self, 'cache_dir', None):

Done.

[borenet, 2014-04-08 19:23:42]

Uploaded patch set 3.

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

https://codereview.chromium.org/228793002/diff/1/gclient_scm.py#newcode159
gclient_scm.py:159: if full_cache_dir == actual_remote_url:
On 2014/04/08 17:29:36, iannucci wrote:
> I'm not sure if this will work well on windows.

Used .replace('\\', '/')

[iannucci, 2014-04-08 19:37:35]

lgtm

[borenet, 2014-04-08 19:38:21]

The CQ bit was checked by borenet@google.com

[commit-bot: I haz the power, 2014-04-08 19:38:27]

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

[commit-bot: I haz the power, 2014-04-08 19:40:31]

Change committed as 262478

[v.putkinen, 2014-04-09 08:56:59]

On 2014/04/08 19:40:31, I haz the power (commit-bot) wrote:
> Change committed as 262478

Awesome, thanks for the quick response! There is a bug in the fix though:
self.url may contain a @revision postfix. A fix proposal:

diff --git a/gclient_scm.py b/gclient_scm.py
index 5f04df0..1bbc394 100644
--- a/gclient_scm.py
+++ b/gclient_scm.py
@@ -166,8 +166,9 @@ class SCMWrapper(object):
       # If a cache_dir is used, obtain the actual remote URL from the cache.
       if getattr(self, 'cache_dir', None):
         try:
+          url, _ = gclient_utils.SplitUrlRevision(self.url)
           full_cache_dir = self._Run(['cache', 'exists', '--cache-dir',
-                                      self.cache_dir, self.url],
+                                      self.cache_dir, url],
                                      options, cwd=self._root_dir).strip()
         except subprocess2.CalledProcessError:
           full_cache_dir = None

[borenet, 2014-04-09 12:39:56]

On 2014/04/09 08:56:59, v.putkinen wrote:
> On 2014/04/08 19:40:31, I haz the power (commit-bot) wrote:
> > Change committed as 262478
> 
> Awesome, thanks for the quick response! There is a bug in the fix though:
> self.url may contain a @revision postfix. A fix proposal:
> 
> diff --git a/gclient_scm.py b/gclient_scm.py
> index 5f04df0..1bbc394 100644
> --- a/gclient_scm.py
> +++ b/gclient_scm.py
> @@ -166,8 +166,9 @@ class SCMWrapper(object):
>        # If a cache_dir is used, obtain the actual remote URL from the cache.
>        if getattr(self, 'cache_dir', None):
>          try:
> +          url, _ = gclient_utils.SplitUrlRevision(self.url)
>            full_cache_dir = self._Run(['cache', 'exists', '--cache-dir',
> -                                      self.cache_dir, self.url],
> +                                      self.cache_dir, url],
>                                       options, cwd=self._root_dir).strip()
>          except subprocess2.CalledProcessError:
>            full_cache_dir = None

Good catch!  Uploaded https://codereview.chromium.org/230773002/

[v.putkinen, 2014-04-09 12:43:59]

On 2014/04/09 08:56:59, v.putkinen wrote:
> Awesome, thanks for the quick response! There is a bug in the fix though:
> self.url may contain a @revision postfix.

Another potential problem: if cache_dir is used but the cache disappears for
some reason, full_cache_dir will be None and the method will fail with
"AttributeError: 'NoneType' object has no attribute 'replace'". This means that
gclient sync will crash with this AttributeError before it can recreate the
cache dir.

My proposal is to replace the "full_cache_dir = None" line with "return url"
(where url has been stripped of the possible @revision suffix). I think that
just returning url is justified because GetActualRemoteURL will return that same
url after the cache has been recreated (by gclient sync for example). This would
allow the system to heal itself in case the cache disappears.

[borenet, 2014-04-09 12:55:08]

On 2014/04/09 12:43:59, v.putkinen wrote:
> On 2014/04/09 08:56:59, v.putkinen wrote:
> > Awesome, thanks for the quick response! There is a bug in the fix though:
> > self.url may contain a @revision postfix.
> 
> Another potential problem: if cache_dir is used but the cache disappears for
> some reason, full_cache_dir will be None and the method will fail with
> "AttributeError: 'NoneType' object has no attribute 'replace'". This means
that
> gclient sync will crash with this AttributeError before it can recreate the
> cache dir.
> 
> My proposal is to replace the "full_cache_dir = None" line with "return url"
> (where url has been stripped of the possible @revision suffix). I think that
> just returning url is justified because GetActualRemoteURL will return that
same
> url after the cache has been recreated (by gclient sync for example). This
would
> allow the system to heal itself in case the cache disappears.

iannucci@ and I discussed the possibility that the cache_dir might be deleted
and decided it wasn't worth worrying about too much.  I think the following
would keep things working if you wanted to delete the cache_dir:

(remove cache_dir from .gclient)
$ gclient sync
(delete cache_dir)

Robbie, what if we set "self.cache_dir = None" if the "git cache" command fails
here?  I *think* that would be equivalent to removing the cache_dir line from
.gclient before syncing.

I'll wait for more discussion before doing anything more on
https://codereview.chromium.org/230773002/

[borenet, 2014-04-09 13:25:22]

On 2014/04/09 12:55:08, borenet wrote:
> On 2014/04/09 12:43:59, v.putkinen wrote:
> > On 2014/04/09 08:56:59, v.putkinen wrote:
> > > Awesome, thanks for the quick response! There is a bug in the fix though:
> > > self.url may contain a @revision postfix.
> > 
> > Another potential problem: if cache_dir is used but the cache disappears for
> > some reason, full_cache_dir will be None and the method will fail with
> > "AttributeError: 'NoneType' object has no attribute 'replace'". This means
> that
> > gclient sync will crash with this AttributeError before it can recreate the
> > cache dir.
> > 
> > My proposal is to replace the "full_cache_dir = None" line with "return url"
> > (where url has been stripped of the possible @revision suffix). I think that
> > just returning url is justified because GetActualRemoteURL will return that
> same
> > url after the cache has been recreated (by gclient sync for example). This
> would
> > allow the system to heal itself in case the cache disappears.
> 
> iannucci@ and I discussed the possibility that the cache_dir might be deleted
> and decided it wasn't worth worrying about too much.  I think the following
> would keep things working if you wanted to delete the cache_dir:
> 
> (remove cache_dir from .gclient)
> $ gclient sync
> (delete cache_dir)
> 
> Robbie, what if we set "self.cache_dir = None" if the "git cache" command
fails
> here?  I *think* that would be equivalent to removing the cache_dir line from
> .gclient before syncing.
> 
> I'll wait for more discussion before doing anything more on
> https://codereview.chromium.org/230773002/

Just hacking around a bit, I was able to make gclient re-create the cache_dir if
it disappears by doing:

diff --git a/gclient_scm.py b/gclient_scm.py
index 5f04df0..da8a342 100644
--- a/gclient_scm.py
+++ b/gclient_scm.py
@@ -166,11 +166,17 @@ class SCMWrapper(object):
       # If a cache_dir is used, obtain the actual remote URL from the cache.
       if getattr(self, 'cache_dir', None):
         try:
+          url, _ = gclient_utils.SplitUrlRevision(self.url)
           full_cache_dir = self._Run(['cache', 'exists', '--cache-dir',
-                                      self.cache_dir, self.url],
+                                      self.cache_dir, url],
                                      options, cwd=self._root_dir).strip()
         except subprocess2.CalledProcessError:
-          full_cache_dir = None
+          gclient_utils.AddWarning('Cannot access cache_dir %s; attempting to '
+                                   'continue anyway.' % self.cache_dir)
+          scm.GIT.Capture(['config', '--local', 'remote.origin.url', url],
+                          self.checkout_path)
+          self.cache_dir = None
+          return url
         if (full_cache_dir.replace('\\', '/') ==
             actual_remote_url.replace('\\', '/')):
           actual_remote_url = shlex.split(scm.GIT.Capture(


Thoughts?

[v.putkinen, 2014-04-09 13:46:16]

On 2014/04/09 13:25:22, borenet wrote:
> Just hacking around a bit, I was able to make gclient re-create the cache_dir
if
> it disappears by doing...

I tested this patch and it works nicely for me.