From e27682abd3007bdcf78781622773873317c226da Mon Sep 17 00:00:00 2001 From: Toby Harradine Date: Sat, 22 Sep 2018 15:05:42 +1000 Subject: [PATCH] [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 --- redbot/cogs/downloader/__init__.py | 7 ++- redbot/cogs/downloader/downloader.py | 25 ++++---- redbot/cogs/downloader/errors.py | 9 +++ redbot/cogs/downloader/repo_manager.py | 74 +++++++++++++++--------- tests/cogs/downloader/test_downloader.py | 14 ----- 5 files changed, 76 insertions(+), 53 deletions(-) diff --git a/redbot/cogs/downloader/__init__.py b/redbot/cogs/downloader/__init__.py index 85df0fa4a..7c50e3395 100644 --- a/redbot/cogs/downloader/__init__.py +++ b/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) diff --git a/redbot/cogs/downloader/downloader.py b/redbot/cogs/downloader/downloader.py index 95ea14a85..7b459c454 100644 --- a/redbot/cogs/downloader/downloader.py +++ b/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 @@ class Downloader: 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 @@ class Downloader: 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: diff --git a/redbot/cogs/downloader/errors.py b/redbot/cogs/downloader/errors.py index 2b25adc9a..cd8f7405b 100644 --- a/redbot/cogs/downloader/errors.py +++ b/redbot/cogs/downloader/errors.py @@ -9,6 +9,7 @@ __all__ = [ "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. diff --git a/redbot/cogs/downloader/repo_manager.py b/redbot/cogs/downloader/repo_manager.py index d70688fed..538d6c395 100644 --- a/redbot/cogs/downloader/repo_manager.py +++ b/redbot/cogs/downloader/repo_manager.py @@ -2,6 +2,7 @@ import asyncio import functools import os import pkgutil +import shutil import re from concurrent.futures import ThreadPoolExecutor from pathlib import Path @@ -11,19 +12,19 @@ from typing import Tuple, MutableMapping, Union, Optional 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 @@ class Repo(RepoJSONMixin): ) 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 @@ class Repo(RepoJSONMixin): ) 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 @@ class Repo(RepoJSONMixin): """ 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 @@ class Repo(RepoJSONMixin): 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 @@ class Repo(RepoJSONMixin): """ 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 @@ class Repo(RepoJSONMixin): 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,8 +277,9 @@ class Repo(RepoJSONMixin): Raises ------ - RuntimeError + .NoRemoteURL When the folder does not contain a git repo with a FETCH URL. + """ if folder is None: folder = self.folder_path @@ -277,7 +287,7 @@ class Repo(RepoJSONMixin): 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 @@ class Repo(RepoJSONMixin): 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 @@ class Repo(RepoJSONMixin): 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 @@ class Repo(RepoJSONMixin): """ 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 @@ class RepoManager: 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 @@ class RepoManager: @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 @@ class RepoManager: """ 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 @@ class RepoManager: 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 @@ class RepoManager: 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 diff --git a/tests/cogs/downloader/test_downloader.py b/tests/cogs/downloader/test_downloader.py index 10d3b0fc2..c5fb029ad 100644 --- a/tests/cogs/downloader/test_downloader.py +++ b/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)