1253068 - vcs-sync can't clone "normal" repos

Status: RESOLVED FIXED

(Developer Services :: General, task)

Description

[Hal Wine [:hwine] use NI!]

The fix for bug 1233611 was not complete, and let to vcs-sync not being able to re-clone a repository.

vcs-sync clones a repository under 2 conditions:
 a) as part of it's automated recovery for corrupted repositories
 b) when a new repository was is added

(a) occurred about 2 weeks ago after the glibc reboots. it was fixed manually.
(b) occurred in bug 1250618

Comment 1

[Hal Wine [:hwine] use NI!]

Attachment: Fix handling of hg_clone_option for normal path

if the hg_clone_option is not set, it must be eliminated from the argument list. otherwise, code down in subprocess tries to iterate on a NoneType.

Comment 2

[Aki Sasaki (not active)]

Comment on attachment 8725942 [details] [diff] [review]
Fix handling of hg_clone_option for normal path

>diff --git a/scripts/vcs-sync/vcs_sync.py b/scripts/vcs-sync/vcs_sync.py
>--- a/scripts/vcs-sync/vcs_sync.py
>+++ b/scripts/vcs-sync/vcs_sync.py
>@@ -331,23 +331,28 @@ intree=1
>             """
>         hg = self._query_hg_exe()
>         dirs = self.query_abs_dirs()
>         repo_name = repo_config['repo_name']
>         source_dest = os.path.join(dirs['abs_source_dir'],
>                                    repo_name)
>         if clobber:
>             self.rmtree(source_dest)
>         if not os.path.exists(source_dest):
>+            # if hg_clone_option isn't specified for this repo, we need
>+            # to remove it from the argument list.  leave it in position
>+            # and remove (most common execution path) rather than doing
>+            # magic insert. (bug 1253068)
>+            args = [hg + ['clone', '--noupdate', repo_config['repo'],
>+                repo_config.get('hg_clone_option'), source_dest], ]

I think you need to remove the outside square brackets.  This is a list-inside-a-list, so when you iterate in the next line, your first item will be a list, and so you won't get rid of the None.

So this line should be

            args = hg + ['clone', '--noupdate', repo_config['repo'],
                repo_config.get('hg_clone_option'), source_dest]

>+            args = [x for x in args if x]

If args will ever need something like an empty string '' or an int 0 (I can't imagine a commandline arg will ever be an int, but I have seen commands require an empty string), then this should be

            args = [x for x in args if x is not None]

If you're not worried about those edge cases, it's fine as is.

>             if self.retry(
>                 self.run_command,
>-                args=(hg + ['clone', '--noupdate', repo_config['repo'],
>-                      repo_config.get('hg_clone_option'),
>-                      source_dest], ),
>+                args=[args],

We've discussed this in #releng.

Once args is a list instead of a list-inside-a-list, [args] is fine.

r- as is, r+ with the original args definition as a list instead of a list-inside-a-list, and it's up to you if you do the |if x is not None|

Comment 3

[Aki Sasaki (not active)]

https://gist.github.com/escapewindow/15131f80712efdc21c28 for examples.

Comment 4

[Hal Wine [:hwine] use NI!]

https://hg.mozilla.org/build/mozharness/rev/59a67e4b4815c8233a0191e995b30808b3ecb357
bug 1253068 - can't clone normal repos; r=aki

Comment 5

[Hal Wine [:hwine] use NI!]

Running without issue for over a day, so declaring fixed.

An annotated example of the issue:
  https://gist.github.com/anonymous/3394b0c5afe4ebfd2a89