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)