[Downloader] More robust repo loading (#2121)

Previously, when downloader was loaded, the RepoManager would spawn a task to load available repos. If one repo failed loading for some reason, the function would raise and the remaining repos would never be loaded, however downloader would still appear to load correctly.

This change handles exceptions better during repo loading, but also, if an unhandled exception is raised, downloader will fail to load as it should.

Also included, as requested in #1968, is the --recurse-submodules flag in cloning/pulling repositories.

This change resolves #1950.

Signed-off-by: Toby Harradine <tobyharradine@gmail.com>

redbot/cogs/downloader/__init__.py

@@ -1,6 +1,7 @@
-from redbot.core.bot import Red
 from .downloader import Downloader
 
 
-def setup(bot: Red):
-    bot.add_cog(Downloader(bot))
+async def setup(bot):
+    cog = Downloader(bot)
+    await cog.initialize()
+    bot.add_cog(cog)

redbot/cogs/downloader/downloader.py

@@ -1,23 +1,20 @@
 import os
 import shutil
+import sys
 from pathlib import Path
 from sys import path as syspath
 from typing import Tuple, Union
 
 import discord
-import sys
-
-from redbot.core import Config
-from redbot.core import checks
+from redbot.core import checks, commands, Config
+from redbot.core.bot import Red
 from redbot.core.data_manager import cog_data_path
 from redbot.core.i18n import Translator, cog_i18n
 from redbot.core.utils.chat_formatting import box, pagify
-from redbot.core import commands
 
-from redbot.core.bot import Red
+from . import errors
 from .checks import do_install_agreement
 from .converters import InstalledCog
-from .errors import CloningError, ExistingGitRepo
 from .installable import Installable
 from .log import log
 from .repo_manager import RepoManager, Repo
@@ -51,6 +48,9 @@ def __init__(self, bot: Red):
 
         self._repo_manager = RepoManager()
 
+    async def initialize(self):
+        await self._repo_manager.initialize()
+
     async def cog_install_path(self):
         """Get the current cog install path.
 
@@ -226,11 +226,16 @@ async def _repo_add(self, ctx, name: str, repo_url: str, branch: str = None):
         try:
             # noinspection PyTypeChecker
             repo = await self._repo_manager.add_repo(name=name, url=repo_url, branch=branch)
-        except ExistingGitRepo:
+        except errors.ExistingGitRepo:
             await ctx.send(_("That git repo has already been added under another name."))
-        except CloningError:
+        except errors.CloningError as err:
             await ctx.send(_("Something went wrong during the cloning process."))
-            log.exception(_("Something went wrong during the cloning process."))
+            log.exception(
+                "Something went wrong whilst cloning %s (to revision: %s)",
+                repo_url,
+                branch,
+                exc_info=err,
+            )
         else:
             await ctx.send(_("Repo `{}` successfully added.").format(name))
             if repo.install_msg is not None:

redbot/cogs/downloader/errors.py

@@ -9,6 +9,7 @@
     "HardResetError",
     "UpdateError",
     "GitDiffError",
+    "NoRemoteURL",
     "PipError",
 ]
 
@@ -96,6 +97,14 @@ class GitDiffError(GitException):
     pass
 
 
+class NoRemoteURL(GitException):
+    """
+    Thrown when no remote URL exists for a repo.
+    """
+
+    pass
+
+
 class PipError(DownloaderException):
     """
     Thrown when pip returns a non-zero return code.

redbot/cogs/downloader/repo_manager.py

@@ -2,6 +2,7 @@
 import functools
 import os
 import pkgutil
+import shutil
 import re
 from concurrent.futures import ThreadPoolExecutor
 from pathlib import Path
@@ -11,19 +12,19 @@
 
 from redbot.core import data_manager, commands
 from redbot.core.utils import safe_delete
-from .errors import *
+from . import errors
 from .installable import Installable, InstallableType
 from .json_mixins import RepoJSONMixin
 from .log import log
 
 
 class Repo(RepoJSONMixin):
-    GIT_CLONE = "git clone -b {branch} {url} {folder}"
-    GIT_CLONE_NO_BRANCH = "git clone {url} {folder}"
+    GIT_CLONE = "git clone --recurse-submodules -b {branch} {url} {folder}"
+    GIT_CLONE_NO_BRANCH = "git clone --recurse-submodules {url} {folder}"
     GIT_CURRENT_BRANCH = "git -C {path} rev-parse --abbrev-ref HEAD"
     GIT_LATEST_COMMIT = "git -C {path} rev-parse {branch}"
     GIT_HARD_RESET = "git -C {path} reset --hard origin/{branch} -q"
-    GIT_PULL = "git -C {path} pull -q --ff-only"
+    GIT_PULL = "git -C {path}  --recurse-submodules=yes -q --ff-only"
     GIT_DIFF_FILE_STATUS = "git -C {path} diff --no-commit-id --name-status {old_hash} {new_hash}"
     GIT_LOG = "git -C {path} log --relative-date --reverse {old_hash}.. {relative_file_path}"
     GIT_DISCOVER_REMOTE_URL = "git -C {path} config --get remote.origin.url"
@@ -93,7 +94,9 @@ async def _get_file_update_statuses(
         )
 
         if p.returncode != 0:
-            raise GitDiffError("Git diff failed for repo at path: {}".format(self.folder_path))
+            raise errors.GitDiffError(
+                "Git diff failed for repo at path: {}".format(self.folder_path)
+            )
 
         stdout = p.stdout.strip().decode().split("\n")
 
@@ -123,7 +126,7 @@ async def _get_commit_notes(self, old_commit_hash: str, relative_file_path: str)
         )
 
         if p.returncode != 0:
-            raise GitException(
+            raise errors.GitException(
                 "An exception occurred while executing git log on"
                 " this repo: {}".format(self.folder_path)
             )
@@ -177,7 +180,7 @@ async def clone(self) -> Tuple[str]:
         """
         exists, path = self._existing_git_repo()
         if exists:
-            raise ExistingGitRepo("A git repo already exists at path: {}".format(path))
+            raise errors.ExistingGitRepo("A git repo already exists at path: {}".format(path))
 
         if self.branch is not None:
             p = await self._run(
@@ -190,8 +193,10 @@ async def clone(self) -> Tuple[str]:
                 self.GIT_CLONE_NO_BRANCH.format(url=self.url, folder=self.folder_path).split()
             )
 
-        if p.returncode != 0:
-            raise CloningError("Error when running git clone.")
+        if p.returncode:
+            # Try cleaning up folder
+            shutil.rmtree(str(self.folder_path), ignore_errors=True)
+            raise errors.CloningError("Error when running git clone.")
 
         if self.branch is None:
             self.branch = await self.current_branch()
@@ -211,12 +216,14 @@ async def current_branch(self) -> str:
         """
         exists, _ = self._existing_git_repo()
         if not exists:
-            raise MissingGitRepo("A git repo does not exist at path: {}".format(self.folder_path))
+            raise errors.MissingGitRepo(
+                "A git repo does not exist at path: {}".format(self.folder_path)
+            )
 
         p = await self._run(self.GIT_CURRENT_BRANCH.format(path=self.folder_path).split())
 
         if p.returncode != 0:
-            raise GitException(
+            raise errors.GitException(
                 "Could not determine current branch at path: {}".format(self.folder_path)
             )
 
@@ -241,14 +248,16 @@ async def current_commit(self, branch: str = None) -> str:
 
         exists, _ = self._existing_git_repo()
         if not exists:
-            raise MissingGitRepo("A git repo does not exist at path: {}".format(self.folder_path))
+            raise errors.MissingGitRepo(
+                "A git repo does not exist at path: {}".format(self.folder_path)
+            )
 
         p = await self._run(
             self.GIT_LATEST_COMMIT.format(path=self.folder_path, branch=branch).split()
         )
 
         if p.returncode != 0:
-            raise CurrentHashError("Unable to determine old commit hash.")
+            raise errors.CurrentHashError("Unable to determine old commit hash.")
 
         return p.stdout.decode().strip()
 
@@ -268,16 +277,17 @@ async def current_url(self, folder: Path = None) -> str:
 
         Raises
         ------
-        RuntimeError
+        .NoRemoteURL
             When the folder does not contain a git repo with a FETCH URL.
+
         """
         if folder is None:
             folder = self.folder_path
 
         p = await self._run(Repo.GIT_DISCOVER_REMOTE_URL.format(path=folder).split())
 
         if p.returncode != 0:
-            raise RuntimeError("Unable to discover a repo URL.")
+            raise errors.NoRemoteURL("Unable to discover a repo URL.")
 
         return p.stdout.decode().strip()
 
@@ -295,14 +305,16 @@ async def hard_reset(self, branch: str = None) -> None:
 
         exists, _ = self._existing_git_repo()
         if not exists:
-            raise MissingGitRepo("A git repo does not exist at path: {}".format(self.folder_path))
+            raise errors.MissingGitRepo(
+                "A git repo does not exist at path: {}".format(self.folder_path)
+            )
 
         p = await self._run(
             self.GIT_HARD_RESET.format(path=self.folder_path, branch=branch).split()
         )
 
         if p.returncode != 0:
-            raise HardResetError(
+            raise errors.HardResetError(
                 "Some error occurred when trying to"
                 " execute a hard reset on the repo at"
                 " the following path: {}".format(self.folder_path)
@@ -325,7 +337,7 @@ async def update(self) -> (str, str):
         p = await self._run(self.GIT_PULL.format(path=self.folder_path).split())
 
         if p.returncode != 0:
-            raise UpdateError(
+            raise errors.UpdateError(
                 "Git pull returned a non zero exit code"
                 " for the repo located at path: {}".format(self.folder_path)
             )
@@ -354,7 +366,7 @@ async def install_cog(self, cog: Installable, target_dir: Path) -> bool:
 
         """
         if cog not in self.available_cogs:
-            raise DownloaderException("That cog does not exist in this repo")
+            raise errors.DownloaderException("That cog does not exist in this repo")
 
         if not target_dir.is_dir():
             raise ValueError("That target directory is not actually a directory.")
@@ -500,6 +512,9 @@ def __init__(self):
         loop = asyncio.get_event_loop()
         loop.create_task(self._load_repos(set=True))  # str_name: Repo
 
+    async def initialize(self):
+        await self._load_repos(set=True)
+
     @property
     def repos_folder(self) -> Path:
         data_folder = data_manager.cog_data_path(self)
@@ -511,7 +526,7 @@ def does_repo_exist(self, name: str) -> bool:
     @staticmethod
     def validate_and_normalize_repo_name(name: str) -> str:
         if not name.isidentifier():
-            raise InvalidRepoName("Not a valid Python variable name.")
+            raise errors.InvalidRepoName("Not a valid Python variable name.")
         return name.lower()
 
     async def add_repo(self, url: str, name: str, branch: Optional[str] = None) -> Repo:
@@ -533,7 +548,7 @@ async def add_repo(self, url: str, name: str, branch: Optional[str] = None) -> R
 
         """
         if self.does_repo_exist(name):
-            raise ExistingGitRepo(
+            raise errors.ExistingGitRepo(
                 "That repo name you provided already exists. Please choose another."
             )
 
@@ -584,13 +599,13 @@ async def delete_repo(self, name: str):
 
         Raises
         ------
-        MissingGitRepo
+        .MissingGitRepo
             If the repo does not exist.
 
         """
         repo = self.get_repo(name)
         if repo is None:
-            raise MissingGitRepo("There is no repo with the name {}".format(name))
+            raise errors.MissingGitRepo("There is no repo with the name {}".format(name))
 
         safe_delete(repo.folder_path)
 
@@ -629,9 +644,16 @@ async def _load_repos(self, set=False) -> MutableMapping[str, Repo]:
                 continue
             try:
                 ret[folder.stem] = await Repo.from_folder(folder)
-            except RuntimeError:
-                # Thrown when there's no findable git remote URL
-                pass
+            except errors.NoRemoteURL:
+                log.warning("A remote URL does not exist for repo %s", folder.stem)
+            except errors.DownloaderException as err:
+                log.error("Discarding repo %s due to error.", folder.stem, exc_info=err)
+                shutil.rmtree(
+                    str(folder),
+                    onerror=lambda func, path, exc: log.error(
+                        "Failed to remove folder %s", path, exc_info=exc
+                    ),
+                )
 
         if set:
             self._repos = ret

tests/cogs/downloader/test_downloader.py

@@ -28,20 +28,6 @@ def test_existing_git_repo(tmpdir):
     assert exists is True
 
 
-@pytest.mark.asyncio
-async def test_clone_repo(repo_norun, capsys):
-    await repo_norun.clone()
-
-    clone_cmd, _ = capsys.readouterr()
-    clone_cmd = clone_cmd.strip("[']\n").split("', '")
-    assert clone_cmd[0] == "git"
-    assert clone_cmd[1] == "clone"
-    assert clone_cmd[2] == "-b"
-    assert clone_cmd[3] == "rewrite_cogs"
-    assert clone_cmd[4] == repo_norun.url
-    assert ("repos", "squid") == pathlib.Path(clone_cmd[5]).parts[-2:]
-
-
 @pytest.mark.asyncio
 async def test_add_repo(monkeypatch, repo_manager):
     monkeypatch.setattr("redbot.cogs.downloader.repo_manager.Repo._run", fake_run_noprint)