From d0802abbf3c382d007a32000c8f999a260e4d53f Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 15 Jan 2018 15:51:03 -0500 Subject: [PATCH 01/26] Log `git checkout` and expose to users --- readthedocs/projects/tasks.py | 6 +++--- readthedocs/vcs_support/backends/git.py | 22 ++++++++++++++++------ 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index a670b2a3b3c..0aa33370251 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -335,7 +335,7 @@ def setup_vcs(self): self.setup_env.update_build(state=BUILD_STATE_CLONING) self._log(msg='Updating docs from VCS') - update_imported_docs(self.version.pk) + update_imported_docs(self.version.pk, self.setup_env) commit = self.project.vcs_repo(self.version.slug).commit if commit: self.build['commit'] = commit @@ -550,7 +550,7 @@ def send_notifications(self): @app.task() -def update_imported_docs(version_pk): +def update_imported_docs(version_pk, env=None): """ Check out or update the given project's repository. @@ -593,7 +593,7 @@ def update_imported_docs(version_pk): version_slug = version.slug version_repo = project.vcs_repo(version_slug) - ret_dict['checkout'] = version_repo.checkout(version.identifier) + ret_dict['checkout'] = version_repo.checkout(version.identifier, env) else: # Does this ever get called? log.info(LOG_TEMPLATE.format( diff --git a/readthedocs/vcs_support/backends/git.py b/readthedocs/vcs_support/backends/git.py index a77628bb2f6..9f597708120 100644 --- a/readthedocs/vcs_support/backends/git.py +++ b/readthedocs/vcs_support/backends/git.py @@ -60,19 +60,29 @@ def fetch(self): if code != 0: raise RepositoryError - def checkout_revision(self, revision=None): + def checkout_revision(self, revision=None, env=None): if not revision: branch = self.default_branch or self.fallback_branch revision = 'origin/%s' % branch - code, out, err = self.run('git', 'checkout', - '--force', '--quiet', revision) + if env: + build_cmd = env.run( + 'git', 'checkout', + '--force', '--quiet', revision, + cwd=self.working_dir, + ) + code, out, err = build_cmd.exit_code, build_cmd.output, build_cmd.error + else: + code, out, err = self.run('git', 'checkout', + '--force', '--quiet', revision) if code != 0: log.warning("Failed to checkout revision '%s': %s", revision, code) return [code, out, err] - def clone(self): + def clone(self, env=None): + + # TODO: use env if it exists code, _, _ = self.run('git', 'clone', '--recursive', '--quiet', self.repo_url, '.') if code != 0: @@ -161,7 +171,7 @@ def commit(self): _, stdout, _ = self.run('git', 'rev-parse', 'HEAD') return stdout.strip() - def checkout(self, identifier=None): + def checkout(self, identifier=None, env=None): self.check_working_dir() # Clone or update repository @@ -179,7 +189,7 @@ def checkout(self, identifier=None): identifier = self.find_ref(identifier) # Checkout the correct identifier for this branch. - code, out, err = self.checkout_revision(identifier) + code, out, err = self.checkout_revision(identifier, env) if code != 0: return code, out, err From e889166200139800e3a2776094d5af0689d6656d Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Mon, 15 Jan 2018 16:08:49 -0500 Subject: [PATCH 02/26] Use `setup_env` and record for `git clone` --- readthedocs/vcs_support/backends/git.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/readthedocs/vcs_support/backends/git.py b/readthedocs/vcs_support/backends/git.py index 9f597708120..0bd1a10cd48 100644 --- a/readthedocs/vcs_support/backends/git.py +++ b/readthedocs/vcs_support/backends/git.py @@ -81,10 +81,16 @@ def checkout_revision(self, revision=None, env=None): return [code, out, err] def clone(self, env=None): - - # TODO: use env if it exists - code, _, _ = self.run('git', 'clone', '--recursive', '--quiet', - self.repo_url, '.') + if env: + build_cmd = env.run( + 'git', 'clone', '--recursive', '--quiet', + self.repo_url, '.', + cwd=self.working_dir, + ) + code, out, err = build_cmd.exit_code, build_cmd.output, build_cmd.error + else: + code, _, _ = self.run('git', 'clone', '--recursive', '--quiet', + self.repo_url, '.') if code != 0: raise RepositoryError @@ -180,7 +186,7 @@ def checkout(self, identifier=None, env=None): self.fetch() else: self.make_clean_working_dir() - self.clone() + self.clone(env) # Find proper identifier if not identifier: From 19f5ba3c63cfcf641fe92ab1b641fd68837425d4 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 18 Jan 2018 09:26:27 -0500 Subject: [PATCH 03/26] Use LocalEnvironment to record VCS commands in the build --- readthedocs/projects/models.py | 20 +++-- readthedocs/projects/tasks.py | 94 +++++++++++++++++----- readthedocs/rtd_tests/tests/test_celery.py | 6 +- readthedocs/vcs_support/base.py | 89 ++++++++------------ 4 files changed, 127 insertions(+), 82 deletions(-) diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index f346f2fc984..5cfc0a344bf 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -33,7 +33,6 @@ determine_stable_version, version_windows) from readthedocs.restapi.client import api from readthedocs.vcs_support.backends import backend_cls -from readthedocs.vcs_support.base import VCSProject from readthedocs.vcs_support.utils import Lock, NonBlockingLock log = logging.getLogger(__name__) @@ -605,14 +604,25 @@ def has_htmlzip(self, version_slug=LATEST): def sponsored(self): return False - def vcs_repo(self, version=LATEST): + def vcs_repo(self, version=LATEST, environment=None): + """ + Return a Backend object for this project able to handle VCS commands. + + :param environment: environment to run the commands + :type environment: doc_builder.environments.BuildEnvironment + :param version: version slug for the backend (``LATEST`` by default) + :type version: str + """ + + # TODO: this seems to be the only method that receives a + # ``version.slug`` instead of a ``Version`` instance (I prefer an + # instance here) + backend = backend_cls.get(self.repo_type) if not backend: repo = None else: - proj = VCSProject( - self.name, self.default_branch, self.checkout_path(version), self.clean_repo) - repo = backend(proj, version) + repo = backend(self, version, environment) return repo def repo_nonblockinglock(self, version, max_lock_age=5): diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index 0aa33370251..bc049a56460 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -68,22 +68,16 @@ class UpdateDocsTask(Task): """ The main entry point for updating documentation. - It handles all of the logic around whether a project is imported or we - created it. Then it will build the html docs and other requested parts. - - `pk` - Primary key of the project to update - - `record` - Whether or not to keep a record of the update in the database. Useful - for preventing changes visible to the end-user when running commands - from the shell, for example. + It handles all of the logic around whether a project is imported, we created + it or a webhook is received. Then it will sync the repository and build the + html docs if needed. """ max_retries = 5 default_retry_delay = (7 * 60) name = __name__ + '.update_docs' + # TODO: the argument from the __init__ are used only in tests def __init__(self, build_env=None, python_env=None, config=None, force=False, search=True, localmedia=True, build=None, project=None, version=None): @@ -114,7 +108,7 @@ def _log(self, msg): def run(self, pk, version_pk=None, build_pk=None, record=True, docker=False, search=True, force=False, localmedia=True, **__): """ - Run a documentation build. + Run a documentation sync or sync n' build. This is fully wrapped in exception handling to account for a number of failure cases. We first run a few commands in a local build environment, @@ -129,15 +123,16 @@ def run(self, pk, version_pk=None, build_pk=None, record=True, the user to bug us. It is therefore a benefit to have as few unhandled errors as possible. + :param pk int: Project id - :param version_pk int: Project Version id - :param build_pk int: Build id + :param version_pk int: Project Version id (latest if None) + :param build_pk int: Build id (if None, commands are not recorded) :param record bool: record a build object in the database :param docker bool: use docker to build the project :param search bool: update search :param force bool: force Sphinx build - :param localmedia: update localmedia - :returns: if build was successful or not + :param localmedia bool: update localmedia + :returns: whether build was successful or not :rtype: bool """ try: @@ -267,7 +262,7 @@ def run_build(self, docker=False, record=True): config=self.config) try: - self.setup_environment() + self.setup_python_environment() # TODO the build object should have an idea of these states, extend # the model to include an idea of these outcomes @@ -335,11 +330,74 @@ def setup_vcs(self): self.setup_env.update_build(state=BUILD_STATE_CLONING) self._log(msg='Updating docs from VCS') - update_imported_docs(self.version.pk, self.setup_env) + self.sync_repo() commit = self.project.vcs_repo(self.version.slug).commit if commit: self.build['commit'] = commit + def sync_repo(self): + """ + Checkout/update the project's repository and hit ``sync_versions`` API. + """ + # Make Dirs + if not os.path.exists(self.project.doc_path): + os.makedirs(self.project.doc_path) + + if not self.project.vcs_repo(): + raise RepositoryError( + _('Repository type "{repo_type}" unknown').format( + repo_type=self.project.repo_type, + ), + ) + + with self.project.repo_nonblockinglock( + version=self.version, + max_lock_age=getattr(settings, 'REPO_LOCK_SECONDS', 30)): + + # Get the actual code on disk + try: + before_vcs.send(sender=self.version) + self._log( + 'Checking out version {slug}: {identifier}'.format( + slug=self.version.slug, + identifier=self.version.identifier, + ), + ) + version_repo = self.project.vcs_repo( + self.version.slug, + self.setup_env, + ) + version_repo.checkout(self.version.identifier) + finally: + after_vcs.send(sender=self.version) + + # Update tags/version + version_post_data = {'repo': version_repo.repo_url} + + if version_repo.supports_tags: + version_post_data['tags'] = [ + {'identifier': v.identifier, + 'verbose_name': v.verbose_name, + } for v in version_repo.tags + ] + + if version_repo.supports_branches: + version_post_data['branches'] = [ + {'identifier': v.identifier, + 'verbose_name': v.verbose_name, + } for v in version_repo.branches + ] + + try: + # Hit the API ``sync_versions`` which may trigger a new build + # for the stable version + api_v2.project(self.project.pk).sync_versions.post(version_post_data) + except HttpClientError: + log.exception('Sync Versions Exception') + except Exception: + log.exception('Unknown Sync Versions Exception') + + def get_env_vars(self): """Get bash environment variables used for all builder commands.""" env = { @@ -420,7 +478,7 @@ def update_app_instances(self, html=False, localmedia=False, search=False, callback=sync_callback.s(version_pk=self.version.pk, commit=self.build['commit']), ) - def setup_environment(self): + def setup_python_environment(self): """ Build the virtualenv and install the project into it. diff --git a/readthedocs/rtd_tests/tests/test_celery.py b/readthedocs/rtd_tests/tests/test_celery.py index 49db6b96dd0..e62ea570581 100644 --- a/readthedocs/rtd_tests/tests/test_celery.py +++ b/readthedocs/rtd_tests/tests/test_celery.py @@ -64,7 +64,7 @@ def test_clear_artifacts(self): self.assertTrue(result.successful()) self.assertFalse(exists(directory)) - @patch('readthedocs.projects.tasks.UpdateDocsTask.setup_environment', new=MagicMock) + @patch('readthedocs.projects.tasks.UpdateDocsTask.setup_python_environment', new=MagicMock) @patch('readthedocs.projects.tasks.UpdateDocsTask.build_docs', new=MagicMock) @patch('readthedocs.projects.tasks.UpdateDocsTask.setup_vcs', new=MagicMock) def test_update_docs(self): @@ -79,7 +79,7 @@ def test_update_docs(self): intersphinx=False) self.assertTrue(result.successful()) - @patch('readthedocs.projects.tasks.UpdateDocsTask.setup_environment', new=MagicMock) + @patch('readthedocs.projects.tasks.UpdateDocsTask.setup_python_environment', new=MagicMock) @patch('readthedocs.projects.tasks.UpdateDocsTask.build_docs', new=MagicMock) @patch('readthedocs.doc_builder.environments.BuildEnvironment.update_build', new=MagicMock) @patch('readthedocs.projects.tasks.UpdateDocsTask.setup_vcs') @@ -97,7 +97,7 @@ def test_update_docs_unexpected_setup_exception(self, mock_setup_vcs): intersphinx=False) self.assertTrue(result.successful()) - @patch('readthedocs.projects.tasks.UpdateDocsTask.setup_environment', new=MagicMock) + @patch('readthedocs.projects.tasks.UpdateDocsTask.setup_python_environment', new=MagicMock) @patch('readthedocs.projects.tasks.UpdateDocsTask.setup_vcs', new=MagicMock) @patch('readthedocs.doc_builder.environments.BuildEnvironment.update_build', new=MagicMock) @patch('readthedocs.projects.tasks.UpdateDocsTask.build_docs') diff --git a/readthedocs/vcs_support/base.py b/readthedocs/vcs_support/base.py index 3416de0ff11..ff81a4aef25 100644 --- a/readthedocs/vcs_support/base.py +++ b/readthedocs/vcs_support/base.py @@ -1,12 +1,11 @@ +# -*- coding: utf-8 -*- + """Base classes for VCS backends.""" from __future__ import absolute_import from builtins import object import logging import os import shutil -import subprocess -from collections import namedtuple -from os.path import basename log = logging.getLogger(__name__) @@ -33,60 +32,12 @@ def __repr__(self): self.verbose_name) -class VCSProject(namedtuple("VCSProject", - "name default_branch working_dir repo_url")): - - """Transient object to encapsulate a projects stuff""" - - pass - - -class BaseCLI(object): - - """Helper class for CLI-heavy classes.""" - - log_tmpl = u'VCS[{name}:{ident}]: {args}' - - def __call__(self, *args): - return self.run(args) - - def run(self, *args): - """:param bits: list of command and args. See `subprocess` docs""" - process = subprocess.Popen(args, stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - cwd=self.working_dir, shell=False, - env=self.env) - try: - log.info(self.log_tmpl.format(ident=basename(self.working_dir), - name=self.name, - args=' '.join(args))) - except UnicodeDecodeError: - # >:x - pass - stdout, stderr = process.communicate() - try: - log.info(self.log_tmpl.format(ident=basename(self.working_dir), - name=self.name, - args=stdout)) - except UnicodeDecodeError: - # >:x - pass - return ( - process.returncode, - stdout.decode('utf-8'), - stderr.decode('utf-8')) - - @property - def env(self): - return os.environ.copy() - - -class BaseVCS(BaseCLI): +class BaseVCS(object): """ Base for VCS Classes. - Built on top of the BaseCLI. + VCS commands are ran inside a ``LocalEnvironment``. """ supports_tags = False # Whether this VCS supports tags or not. @@ -98,11 +49,27 @@ class BaseVCS(BaseCLI): # Defining a base API, so we'll have unused args # pylint: disable=unused-argument - def __init__(self, project, version, **kwargs): + def __init__(self, project, version_slug, environment=None, **kwargs): self.default_branch = project.default_branch self.name = project.name - self.repo_url = project.repo_url - self.working_dir = project.working_dir + self.repo_url = project.clean_repo + self.working_dir = project.checkout_path(version_slug) + + if environment: + self.environment = environment + else: + from readthedocs.doc_builder.environments import LocalEnvironment + + try: + # TODO: it's supposed that we will always have the `latest` + # version on a Project since it's created at the Project.save() + # method, but there are a couple of tests that have inconsistent + # data + version = project.versions.get(slug=version_slug) + self.environment = LocalEnvironment( + project, version, record=False, update_on_success=False) + except Exception: + log.exception('Project has no version: %s', version_slug) def check_working_dir(self): if not os.path.exists(self.working_dir): @@ -122,6 +89,16 @@ def update(self): """ self.check_working_dir() + def run(self, *cmd, **kwargs): + kwargs.update({ + 'cwd': self.working_dir, + 'shell': False, + }) + + build_cmd = self.environment.run(*cmd, **kwargs) + # Return a tuple to keep compatibility + return (build_cmd.exit_code, build_cmd.output, build_cmd.error) + # ========================================================================= # Tag / Branch related methods # These methods only apply if supports_tags = True and/or From e807a247608e405876e12d58a203c71eb7e95925 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 18 Jan 2018 11:58:50 -0500 Subject: [PATCH 04/26] Split LocalEnvironment into LocalBuildEnvironment and LocalEnvironment --- readthedocs/doc_builder/environments.py | 116 +++++++++++------- readthedocs/projects/tasks.py | 10 +- readthedocs/rtd_tests/tests/test_builds.py | 18 +-- .../rtd_tests/tests/test_doc_building.py | 44 +++---- readthedocs/vcs_support/base.py | 19 +-- 5 files changed, 111 insertions(+), 96 deletions(-) diff --git a/readthedocs/doc_builder/environments.py b/readthedocs/doc_builder/environments.py index 7995de160de..a3095d19e40 100644 --- a/readthedocs/doc_builder/environments.py +++ b/readthedocs/doc_builder/environments.py @@ -40,7 +40,7 @@ __all__ = ( 'api_v2', 'BuildCommand', 'DockerBuildCommand', - 'BuildEnvironment', 'LocalEnvironment', 'DockerEnvironment', + 'BuildEnvironment', 'LocalBuildEnvironment', 'DockerBuildEnvironment', ) @@ -268,7 +268,65 @@ def get_wrapped_command(self): for part in self.command])))) -class BuildEnvironment(object): +class BaseEnvironment(object): + + # TODO: BuildCommand name doesn't make sense here, should be just Command + command_class = BuildCommand + + def __init__(self, project=None): + # TODO: maybe we can remove this Project dependency also + self.project = project + + def record_command(self, command): + pass + + def run(self, *cmd, **kwargs): + """Shortcut to run command from environment.""" + return self.run_command_class(cls=self.command_class, cmd=cmd, **kwargs) + + def run_command_class(self, cls, cmd, warn_only=False, **kwargs): + """ + Run command from this environment. + + Use ``cls`` to instantiate a command + + :param warn_only: Don't raise an exception on command failure + """ + # Remove PATH from env, and set it to bin_path if it isn't passed in + env_path = self.environment.pop('BIN_PATH', None) + if 'bin_path' not in kwargs and env_path: + kwargs['bin_path'] = env_path + assert 'environment' not in kwargs, "environment can't be passed in via commands." + kwargs['environment'] = self.environment + + # TODO: ``build_env`` is passed as ``kwargs`` when it's called from a ``*BuildEnvironment`` + # kwargs['build_env'] = self + + build_cmd = cls(cmd, **kwargs) + self.commands.append(build_cmd) + build_cmd.run() + + # TODO: I don't like how it's handled this entry point here + self.record_command(build_cmd) + + if build_cmd.failed: + msg = u'Command {cmd} failed'.format(cmd=build_cmd.get_command()) + + if build_cmd.output: + msg += u':\n{out}'.format(out=build_cmd.output) + + if warn_only: + # TODO: remove version dependency to log here + log.warning(LOG_TEMPLATE + .format(project=self.project.slug, + version=self.version.slug, + msg=msg)) + else: + raise BuildEnvironmentWarning(msg) + return build_cmd + + +class BuildEnvironment(BaseEnvironment): """ Base build environment. @@ -349,47 +407,17 @@ def handle_exception(self, exc_type, exc_value, _): return True def run(self, *cmd, **kwargs): - """Shortcut to run command from environment.""" - return self.run_command_class(cls=self.command_class, cmd=cmd, **kwargs) - - def run_command_class(self, cls, cmd, **kwargs): - """ - Run command from this environment. + kwargs.update({ + 'build_env': self, + }) + return super(BuildEnvironment, self).run(*cmd, **kwargs) - Use ``cls`` to instantiate a command + def run_command_class(self, *cmd, **kwargs): + kwargs.update({ + 'build_env': self, + }) + return super(BuildEnvironment, self).run_command_class(*cmd, **kwargs) - :param warn_only: Don't raise an exception on command failure - """ - warn_only = kwargs.pop('warn_only', False) - # Remove PATH from env, and set it to bin_path if it isn't passed in - env_path = self.environment.pop('BIN_PATH', None) - if 'bin_path' not in kwargs and env_path: - kwargs['bin_path'] = env_path - assert 'environment' not in kwargs, "environment can't be passed in via commands." - kwargs['environment'] = self.environment - kwargs['build_env'] = self - build_cmd = cls(cmd, **kwargs) - self.commands.append(build_cmd) - build_cmd.run() - - # Save to database - if self.record: - build_cmd.save() - - if build_cmd.failed: - msg = u'Command {cmd} failed'.format(cmd=build_cmd.get_command()) - - if build_cmd.output: - msg += u':\n{out}'.format(out=build_cmd.output) - - if warn_only: - log.warning(LOG_TEMPLATE - .format(project=self.project.slug, - version=self.version.slug, - msg=msg)) - else: - raise BuildEnvironmentWarning(msg) - return build_cmd @property def successful(self): @@ -497,14 +525,14 @@ def update_build(self, state=None): log.exception("Unknown build exception") -class LocalEnvironment(BuildEnvironment): +class LocalBuildEnvironment(BuildEnvironment): """Local execution environment.""" command_class = BuildCommand -class DockerEnvironment(BuildEnvironment): +class DockerBuildEnvironment(BuildEnvironment): """ Docker build environment, uses docker to contain builds. @@ -527,7 +555,7 @@ class DockerEnvironment(BuildEnvironment): def __init__(self, *args, **kwargs): self.docker_socket = kwargs.pop('docker_socket', DOCKER_SOCKET) - super(DockerEnvironment, self).__init__(*args, **kwargs) + super(DockerBuildEnvironment, self).__init__(*args, **kwargs) self.client = None self.container = None self.container_name = slugify( diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index bc049a56460..1ce2580f08a 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -45,8 +45,8 @@ from readthedocs.core.utils import send_email, broadcast from readthedocs.doc_builder.config import load_yaml_config from readthedocs.doc_builder.constants import DOCKER_LIMITS -from readthedocs.doc_builder.environments import (LocalEnvironment, - DockerEnvironment) +from readthedocs.doc_builder.environments import (LocalBuildEnvironment, + DockerBuildEnvironment) from readthedocs.doc_builder.exceptions import BuildEnvironmentError from readthedocs.doc_builder.loader import get_builder_class from readthedocs.doc_builder.python_environments import Virtualenv, Conda @@ -186,7 +186,7 @@ def run_setup(self, record=True): Return True if successful. """ - self.setup_env = LocalEnvironment( + self.setup_env = LocalBuildEnvironment( project=self.project, version=self.version, build=self.build, @@ -241,9 +241,9 @@ def run_build(self, docker=False, record=True): env_vars = self.get_env_vars() if docker or settings.DOCKER_ENABLE: - env_cls = DockerEnvironment + env_cls = DockerBuildEnvironment else: - env_cls = LocalEnvironment + env_cls = LocalBuildEnvironment self.build_env = env_cls(project=self.project, version=self.version, config=self.config, build=self.build, record=record, environment=env_vars) diff --git a/readthedocs/rtd_tests/tests/test_builds.py b/readthedocs/rtd_tests/tests/test_builds.py index 19f383aa3e8..b53cd80b7ad 100644 --- a/readthedocs/rtd_tests/tests/test_builds.py +++ b/readthedocs/rtd_tests/tests/test_builds.py @@ -9,7 +9,7 @@ from readthedocs.projects.models import Project from readthedocs.doc_builder.config import ConfigWrapper -from readthedocs.doc_builder.environments import LocalEnvironment +from readthedocs.doc_builder.environments import LocalBuildEnvironment from readthedocs.doc_builder.python_environments import Virtualenv from readthedocs.doc_builder.loader import get_builder_class from readthedocs.projects.tasks import UpdateDocsTask @@ -41,7 +41,7 @@ def test_build(self): }) self.mocks.patches['html_build'].stop() - build_env = LocalEnvironment(project=project, version=version, build={}) + build_env = LocalBuildEnvironment(project=project, version=version, build={}) python_env = Virtualenv(version=version, build_env=build_env) config = ConfigWrapper(version=version, yaml_config=create_load()()[0]) task = UpdateDocsTask(build_env=build_env, project=project, python_env=python_env, @@ -65,7 +65,7 @@ def test_build_respects_pdf_flag(self): versions=[fixture()]) version = project.versions.all()[0] - build_env = LocalEnvironment(project=project, version=version, build={}) + build_env = LocalBuildEnvironment(project=project, version=version, build={}) python_env = Virtualenv(version=version, build_env=build_env) config = ConfigWrapper(version=version, yaml_config=create_load()()[0]) task = UpdateDocsTask(build_env=build_env, project=project, python_env=python_env, @@ -90,7 +90,7 @@ def test_build_respects_epub_flag(self): versions=[fixture()]) version = project.versions.all()[0] - build_env = LocalEnvironment(project=project, version=version, build={}) + build_env = LocalBuildEnvironment(project=project, version=version, build={}) python_env = Virtualenv(version=version, build_env=build_env) config = ConfigWrapper(version=version, yaml_config=create_load()()[0]) task = UpdateDocsTask(build_env=build_env, project=project, python_env=python_env, @@ -114,7 +114,7 @@ def test_build_respects_yaml(self): versions=[fixture()]) version = project.versions.all()[0] - build_env = LocalEnvironment(project=project, version=version, build={}) + build_env = LocalBuildEnvironment(project=project, version=version, build={}) python_env = Virtualenv(version=version, build_env=build_env) config = ConfigWrapper(version=version, yaml_config=create_load({ 'formats': ['epub'] @@ -136,7 +136,7 @@ def test_builder_comments(self): allow_comments=True, versions=[fixture()]) version = project.versions.all()[0] - build_env = LocalEnvironment(version=version, project=project, build={}) + build_env = LocalBuildEnvironment(version=version, project=project, build={}) python_env = Virtualenv(version=version, build_env=build_env) builder_class = get_builder_class(project.documentation_type) builder = builder_class(build_env, python_env) @@ -149,7 +149,7 @@ def test_builder_no_comments(self): allow_comments=False, versions=[fixture()]) version = project.versions.all()[0] - build_env = LocalEnvironment(version=version, project=project, build={}) + build_env = LocalBuildEnvironment(version=version, project=project, build={}) python_env = Virtualenv(version=version, build_env=build_env) builder_class = get_builder_class(project.documentation_type) builder = builder_class(build_env, python_env) @@ -171,7 +171,7 @@ def test_build_pdf_latex_failures(self): version = project.versions.all()[0] assert project.conf_dir() == '/tmp/rtd' - build_env = LocalEnvironment(project=project, version=version, build={}) + build_env = LocalBuildEnvironment(project=project, version=version, build={}) python_env = Virtualenv(version=version, build_env=build_env) config = ConfigWrapper(version=version, yaml_config=create_load()()[0]) task = UpdateDocsTask(build_env=build_env, project=project, python_env=python_env, @@ -213,7 +213,7 @@ def test_build_pdf_latex_not_failure(self): version = project.versions.all()[0] assert project.conf_dir() == '/tmp/rtd' - build_env = LocalEnvironment(project=project, version=version, build={}) + build_env = LocalBuildEnvironment(project=project, version=version, build={}) python_env = Virtualenv(version=version, build_env=build_env) config = ConfigWrapper(version=version, yaml_config=create_load()()[0]) task = UpdateDocsTask(build_env=build_env, project=project, python_env=python_env, diff --git a/readthedocs/rtd_tests/tests/test_doc_building.py b/readthedocs/rtd_tests/tests/test_doc_building.py index 8e928d27dae..880eb5d4397 100644 --- a/readthedocs/rtd_tests/tests/test_doc_building.py +++ b/readthedocs/rtd_tests/tests/test_doc_building.py @@ -24,7 +24,7 @@ from readthedocs.builds.models import Version from readthedocs.doc_builder.config import ConfigWrapper from readthedocs.doc_builder.environments import ( - BuildCommand, DockerBuildCommand, DockerEnvironment, LocalEnvironment) + BuildCommand, DockerBuildCommand, DockerBuildEnvironment, LocalBuildEnvironment) from readthedocs.doc_builder.exceptions import BuildEnvironmentError from readthedocs.doc_builder.python_environments import Virtualenv from readthedocs.projects.models import Project @@ -36,7 +36,7 @@ SAMPLE_UTF8_BYTES = SAMPLE_UNICODE.encode('utf-8') -class TestLocalEnvironment(TestCase): +class TestLocalBuildEnvironment(TestCase): """Test execution and exception handling in environment.""" fixtures = ['test_data'] @@ -58,7 +58,7 @@ def test_normal_execution(self): }) type(self.mocks.process).returncode = PropertyMock(return_value=0) - build_env = LocalEnvironment( + build_env = LocalBuildEnvironment( version=self.version, project=self.project, build={'id': DUMMY_BUILD_ID}, @@ -92,12 +92,12 @@ def test_normal_execution(self): def test_incremental_state_update_with_no_update(self): """Build updates to a non-finished state when update_on_success=True.""" build_envs = [ - LocalEnvironment( + LocalBuildEnvironment( version=self.version, project=self.project, build={'id': DUMMY_BUILD_ID}, ), - LocalEnvironment( + LocalBuildEnvironment( version=self.version, project=self.project, build={'id': DUMMY_BUILD_ID}, @@ -130,7 +130,7 @@ def test_failing_execution(self): }) type(self.mocks.process).returncode = PropertyMock(return_value=1) - build_env = LocalEnvironment( + build_env = LocalBuildEnvironment( version=self.version, project=self.project, build={'id': DUMMY_BUILD_ID}, @@ -164,7 +164,7 @@ def test_failing_execution(self): def test_failing_execution_with_caught_exception(self): """Build in failing state with BuildEnvironmentError exception.""" - build_env = LocalEnvironment( + build_env = LocalBuildEnvironment( version=self.version, project=self.project, build={'id': DUMMY_BUILD_ID}, @@ -197,7 +197,7 @@ def test_failing_execution_with_caught_exception(self): def test_failing_execution_with_unexpected_exception(self): """Build in failing state with exception from code.""" - build_env = LocalEnvironment( + build_env = LocalBuildEnvironment( version=self.version, project=self.project, build={'id': DUMMY_BUILD_ID}, @@ -230,7 +230,7 @@ def test_failing_execution_with_unexpected_exception(self): }) -class TestDockerEnvironment(TestCase): +class TestDockerBuildEnvironment(TestCase): """Test docker build environment.""" @@ -248,7 +248,7 @@ def tearDown(self): def test_container_id(self): """Test docker build command.""" - docker = DockerEnvironment( + docker = DockerBuildEnvironment( version=self.version, project=self.project, build={'id': DUMMY_BUILD_ID}, @@ -257,7 +257,7 @@ def test_container_id(self): def test_environment_successful_build(self): """A successful build exits cleanly and reports the build output.""" - build_env = DockerEnvironment( + build_env = DockerBuildEnvironment( version=self.version, project=self.project, build={'id': DUMMY_BUILD_ID}, @@ -286,7 +286,7 @@ def test_environment_successful_build(self): def test_environment_successful_build_without_update(self): """A successful build exits cleanly and doesn't update build.""" - build_env = DockerEnvironment( + build_env = DockerBuildEnvironment( version=self.version, project=self.project, build={'id': DUMMY_BUILD_ID}, @@ -304,7 +304,7 @@ def test_environment_successful_build_without_update(self): def test_environment_failed_build_without_update_but_with_error(self): """A failed build exits cleanly and doesn't update build.""" - build_env = DockerEnvironment( + build_env = DockerBuildEnvironment( version=self.version, project=self.project, build={'id': DUMMY_BUILD_ID}, @@ -336,7 +336,7 @@ def test_environment_failed_build_without_update_but_with_error(self): def test_connection_failure(self): """Connection failure on to docker socket should raise exception.""" self.mocks.configure_mock('docker', {'side_effect': DockerException}) - build_env = DockerEnvironment( + build_env = DockerBuildEnvironment( version=self.version, project=self.project, build={'id': DUMMY_BUILD_ID}, @@ -379,7 +379,7 @@ def test_api_failure(self): 'Failure creating container') }) - build_env = DockerEnvironment( + build_env = DockerBuildEnvironment( version=self.version, project=self.project, build={'id': DUMMY_BUILD_ID}, @@ -418,7 +418,7 @@ def test_api_failure_on_docker_memory_limit(self): 'Failure creating container'), }) - build_env = DockerEnvironment( + build_env = DockerBuildEnvironment( version=self.version, project=self.project, build={'id': DUMMY_BUILD_ID}, @@ -454,7 +454,7 @@ def test_api_failure_on_error_in_exit(self): 'kill.side_effect': BuildEnvironmentError('Failed') }) - build_env = DockerEnvironment( + build_env = DockerBuildEnvironment( version=self.version, project=self.project, build={'id': DUMMY_BUILD_ID}, @@ -492,7 +492,7 @@ def test_api_failure_returns_previous_error_on_error_in_exit(self): 'kill.side_effect': BuildEnvironmentError('Outer failed') }) - build_env = DockerEnvironment( + build_env = DockerBuildEnvironment( version=self.version, project=self.project, build={'id': DUMMY_BUILD_ID}, @@ -527,7 +527,7 @@ def test_command_execution(self): 'exec_inspect.return_value': {'ExitCode': 1}, }) - build_env = DockerEnvironment( + build_env = DockerBuildEnvironment( version=self.version, project=self.project, build={'id': DUMMY_BUILD_ID}, @@ -576,7 +576,7 @@ def test_command_execution_cleanup_exception(self): ) }) - build_env = DockerEnvironment( + build_env = DockerBuildEnvironment( version=self.version, project=self.project, build={'id': DUMMY_BUILD_ID}, @@ -615,7 +615,7 @@ def test_container_already_exists(self): 'exec_inspect.return_value': {'ExitCode': 0}, }) - build_env = DockerEnvironment( + build_env = DockerBuildEnvironment( version=self.version, project=self.project, build={'id': DUMMY_BUILD_ID}, @@ -667,7 +667,7 @@ def test_container_timeout(self): 'exec_inspect.return_value': {'ExitCode': 0}, }) - build_env = DockerEnvironment( + build_env = DockerBuildEnvironment( version=self.version, project=self.project, build={'id': DUMMY_BUILD_ID}, diff --git a/readthedocs/vcs_support/base.py b/readthedocs/vcs_support/base.py index ff81a4aef25..594f408d79e 100644 --- a/readthedocs/vcs_support/base.py +++ b/readthedocs/vcs_support/base.py @@ -37,7 +37,7 @@ class BaseVCS(object): """ Base for VCS Classes. - VCS commands are ran inside a ``LocalEnvironment``. + VCS commands are ran inside a ``LocalBuildEnvironment``. """ supports_tags = False # Whether this VCS supports tags or not. @@ -55,21 +55,8 @@ def __init__(self, project, version_slug, environment=None, **kwargs): self.repo_url = project.clean_repo self.working_dir = project.checkout_path(version_slug) - if environment: - self.environment = environment - else: - from readthedocs.doc_builder.environments import LocalEnvironment - - try: - # TODO: it's supposed that we will always have the `latest` - # version on a Project since it's created at the Project.save() - # method, but there are a couple of tests that have inconsistent - # data - version = project.versions.get(slug=version_slug) - self.environment = LocalEnvironment( - project, version, record=False, update_on_success=False) - except Exception: - log.exception('Project has no version: %s', version_slug) + from readthedocs.doc_builder.environments import LocalEnvironment + self.environment = environment or LocalEnvironment(project) def check_working_dir(self): if not os.path.exists(self.working_dir): From fdca1f58c4c0ea2e702f08a95a03de985e71dee5 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 18 Jan 2018 12:00:56 -0500 Subject: [PATCH 05/26] LocalEnvironment --- readthedocs/doc_builder/environments.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/readthedocs/doc_builder/environments.py b/readthedocs/doc_builder/environments.py index a3095d19e40..5ff2b2f9fba 100644 --- a/readthedocs/doc_builder/environments.py +++ b/readthedocs/doc_builder/environments.py @@ -270,9 +270,6 @@ def get_wrapped_command(self): class BaseEnvironment(object): - # TODO: BuildCommand name doesn't make sense here, should be just Command - command_class = BuildCommand - def __init__(self, project=None): # TODO: maybe we can remove this Project dependency also self.project = project @@ -326,6 +323,12 @@ def run_command_class(self, cls, cmd, warn_only=False, **kwargs): return build_cmd +class LocalEnvironment(BaseEnvironment): + + # TODO: BuildCommand name doesn't make sense here, should be just Command + command_class = BuildCommand + + class BuildEnvironment(BaseEnvironment): """ From ae7ad7939bddfde002d9212468867ee3e2116e1a Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 18 Jan 2018 12:07:50 -0500 Subject: [PATCH 06/26] env fix --- readthedocs/doc_builder/environments.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/readthedocs/doc_builder/environments.py b/readthedocs/doc_builder/environments.py index 5ff2b2f9fba..bb62e179840 100644 --- a/readthedocs/doc_builder/environments.py +++ b/readthedocs/doc_builder/environments.py @@ -270,9 +270,10 @@ def get_wrapped_command(self): class BaseEnvironment(object): - def __init__(self, project=None): + def __init__(self, project, environment=None): # TODO: maybe we can remove this Project dependency also self.project = project + self.environment = environment or {} def record_command(self, command): pass From ea547d8b44dfdcd2513c08424b5af3e0b8552b38 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 18 Jan 2018 12:20:42 -0500 Subject: [PATCH 07/26] Run VCS command in the proper environment --- readthedocs/vcs_support/backends/git.py | 36 ++++++++----------------- readthedocs/vcs_support/base.py | 1 + 2 files changed, 12 insertions(+), 25 deletions(-) diff --git a/readthedocs/vcs_support/backends/git.py b/readthedocs/vcs_support/backends/git.py index 0bd1a10cd48..dc47a892418 100644 --- a/readthedocs/vcs_support/backends/git.py +++ b/readthedocs/vcs_support/backends/git.py @@ -60,37 +60,23 @@ def fetch(self): if code != 0: raise RepositoryError - def checkout_revision(self, revision=None, env=None): + def checkout_revision(self, revision=None): if not revision: branch = self.default_branch or self.fallback_branch revision = 'origin/%s' % branch - if env: - build_cmd = env.run( - 'git', 'checkout', - '--force', '--quiet', revision, - cwd=self.working_dir, - ) - code, out, err = build_cmd.exit_code, build_cmd.output, build_cmd.error - else: - code, out, err = self.run('git', 'checkout', - '--force', '--quiet', revision) + command = self.run('git', 'checkout', + '--force', '--quiet', revision) + code, out, err = command.exit_code, command.output, command.error if code != 0: log.warning("Failed to checkout revision '%s': %s", revision, code) return [code, out, err] - def clone(self, env=None): - if env: - build_cmd = env.run( - 'git', 'clone', '--recursive', '--quiet', - self.repo_url, '.', - cwd=self.working_dir, - ) - code, out, err = build_cmd.exit_code, build_cmd.output, build_cmd.error - else: - code, _, _ = self.run('git', 'clone', '--recursive', '--quiet', - self.repo_url, '.') + def clone(self): + command = self.run('git', 'clone', '--recursive', '--quiet', + self.repo_url, '.') + code, _, _ = command.exit_code, command.output, command.error if code != 0: raise RepositoryError @@ -177,7 +163,7 @@ def commit(self): _, stdout, _ = self.run('git', 'rev-parse', 'HEAD') return stdout.strip() - def checkout(self, identifier=None, env=None): + def checkout(self, identifier=None): self.check_working_dir() # Clone or update repository @@ -186,7 +172,7 @@ def checkout(self, identifier=None, env=None): self.fetch() else: self.make_clean_working_dir() - self.clone(env) + self.clone() # Find proper identifier if not identifier: @@ -195,7 +181,7 @@ def checkout(self, identifier=None, env=None): identifier = self.find_ref(identifier) # Checkout the correct identifier for this branch. - code, out, err = self.checkout_revision(identifier, env) + code, out, err = self.checkout_revision(identifier) if code != 0: return code, out, err diff --git a/readthedocs/vcs_support/base.py b/readthedocs/vcs_support/base.py index 594f408d79e..79ab8691b60 100644 --- a/readthedocs/vcs_support/base.py +++ b/readthedocs/vcs_support/base.py @@ -80,6 +80,7 @@ def run(self, *cmd, **kwargs): kwargs.update({ 'cwd': self.working_dir, 'shell': False, + 'environment': self.env, }) build_cmd = self.environment.run(*cmd, **kwargs) From 6247906f5599ad50964b1d40d4c5a36fdaf403e2 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 18 Jan 2018 12:22:03 -0500 Subject: [PATCH 08/26] Define env --- readthedocs/vcs_support/base.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/readthedocs/vcs_support/base.py b/readthedocs/vcs_support/base.py index 79ab8691b60..cf740f3aa04 100644 --- a/readthedocs/vcs_support/base.py +++ b/readthedocs/vcs_support/base.py @@ -67,6 +67,10 @@ def make_clean_working_dir(self): shutil.rmtree(self.working_dir, ignore_errors=True) self.check_working_dir() + @property + def env(self): + return os.environ.copy() + def update(self): """ Update a local copy of the repository in self.working_dir. From 2d012fd0153effd871823819759641ad290a40c5 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 18 Jan 2018 12:32:30 -0500 Subject: [PATCH 09/26] Proper env variables --- readthedocs/vcs_support/base.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/readthedocs/vcs_support/base.py b/readthedocs/vcs_support/base.py index cf740f3aa04..ec64e312bef 100644 --- a/readthedocs/vcs_support/base.py +++ b/readthedocs/vcs_support/base.py @@ -56,7 +56,8 @@ def __init__(self, project, version_slug, environment=None, **kwargs): self.working_dir = project.checkout_path(version_slug) from readthedocs.doc_builder.environments import LocalEnvironment - self.environment = environment or LocalEnvironment(project) + self.environment = environment or LocalEnvironment( + project, environment=self.env) def check_working_dir(self): if not os.path.exists(self.working_dir): @@ -84,7 +85,6 @@ def run(self, *cmd, **kwargs): kwargs.update({ 'cwd': self.working_dir, 'shell': False, - 'environment': self.env, }) build_cmd = self.environment.run(*cmd, **kwargs) From fd13345970c8b1c8038e82eb5ea8b255e2173a2c Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 18 Jan 2018 12:40:55 -0500 Subject: [PATCH 10/26] Pass proper env variables when running VCS commands --- readthedocs/vcs_support/base.py | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/readthedocs/vcs_support/base.py b/readthedocs/vcs_support/base.py index ec64e312bef..2fbebf2ade1 100644 --- a/readthedocs/vcs_support/base.py +++ b/readthedocs/vcs_support/base.py @@ -56,8 +56,10 @@ def __init__(self, project, version_slug, environment=None, **kwargs): self.working_dir = project.checkout_path(version_slug) from readthedocs.doc_builder.environments import LocalEnvironment - self.environment = environment or LocalEnvironment( - project, environment=self.env) + self.environment = environment or LocalEnvironment(project) + + # Update the env variables with the proper VCS env variables + self.environment.environment.update(self.env) def check_working_dir(self): if not os.path.exists(self.working_dir): @@ -70,7 +72,12 @@ def make_clean_working_dir(self): @property def env(self): - return os.environ.copy() + environment = os.environ.copy() + + # TODO: kind of a hack + del environment['PATH'] + + return environment def update(self): """ From 9b644275669c66e21e9da48ac7e89addf8c079be Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 18 Jan 2018 12:43:55 -0500 Subject: [PATCH 11/26] warn_only in status --- readthedocs/vcs_support/backends/git.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/readthedocs/vcs_support/backends/git.py b/readthedocs/vcs_support/backends/git.py index dc47a892418..2040a302f78 100644 --- a/readthedocs/vcs_support/backends/git.py +++ b/readthedocs/vcs_support/backends/git.py @@ -52,7 +52,7 @@ def update(self): self.checkout() def repo_exists(self): - code, _, _ = self.run('git', 'status') + code, _, _ = self.run('git', 'status', warn_only=True) return code == 0 def fetch(self): @@ -65,18 +65,16 @@ def checkout_revision(self, revision=None): branch = self.default_branch or self.fallback_branch revision = 'origin/%s' % branch - command = self.run('git', 'checkout', + code, out, err = self.run('git', 'checkout', '--force', '--quiet', revision) - code, out, err = command.exit_code, command.output, command.error if code != 0: log.warning("Failed to checkout revision '%s': %s", revision, code) return [code, out, err] def clone(self): - command = self.run('git', 'clone', '--recursive', '--quiet', + code, _, _ = self.run('git', 'clone', '--recursive', '--quiet', self.repo_url, '.') - code, _, _ = command.exit_code, command.output, command.error if code != 0: raise RepositoryError From 390924dbb3089e4edd7998c04ea671e6c8f91830 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 18 Jan 2018 12:46:55 -0500 Subject: [PATCH 12/26] Record command when inside a BuildEnvironment --- readthedocs/doc_builder/environments.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/readthedocs/doc_builder/environments.py b/readthedocs/doc_builder/environments.py index bb62e179840..ee91ab470ea 100644 --- a/readthedocs/doc_builder/environments.py +++ b/readthedocs/doc_builder/environments.py @@ -274,6 +274,7 @@ def __init__(self, project, environment=None): # TODO: maybe we can remove this Project dependency also self.project = project self.environment = environment or {} + self.commands = [] def record_command(self, command): pass @@ -373,7 +374,9 @@ def __init__(self, project=None, version=None, build=None, config=None, self.environment = environment or {} self.update_on_success = update_on_success + # TODO: remove this one, comes from super self.commands = [] + self.failure = None self.start_time = datetime.utcnow() @@ -410,6 +413,10 @@ def handle_exception(self, exc_type, exc_value, _): self.failure = exc_value return True + def record_command(self, command): + if self.record: + command.save() + def run(self, *cmd, **kwargs): kwargs.update({ 'build_env': self, From 80c964cdf1834d734751a1ac5b909a345d532975 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 18 Jan 2018 12:56:30 -0500 Subject: [PATCH 13/26] Remove --quiet options --- readthedocs/vcs_support/backends/git.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/readthedocs/vcs_support/backends/git.py b/readthedocs/vcs_support/backends/git.py index 2040a302f78..32a316f1c5a 100644 --- a/readthedocs/vcs_support/backends/git.py +++ b/readthedocs/vcs_support/backends/git.py @@ -66,14 +66,14 @@ def checkout_revision(self, revision=None): revision = 'origin/%s' % branch code, out, err = self.run('git', 'checkout', - '--force', '--quiet', revision) + '--force', revision) if code != 0: log.warning("Failed to checkout revision '%s': %s", revision, code) return [code, out, err] def clone(self): - code, _, _ = self.run('git', 'clone', '--recursive', '--quiet', + code, _, _ = self.run('git', 'clone', '--recursive', self.repo_url, '.') if code != 0: raise RepositoryError From c35b44b08151b70918bdb05bfece82963950a9a9 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 18 Jan 2018 12:58:51 -0500 Subject: [PATCH 14/26] Do not record ``warn_only`` commands --- readthedocs/doc_builder/environments.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/readthedocs/doc_builder/environments.py b/readthedocs/doc_builder/environments.py index ee91ab470ea..cb2f49cafec 100644 --- a/readthedocs/doc_builder/environments.py +++ b/readthedocs/doc_builder/environments.py @@ -306,7 +306,16 @@ def run_command_class(self, cls, cmd, warn_only=False, **kwargs): build_cmd.run() # TODO: I don't like how it's handled this entry point here - self.record_command(build_cmd) + if not warn_only: + # TODO: maybe receive ``record`` as an attribute for skip/record + # just specific commands but not all of them ran under the + # *BuildEnvironment + + # TODO: do we want to save commands that FAILED but not raised and + # exception? This will cause the first `git status` (when + # importing) to fail and be marked with RED in the Build command + # details + self.record_command(build_cmd) if build_cmd.failed: msg = u'Command {cmd} failed'.format(cmd=build_cmd.get_command()) From eab2f96a951d1baafc05526eb72a810cfe90ca87 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 18 Jan 2018 17:43:40 -0500 Subject: [PATCH 15/26] Refactor --- readthedocs/doc_builder/environments.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/readthedocs/doc_builder/environments.py b/readthedocs/doc_builder/environments.py index cb2f49cafec..e1be25c5cb4 100644 --- a/readthedocs/doc_builder/environments.py +++ b/readthedocs/doc_builder/environments.py @@ -1,3 +1,5 @@ +# -*- coding: utf-8 -*- + """Documentation Builder Environments.""" from __future__ import absolute_import @@ -14,7 +16,7 @@ from readthedocs.core.utils import slugify from django.conf import settings -from django.utils.translation import ugettext_lazy as _, ugettext_noop +from django.utils.translation import ugettext_lazy as _ from docker import Client from docker.utils import create_host_config from docker.errors import APIError as DockerAPIError, DockerException @@ -40,7 +42,8 @@ __all__ = ( 'api_v2', 'BuildCommand', 'DockerBuildCommand', - 'BuildEnvironment', 'LocalBuildEnvironment', 'DockerBuildEnvironment', + 'LocalEnvironment', + 'LocalBuildEnvironment', 'DockerBuildEnvironment', ) From 66094e5ab90d374119ffef65ce4a39c2855340ae Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 18 Jan 2018 18:23:08 -0500 Subject: [PATCH 16/26] Add warn_only to other VCS --- readthedocs/doc_builder/environments.py | 8 +++++++- readthedocs/vcs_support/backends/bzr.py | 5 +++-- readthedocs/vcs_support/backends/git.py | 2 +- readthedocs/vcs_support/backends/hg.py | 19 ++++++++++--------- readthedocs/vcs_support/backends/svn.py | 9 +++++---- readthedocs/vcs_support/base.py | 2 +- 6 files changed, 27 insertions(+), 18 deletions(-) diff --git a/readthedocs/doc_builder/environments.py b/readthedocs/doc_builder/environments.py index e1be25c5cb4..f1e31619057 100644 --- a/readthedocs/doc_builder/environments.py +++ b/readthedocs/doc_builder/environments.py @@ -273,6 +273,12 @@ def get_wrapped_command(self): class BaseEnvironment(object): + """ + Base environment class. + + Used to run arbitrary commands outside a build. + """ + def __init__(self, project, environment=None): # TODO: maybe we can remove this Project dependency also self.project = project @@ -550,7 +556,7 @@ def update_build(self, state=None): class LocalBuildEnvironment(BuildEnvironment): - """Local execution environment.""" + """Local execution build environment.""" command_class = BuildCommand diff --git a/readthedocs/vcs_support/backends/bzr.py b/readthedocs/vcs_support/backends/bzr.py index 6c58a667de1..952d5f56070 100644 --- a/readthedocs/vcs_support/backends/bzr.py +++ b/readthedocs/vcs_support/backends/bzr.py @@ -1,3 +1,4 @@ +# -*- coding: utf-8 -*- """Bazaar-related utilities.""" from __future__ import absolute_import @@ -21,7 +22,7 @@ class Backend(BaseVCS): def update(self): super(Backend, self).update() - retcode = self.run('bzr', 'status')[0] + retcode = self.run('bzr', 'status', warn_only=True)[0] if retcode == 0: self.up() else: @@ -44,7 +45,7 @@ def clone(self): @property def tags(self): - retcode, stdout = self.run('bzr', 'tags')[:2] + retcode, stdout = self.run('bzr', 'tags', warn_only=True)[:2] # error (or no tags found) if retcode != 0: return [] diff --git a/readthedocs/vcs_support/backends/git.py b/readthedocs/vcs_support/backends/git.py index 32a316f1c5a..a745accb9ee 100644 --- a/readthedocs/vcs_support/backends/git.py +++ b/readthedocs/vcs_support/backends/git.py @@ -80,7 +80,7 @@ def clone(self): @property def tags(self): - retcode, stdout, _ = self.run('git', 'show-ref', '--tags') + retcode, stdout, _ = self.run('git', 'show-ref', '--tags', warn_only=True) # error (or no tags found) if retcode != 0: return [] diff --git a/readthedocs/vcs_support/backends/hg.py b/readthedocs/vcs_support/backends/hg.py index c1f60ef8f6e..d8dfa3c7ec9 100644 --- a/readthedocs/vcs_support/backends/hg.py +++ b/readthedocs/vcs_support/backends/hg.py @@ -1,3 +1,4 @@ +# -*- coding: utf-8 -*- """Mercurial-related utilities.""" from __future__ import absolute_import from readthedocs.projects.exceptions import RepositoryError @@ -14,7 +15,7 @@ class Backend(BaseVCS): def update(self): super(Backend, self).update() - retcode = self.run('hg', 'status')[0] + retcode = self.run('hg', 'status', warn_only=True)[0] if retcode == 0: return self.pull() return self.clone() @@ -23,7 +24,7 @@ def pull(self): (pull_retcode, _, _) = self.run('hg', 'pull') if pull_retcode != 0: raise RepositoryError - (update_retcode, stdout, stderr) = self.run('hg', 'update', '-C') + (update_retcode, stdout, stderr) = self.run('hg', 'update', '--clean') if update_retcode != 0: raise RepositoryError return (update_retcode, stdout, stderr) @@ -37,7 +38,7 @@ def clone(self): @property def branches(self): - retcode, stdout = self.run('hg', 'branches', '-q')[:2] + retcode, stdout = self.run('hg', 'branches', warn_only=True)[:2] # error (or no tags found) if retcode != 0: return [] @@ -50,7 +51,7 @@ def parse_branches(self, data): @property def tags(self): - retcode, stdout = self.run('hg', 'tags')[:2] + retcode, stdout = self.run('hg', 'tags', warn_only=True)[:2] # error (or no tags found) if retcode != 0: return [] @@ -86,16 +87,16 @@ def parse_tags(self, data): @property def commit(self): - _, stdout = self.run('hg', 'id', '-i')[:2] + _, stdout = self.run('hg', 'identify', '--id')[:2] return stdout.strip() def checkout(self, identifier=None): super(Backend, self).checkout() if not identifier: identifier = 'tip' - retcode = self.run('hg', 'status')[0] + retcode = self.run('hg', 'status', warn_only=True)[0] if retcode == 0: self.run('hg', 'pull') - return self.run('hg', 'update', '-C', identifier) - self.clone() - return self.run('hg', 'update', '-C', identifier) + else: + self.clone() + return self.run('hg', 'update', '--clean', identifier) diff --git a/readthedocs/vcs_support/backends/svn.py b/readthedocs/vcs_support/backends/svn.py index 5c952e30882..123042edd45 100644 --- a/readthedocs/vcs_support/backends/svn.py +++ b/readthedocs/vcs_support/backends/svn.py @@ -1,3 +1,4 @@ +# -*- coding: utf-8 -*- """Subversion-related utilities.""" from __future__ import absolute_import @@ -33,7 +34,7 @@ def update(self): super(Backend, self).update() # For some reason `svn status` gives me retcode 0 in non-svn # directories that's why I use `svn info` here. - retcode = self.run('svn', 'info')[0] + retcode = self.run('svn', 'info', warn_only=True)[0] if retcode == 0: self.up() else: @@ -56,7 +57,7 @@ def co(self, identifier=None): url = self.base_url + identifier else: url = self.repo_url - retcode, out, err = self.run('svn', 'checkout', '--quiet', url, '.') + retcode, out, err = self.run('svn', 'checkout', url, '.') if retcode != 0: raise RepositoryError return retcode, out, err @@ -64,7 +65,7 @@ def co(self, identifier=None): @property def tags(self): retcode, stdout = self.run('svn', 'list', '%s/tags/' - % self.base_url)[:2] + % self.base_url, warn_only=True)[:2] # error (or no tags found) if retcode != 0: return [] @@ -98,7 +99,7 @@ def commit(self): def checkout(self, identifier=None): super(Backend, self).checkout() - retcode = self.run('svn', 'info')[0] + retcode = self.run('svn', 'info', warn_only=True)[0] if retcode == 0: result = self.up() else: diff --git a/readthedocs/vcs_support/base.py b/readthedocs/vcs_support/base.py index 2fbebf2ade1..9a623fcc1a2 100644 --- a/readthedocs/vcs_support/base.py +++ b/readthedocs/vcs_support/base.py @@ -37,7 +37,7 @@ class BaseVCS(object): """ Base for VCS Classes. - VCS commands are ran inside a ``LocalBuildEnvironment``. + VCS commands are ran inside a ``LocalEnvironment``. """ supports_tags = False # Whether this VCS supports tags or not. From 2fb632bad57c29bf81d4e25ea369f0355765a34f Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 18 Jan 2018 19:26:14 -0500 Subject: [PATCH 17/26] Remove `update_imported_docs` and use UpdateDocsTask --- docs/builds.rst | 2 +- readthedocs/core/management/commands/pull.py | 7 +- readthedocs/core/views/hooks.py | 14 ++- readthedocs/projects/tasks.py | 94 +++----------------- readthedocs/rtd_tests/tests/test_celery.py | 6 +- 5 files changed, 32 insertions(+), 91 deletions(-) diff --git a/docs/builds.rst b/docs/builds.rst index ec36a3e32a4..b3f84c713e2 100644 --- a/docs/builds.rst +++ b/docs/builds.rst @@ -75,7 +75,7 @@ An example in code: .. code-block:: python - update_imported_docs(version) + update_docs_from_vcs(version) if exists('setup.py'): run('python setup.py install') if project.requirements_file: diff --git a/readthedocs/core/management/commands/pull.py b/readthedocs/core/management/commands/pull.py index fa6810e0b52..0bdd3017f21 100644 --- a/readthedocs/core/management/commands/pull.py +++ b/readthedocs/core/management/commands/pull.py @@ -19,6 +19,9 @@ class Command(BaseCommand): def handle(self, *args, **options): if args: for slug in args: - tasks.update_imported_docs( - utils.version_from_slug(slug, LATEST).pk + version = utils.version_from_slug(slug, LATEST) + tasks.UpdateDocsTask().run( + version.project, + version.pk, + sync_only=True, ) diff --git a/readthedocs/core/views/hooks.py b/readthedocs/core/views/hooks.py index 9140c45a59c..3bdf09a737d 100644 --- a/readthedocs/core/views/hooks.py +++ b/readthedocs/core/views/hooks.py @@ -12,7 +12,7 @@ from readthedocs.builds.constants import LATEST from readthedocs.projects import constants from readthedocs.projects.models import Project, Feature -from readthedocs.projects.tasks import update_imported_docs +from readthedocs.projects.tasks import UpdateDocsTask import logging @@ -123,8 +123,16 @@ def _build_url(url, projects, branches): for project in projects: (built, not_building) = build_branches(project, branches) if not built: - # Call update_imported_docs to update tag/branch info - update_imported_docs.delay(project.versions.get(slug=LATEST).pk) + # Call UpdateDocsTask with ``sync_only`` to update tag/branch info + version = project.versions.get(slug=LATEST) + update_docs = UpdateDocsTask() + update_docs.apply_async( + args=(project.pk,), + kwargs={ + 'version_pk': version.pk, + 'sync_only': True, + }, + ) msg = '(URL Build) Syncing versions for %s' % project.slug log.info(msg) all_built[project.slug] = built diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index 1ce2580f08a..6b30d8124b4 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -106,7 +106,8 @@ def _log(self, msg): # pylint: disable=arguments-differ def run(self, pk, version_pk=None, build_pk=None, record=True, - docker=False, search=True, force=False, localmedia=True, **__): + docker=False, search=True, force=False, localmedia=True, + sync_only=False, **__): """ Run a documentation sync or sync n' build. @@ -144,9 +145,15 @@ def run(self, pk, version_pk=None, build_pk=None, record=True, self.build_force = force self.config = None + if sync_only: + self.sync_repo() + return True + setup_successful = self.run_setup(record=record) if not setup_successful: return False + + # Catch unhandled errors in the setup step except Exception as e: # noqa log.exception( @@ -365,7 +372,8 @@ def sync_repo(self): ) version_repo = self.project.vcs_repo( self.version.slug, - self.setup_env, + # When ``sync_only`` we don't a setup_env + getattr(self, 'setup_env', None), ) version_repo.checkout(self.version.identifier) finally: @@ -607,88 +615,6 @@ def send_notifications(self): send_notifications.delay(self.version.pk, build_pk=self.build['id']) -@app.task() -def update_imported_docs(version_pk, env=None): - """ - Check out or update the given project's repository. - - :param version_pk: Version id to update - """ - version_data = api_v2.version(version_pk).get() - version = APIVersion(**version_data) - project = version.project - ret_dict = {} - - # Make Dirs - if not os.path.exists(project.doc_path): - os.makedirs(project.doc_path) - - if not project.vcs_repo(): - raise RepositoryError( - _('Repository type "{repo_type}" unknown').format( - repo_type=project.repo_type - ) - ) - - with project.repo_nonblockinglock( - version=version, - max_lock_age=getattr(settings, 'REPO_LOCK_SECONDS', 30)): - - # Get the actual code on disk - try: - before_vcs.send(sender=version) - if version: - log.info( - LOG_TEMPLATE.format( - project=project.slug, - version=version.slug, - msg='Checking out version {slug}: {identifier}'.format( - slug=version.slug, - identifier=version.identifier - ) - ) - ) - version_slug = version.slug - version_repo = project.vcs_repo(version_slug) - - ret_dict['checkout'] = version_repo.checkout(version.identifier, env) - else: - # Does this ever get called? - log.info(LOG_TEMPLATE.format( - project=project.slug, version=version.slug, msg='Updating to latest revision')) - version_slug = LATEST - version_repo = project.vcs_repo(version_slug) - ret_dict['checkout'] = version_repo.update() - finally: - after_vcs.send(sender=version) - - # Update tags/version - - version_post_data = {'repo': version_repo.repo_url} - - if version_repo.supports_tags: - version_post_data['tags'] = [ - {'identifier': v.identifier, - 'verbose_name': v.verbose_name, - } for v in version_repo.tags - ] - - if version_repo.supports_branches: - version_post_data['branches'] = [ - {'identifier': v.identifier, - 'verbose_name': v.verbose_name, - } for v in version_repo.branches - ] - - try: - api_v2.project(project.pk).sync_versions.post(version_post_data) - except HttpClientError: - log.exception("Sync Versions Exception") - except Exception: - log.exception("Unknown Sync Versions Exception") - return ret_dict - - # Web tasks @app.task(queue='web') def sync_files(project_pk, version_pk, hostname=None, html=False, diff --git a/readthedocs/rtd_tests/tests/test_celery.py b/readthedocs/rtd_tests/tests/test_celery.py index e62ea570581..8ffc110f20b 100644 --- a/readthedocs/rtd_tests/tests/test_celery.py +++ b/readthedocs/rtd_tests/tests/test_celery.py @@ -117,5 +117,9 @@ def test_update_docs_unexpected_build_exception(self, mock_build_docs): def test_update_imported_doc(self): with mock_api(self.repo): - result = tasks.update_imported_docs.delay(self.project.pk) + update_docs = tasks.UpdateDocsTask() + result = update_docs.apply_async( + args=(self.project.pk,), + kwargs={'sync_only': True}, + ) self.assertTrue(result.successful()) From d209ec3e52bd11789ef455bdcf45815f730300de Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 18 Jan 2018 19:27:16 -0500 Subject: [PATCH 18/26] Hack the Environment logs to avoid fails because no version --- readthedocs/doc_builder/environments.py | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/readthedocs/doc_builder/environments.py b/readthedocs/doc_builder/environments.py index f1e31619057..315e334d5a3 100644 --- a/readthedocs/doc_builder/environments.py +++ b/readthedocs/doc_builder/environments.py @@ -288,6 +288,13 @@ def __init__(self, project, environment=None): def record_command(self, command): pass + def _log_warn_only(self, msg): + log.warning(LOG_TEMPLATE.format( + project=self.project.slug, + version='latest', + msg=msg, + )) + def run(self, *cmd, **kwargs): """Shortcut to run command from environment.""" return self.run_command_class(cls=self.command_class, cmd=cmd, **kwargs) @@ -333,11 +340,7 @@ def run_command_class(self, cls, cmd, warn_only=False, **kwargs): msg += u':\n{out}'.format(out=build_cmd.output) if warn_only: - # TODO: remove version dependency to log here - log.warning(LOG_TEMPLATE - .format(project=self.project.slug, - version=self.version.slug, - msg=msg)) + self._log_warn_only(msg) else: raise BuildEnvironmentWarning(msg) return build_cmd @@ -435,6 +438,14 @@ def record_command(self, command): if self.record: command.save() + def _log_warn_only(self, msg): + # :'( + log.warning(LOG_TEMPLATE.format( + project=self.project.slug, + version=self.version.slug, + msg=msg, + )) + def run(self, *cmd, **kwargs): kwargs.update({ 'build_env': self, From 92368d9f25cf5c56228d89ee0dd1383d8fbec48c Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 23 Jan 2018 10:54:49 -0500 Subject: [PATCH 19/26] Abstraction for UpdateDocsTask and SyncRepositoryTask These are two separated tasks that share some code by inheriting from SyncRepositoryMixin. The final goal (as ``core.views.hooks._build_url`` is *DEPRECATED*) is to finally remove the SyncRepositoryTask and merge that code into UpdateDocsTask. --- readthedocs/core/management/commands/pull.py | 4 +- readthedocs/core/views/hooks.py | 14 +- readthedocs/projects/apps.py | 5 +- readthedocs/projects/tasks.py | 209 +++++++++++-------- readthedocs/rtd_tests/tests/test_celery.py | 12 +- 5 files changed, 142 insertions(+), 102 deletions(-) diff --git a/readthedocs/core/management/commands/pull.py b/readthedocs/core/management/commands/pull.py index 0bdd3017f21..c6c8d783043 100644 --- a/readthedocs/core/management/commands/pull.py +++ b/readthedocs/core/management/commands/pull.py @@ -20,8 +20,6 @@ def handle(self, *args, **options): if args: for slug in args: version = utils.version_from_slug(slug, LATEST) - tasks.UpdateDocsTask().run( - version.project, + tasks.SyncRepositoryTask().run( version.pk, - sync_only=True, ) diff --git a/readthedocs/core/views/hooks.py b/readthedocs/core/views/hooks.py index 3bdf09a737d..4564f7394d6 100644 --- a/readthedocs/core/views/hooks.py +++ b/readthedocs/core/views/hooks.py @@ -12,7 +12,7 @@ from readthedocs.builds.constants import LATEST from readthedocs.projects import constants from readthedocs.projects.models import Project, Feature -from readthedocs.projects.tasks import UpdateDocsTask +from readthedocs.projects.tasks import SyncRepositoryTask import logging @@ -123,15 +123,11 @@ def _build_url(url, projects, branches): for project in projects: (built, not_building) = build_branches(project, branches) if not built: - # Call UpdateDocsTask with ``sync_only`` to update tag/branch info + # Call SyncRepositoryTask to update tag/branch info version = project.versions.get(slug=LATEST) - update_docs = UpdateDocsTask() - update_docs.apply_async( - args=(project.pk,), - kwargs={ - 'version_pk': version.pk, - 'sync_only': True, - }, + sync_repository = SyncRepositoryTask() + sync_repository.apply_async( + args=(version.pk,), ) msg = '(URL Build) Syncing versions for %s' % project.slug log.info(msg) diff --git a/readthedocs/projects/apps.py b/readthedocs/projects/apps.py index 71111f4e8d0..c6c0bf29017 100644 --- a/readthedocs/projects/apps.py +++ b/readthedocs/projects/apps.py @@ -7,6 +7,7 @@ class ProjectsConfig(AppConfig): name = 'readthedocs.projects' def ready(self): - from readthedocs.projects.tasks import UpdateDocsTask + from readthedocs.projects import tasks from readthedocs.worker import app - app.tasks.register(UpdateDocsTask) + app.tasks.register(tasks.SyncRepositoryTask) + app.tasks.register(tasks.UpdateDocsTask) diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index 6b30d8124b4..4c2e4f9d96b 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -63,7 +63,132 @@ HTML_ONLY = getattr(settings, 'HTML_ONLY_PROJECTS', ()) -class UpdateDocsTask(Task): +class SyncRepositoryMixin(object): + + """ + Mixin that handles the VCS sync/update. + """ + + @staticmethod + def get_version(project=None, version_pk=None): + """ + Retrieve version data from the API. + + :param project: project object to sync + :type project: projects.models.Project + :param version_pk: version pk to sync + :type version_pk: int + :returns: a data-complete version object + :rtype: builds.models.APIVersion + """ + assert (project or version_pk), 'project or version_pk is needed' + if version_pk: + version_data = api_v2.version(version_pk).get() + else: + version_data = (api_v2 + .version(project.slug) + .get(slug=LATEST)['objects'][0]) + return APIVersion(**version_data) + + def sync_repo(self): + """ + Checkout/update the project's repository and hit ``sync_versions`` API. + """ + # Make Dirs + if not os.path.exists(self.project.doc_path): + os.makedirs(self.project.doc_path) + + if not self.project.vcs_repo(): + raise RepositoryError( + _('Repository type "{repo_type}" unknown').format( + repo_type=self.project.repo_type, + ), + ) + + with self.project.repo_nonblockinglock( + version=self.version, + max_lock_age=getattr(settings, 'REPO_LOCK_SECONDS', 30)): + + # Get the actual code on disk + try: + before_vcs.send(sender=self.version) + self._log( + 'Checking out version {slug}: {identifier}'.format( + slug=self.version.slug, + identifier=self.version.identifier, + ), + ) + version_repo = self.project.vcs_repo( + self.version.slug, + # When called from ``SyncRepositoryTask.run`` we don't have + # a ``setup_env`` so we use just ``None`` and commands won't + # be recorded + getattr(self, 'setup_env', None), + ) + version_repo.checkout(self.version.identifier) + finally: + after_vcs.send(sender=self.version) + + # Update tags/version + version_post_data = {'repo': version_repo.repo_url} + + if version_repo.supports_tags: + version_post_data['tags'] = [ + {'identifier': v.identifier, + 'verbose_name': v.verbose_name, + } for v in version_repo.tags + ] + + if version_repo.supports_branches: + version_post_data['branches'] = [ + {'identifier': v.identifier, + 'verbose_name': v.verbose_name, + } for v in version_repo.branches + ] + + try: + # Hit the API ``sync_versions`` which may trigger a new build + # for the stable version + api_v2.project(self.project.pk).sync_versions.post(version_post_data) + except HttpClientError: + log.exception('Sync Versions Exception') + except Exception: + log.exception('Unknown Sync Versions Exception') + + +class SyncRepositoryTask(SyncRepositoryMixin, Task): + + """ + Entry point to synchronize the VCS documentation. + """ + + max_retries = 5 + default_retry_delay = (7 * 60) + name = __name__ + '.sync_repository' + + def run(self, version_pk): + """ + Run the VCS synchronization. + + :param version_pk: version pk to sync + :type version_pk: int + :returns: whether or not the task ended successfully + :rtype: bool + """ + try: + self.version = self.get_version(version_pk=version_pk) + self.project = self.version.project + self.sync_repo() + return True + # Catch unhandled errors when syncing + except Exception: + log.exception( + 'An unhandled exception was raised during VCS syncing', + ) + return False + + +class UpdateDocsTask(SyncRepositoryMixin, Task): """ The main entry point for updating documentation. @@ -106,8 +231,7 @@ def _log(self, msg): # pylint: disable=arguments-differ def run(self, pk, version_pk=None, build_pk=None, record=True, - docker=False, search=True, force=False, localmedia=True, - sync_only=False, **__): + docker=False, search=True, force=False, localmedia=True, **__): """ Run a documentation sync or sync n' build. @@ -145,10 +269,6 @@ def run(self, pk, version_pk=None, build_pk=None, record=True, self.build_force = force self.config = None - if sync_only: - self.sync_repo() - return True - setup_successful = self.run_setup(record=record) if not setup_successful: return False @@ -301,17 +421,6 @@ def get_project(project_pk): project_data = api_v2.project(project_pk).get() return APIProject(**project_data) - @staticmethod - def get_version(project, version_pk): - """Ensure we're using a sane version.""" - if version_pk: - version_data = api_v2.version(version_pk).get() - else: - version_data = (api_v2 - .version(project.slug) - .get(slug=LATEST)['objects'][0]) - return APIVersion(**version_data) - @staticmethod def get_build(build_pk): """ @@ -342,70 +451,6 @@ def setup_vcs(self): if commit: self.build['commit'] = commit - def sync_repo(self): - """ - Checkout/update the project's repository and hit ``sync_versions`` API. - """ - # Make Dirs - if not os.path.exists(self.project.doc_path): - os.makedirs(self.project.doc_path) - - if not self.project.vcs_repo(): - raise RepositoryError( - _('Repository type "{repo_type}" unknown').format( - repo_type=self.project.repo_type, - ), - ) - - with self.project.repo_nonblockinglock( - version=self.version, - max_lock_age=getattr(settings, 'REPO_LOCK_SECONDS', 30)): - - # Get the actual code on disk - try: - before_vcs.send(sender=self.version) - self._log( - 'Checking out version {slug}: {identifier}'.format( - slug=self.version.slug, - identifier=self.version.identifier, - ), - ) - version_repo = self.project.vcs_repo( - self.version.slug, - # When ``sync_only`` we don't a setup_env - getattr(self, 'setup_env', None), - ) - version_repo.checkout(self.version.identifier) - finally: - after_vcs.send(sender=self.version) - - # Update tags/version - version_post_data = {'repo': version_repo.repo_url} - - if version_repo.supports_tags: - version_post_data['tags'] = [ - {'identifier': v.identifier, - 'verbose_name': v.verbose_name, - } for v in version_repo.tags - ] - - if version_repo.supports_branches: - version_post_data['branches'] = [ - {'identifier': v.identifier, - 'verbose_name': v.verbose_name, - } for v in version_repo.branches - ] - - try: - # Hit the API ``sync_versions`` which may trigger a new build - # for the stable version - api_v2.project(self.project.pk).sync_versions.post(version_post_data) - except HttpClientError: - log.exception('Sync Versions Exception') - except Exception: - log.exception('Unknown Sync Versions Exception') - - def get_env_vars(self): """Get bash environment variables used for all builder commands.""" env = { diff --git a/readthedocs/rtd_tests/tests/test_celery.py b/readthedocs/rtd_tests/tests/test_celery.py index 8ffc110f20b..e9124baf104 100644 --- a/readthedocs/rtd_tests/tests/test_celery.py +++ b/readthedocs/rtd_tests/tests/test_celery.py @@ -8,7 +8,7 @@ from django_dynamic_fixture import get from mock import patch, MagicMock -from readthedocs.builds.constants import BUILD_STATE_INSTALLING, BUILD_STATE_FINISHED +from readthedocs.builds.constants import BUILD_STATE_INSTALLING, BUILD_STATE_FINISHED, LATEST from readthedocs.builds.models import Build from readthedocs.projects.models import Project from readthedocs.projects import tasks @@ -115,11 +115,11 @@ def test_update_docs_unexpected_build_exception(self, mock_build_docs): intersphinx=False) self.assertTrue(result.successful()) - def test_update_imported_doc(self): + def test_sync_repository(self): + version = self.project.versions.get(slug=LATEST) with mock_api(self.repo): - update_docs = tasks.UpdateDocsTask() - result = update_docs.apply_async( - args=(self.project.pk,), - kwargs={'sync_only': True}, + sync_repository = tasks.SyncRepositoryTask() + result = sync_repository.apply_async( + args=(version.pk,), ) self.assertTrue(result.successful()) From f7e15aa164af949cc0367829adf76d10b37c6931 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 23 Jan 2018 11:41:42 -0500 Subject: [PATCH 20/26] Allow to not record specific command Either `record=False` or `warn_only=True` commands are not recorded. This attributes can be set at initialization time or when calling the `run()` method. --- readthedocs/doc_builder/environments.py | 32 ++++++++++++------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/readthedocs/doc_builder/environments.py b/readthedocs/doc_builder/environments.py index 315e334d5a3..53d74583085 100644 --- a/readthedocs/doc_builder/environments.py +++ b/readthedocs/doc_builder/environments.py @@ -299,14 +299,19 @@ def run(self, *cmd, **kwargs): """Shortcut to run command from environment.""" return self.run_command_class(cls=self.command_class, cmd=cmd, **kwargs) - def run_command_class(self, cls, cmd, warn_only=False, **kwargs): + def run_command_class(self, cls, cmd, record=None, warn_only=False, **kwargs): """ Run command from this environment. - Use ``cls`` to instantiate a command - - :param warn_only: Don't raise an exception on command failure + :param cls: command class to instantiate a command + :param cmd: command (as a list) to execute in this environment + :param warn_only: don't raise an exception on command failure + :param record: whether or not to record this particular command """ + if record is None: + # ``self.record`` only exists when called from ``*BuildEnvironment`` + record = getattr(self, 'record', False) + # Remove PATH from env, and set it to bin_path if it isn't passed in env_path = self.environment.pop('BIN_PATH', None) if 'bin_path' not in kwargs and env_path: @@ -314,23 +319,16 @@ def run_command_class(self, cls, cmd, warn_only=False, **kwargs): assert 'environment' not in kwargs, "environment can't be passed in via commands." kwargs['environment'] = self.environment - # TODO: ``build_env`` is passed as ``kwargs`` when it's called from a ``*BuildEnvironment`` - # kwargs['build_env'] = self - + # ``build_env`` is passed as ``kwargs`` when it's called from a + # ``*BuildEnvironment`` build_cmd = cls(cmd, **kwargs) self.commands.append(build_cmd) build_cmd.run() - # TODO: I don't like how it's handled this entry point here - if not warn_only: - # TODO: maybe receive ``record`` as an attribute for skip/record - # just specific commands but not all of them ran under the - # *BuildEnvironment - - # TODO: do we want to save commands that FAILED but not raised and - # exception? This will cause the first `git status` (when - # importing) to fail and be marked with RED in the Build command - # details + if record and not warn_only: + # TODO: I don't like how it's handled this entry point here since + # this class should know nothing about a BuildCommand (which are the + # only ones that can be saved/recorded) self.record_command(build_cmd) if build_cmd.failed: From d833b2d646bc9ef6235202d1dfba0a370ac68786 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 24 Jan 2018 10:55:44 -0500 Subject: [PATCH 21/26] Proper use of `record`, `warn_only` and the new `force_success` --- readthedocs/doc_builder/environments.py | 24 ++++++++++++++++++------ readthedocs/vcs_support/backends/bzr.py | 4 ++-- readthedocs/vcs_support/backends/git.py | 6 +++--- readthedocs/vcs_support/backends/hg.py | 8 ++++---- readthedocs/vcs_support/backends/svn.py | 6 +++--- 5 files changed, 30 insertions(+), 18 deletions(-) diff --git a/readthedocs/doc_builder/environments.py b/readthedocs/doc_builder/environments.py index 53d74583085..33936ab304f 100644 --- a/readthedocs/doc_builder/environments.py +++ b/readthedocs/doc_builder/environments.py @@ -288,7 +288,7 @@ def __init__(self, project, environment=None): def record_command(self, command): pass - def _log_warn_only(self, msg): + def _log_warning(self, msg): log.warning(LOG_TEMPLATE.format( project=self.project.slug, version='latest', @@ -299,19 +299,27 @@ def run(self, *cmd, **kwargs): """Shortcut to run command from environment.""" return self.run_command_class(cls=self.command_class, cmd=cmd, **kwargs) - def run_command_class(self, cls, cmd, record=None, warn_only=False, **kwargs): + def run_command_class( + self, cls, cmd, record=None, warn_only=False, force_success=False, + **kwargs): """ Run command from this environment. :param cls: command class to instantiate a command :param cmd: command (as a list) to execute in this environment - :param warn_only: don't raise an exception on command failure :param record: whether or not to record this particular command + (``False`` implies ``warn_only=True``) + :param warn_only: don't raise an exception on command failure + :param force_success: force command ``exit_code`` to be saved as ``0`` + (``True`` implies ``warn_only=True``) """ if record is None: # ``self.record`` only exists when called from ``*BuildEnvironment`` record = getattr(self, 'record', False) + if not record or force_success: + warn_only = True + # Remove PATH from env, and set it to bin_path if it isn't passed in env_path = self.environment.pop('BIN_PATH', None) if 'bin_path' not in kwargs and env_path: @@ -325,20 +333,24 @@ def run_command_class(self, cls, cmd, record=None, warn_only=False, **kwargs): self.commands.append(build_cmd) build_cmd.run() - if record and not warn_only: + if record: # TODO: I don't like how it's handled this entry point here since # this class should know nothing about a BuildCommand (which are the # only ones that can be saved/recorded) self.record_command(build_cmd) if build_cmd.failed: + if force_success: + self._log_warning('Forcing command to exit gracefully (exit_code = 0)') + build_cmd.exit_code = 0 + msg = u'Command {cmd} failed'.format(cmd=build_cmd.get_command()) if build_cmd.output: msg += u':\n{out}'.format(out=build_cmd.output) if warn_only: - self._log_warn_only(msg) + self._log_warning(msg) else: raise BuildEnvironmentWarning(msg) return build_cmd @@ -436,7 +448,7 @@ def record_command(self, command): if self.record: command.save() - def _log_warn_only(self, msg): + def _log_warning(self, msg): # :'( log.warning(LOG_TEMPLATE.format( project=self.project.slug, diff --git a/readthedocs/vcs_support/backends/bzr.py b/readthedocs/vcs_support/backends/bzr.py index 952d5f56070..b761ade5056 100644 --- a/readthedocs/vcs_support/backends/bzr.py +++ b/readthedocs/vcs_support/backends/bzr.py @@ -22,7 +22,7 @@ class Backend(BaseVCS): def update(self): super(Backend, self).update() - retcode = self.run('bzr', 'status', warn_only=True)[0] + retcode = self.run('bzr', 'status', record=False)[0] if retcode == 0: self.up() else: @@ -45,7 +45,7 @@ def clone(self): @property def tags(self): - retcode, stdout = self.run('bzr', 'tags', warn_only=True)[:2] + retcode, stdout = self.run('bzr', 'tags', force_success=True)[:2] # error (or no tags found) if retcode != 0: return [] diff --git a/readthedocs/vcs_support/backends/git.py b/readthedocs/vcs_support/backends/git.py index a745accb9ee..217c7d932d7 100644 --- a/readthedocs/vcs_support/backends/git.py +++ b/readthedocs/vcs_support/backends/git.py @@ -52,7 +52,7 @@ def update(self): self.checkout() def repo_exists(self): - code, _, _ = self.run('git', 'status', warn_only=True) + code, _, _ = self.run('git', 'status', record=False) return code == 0 def fetch(self): @@ -80,7 +80,7 @@ def clone(self): @property def tags(self): - retcode, stdout, _ = self.run('git', 'show-ref', '--tags', warn_only=True) + retcode, stdout, _ = self.run('git', 'show-ref', '--tags', force_success=True) # error (or no tags found) if retcode != 0: return [] @@ -205,7 +205,7 @@ def find_ref(self, ref): return ref def ref_exists(self, ref): - code, _, _ = self.run('git', 'show-ref', ref) + code, _, _ = self.run('git', 'show-ref', ref, force_success=True) return code == 0 @property diff --git a/readthedocs/vcs_support/backends/hg.py b/readthedocs/vcs_support/backends/hg.py index d8dfa3c7ec9..af556ad5684 100644 --- a/readthedocs/vcs_support/backends/hg.py +++ b/readthedocs/vcs_support/backends/hg.py @@ -15,7 +15,7 @@ class Backend(BaseVCS): def update(self): super(Backend, self).update() - retcode = self.run('hg', 'status', warn_only=True)[0] + retcode = self.run('hg', 'status', record=False)[0] if retcode == 0: return self.pull() return self.clone() @@ -38,7 +38,7 @@ def clone(self): @property def branches(self): - retcode, stdout = self.run('hg', 'branches', warn_only=True)[:2] + retcode, stdout = self.run('hg', 'branches', force_success=True)[:2] # error (or no tags found) if retcode != 0: return [] @@ -51,7 +51,7 @@ def parse_branches(self, data): @property def tags(self): - retcode, stdout = self.run('hg', 'tags', warn_only=True)[:2] + retcode, stdout = self.run('hg', 'tags', force_success=True)[:2] # error (or no tags found) if retcode != 0: return [] @@ -94,7 +94,7 @@ def checkout(self, identifier=None): super(Backend, self).checkout() if not identifier: identifier = 'tip' - retcode = self.run('hg', 'status', warn_only=True)[0] + retcode = self.run('hg', 'status', record=False)[0] if retcode == 0: self.run('hg', 'pull') else: diff --git a/readthedocs/vcs_support/backends/svn.py b/readthedocs/vcs_support/backends/svn.py index 123042edd45..f342f9afa76 100644 --- a/readthedocs/vcs_support/backends/svn.py +++ b/readthedocs/vcs_support/backends/svn.py @@ -34,7 +34,7 @@ def update(self): super(Backend, self).update() # For some reason `svn status` gives me retcode 0 in non-svn # directories that's why I use `svn info` here. - retcode = self.run('svn', 'info', warn_only=True)[0] + retcode = self.run('svn', 'info', force_success=True)[0] if retcode == 0: self.up() else: @@ -65,7 +65,7 @@ def co(self, identifier=None): @property def tags(self): retcode, stdout = self.run('svn', 'list', '%s/tags/' - % self.base_url, warn_only=True)[:2] + % self.base_url, force_success=True)[:2] # error (or no tags found) if retcode != 0: return [] @@ -99,7 +99,7 @@ def commit(self): def checkout(self, identifier=None): super(Backend, self).checkout() - retcode = self.run('svn', 'info', warn_only=True)[0] + retcode = self.run('svn', 'info', record=False)[0] if retcode == 0: result = self.up() else: From e7ce7d753110cb98c42efd244b6aa5df56a3eceb Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 24 Jan 2018 15:17:16 -0500 Subject: [PATCH 22/26] Use record_as_success instead of force_success We need the real exit_code for some command since there are decisions based on that code, but we want to record it as success. So, the fake exit_code is saved in the database but the real exit_code is used in the whole flow of the building process. --- readthedocs/doc_builder/environments.py | 37 ++++++++++++++++--------- 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/readthedocs/doc_builder/environments.py b/readthedocs/doc_builder/environments.py index 33936ab304f..34057123855 100644 --- a/readthedocs/doc_builder/environments.py +++ b/readthedocs/doc_builder/environments.py @@ -73,7 +73,7 @@ class BuildCommand(BuildCommandResultMixin): def __init__(self, command, cwd=None, shell=False, environment=None, combine_output=True, input_data=None, build_env=None, - bin_path=None, description=None): + bin_path=None, description=None, record_as_success=False): self.command = command self.shell = shell if cwd is None: @@ -96,6 +96,7 @@ def __init__(self, command, cwd=None, shell=False, environment=None, self.description = '' if description is not None: self.description = description + self.record_as_success = record_as_success self.exit_code = None def __str__(self): @@ -183,12 +184,21 @@ def get_command(self): def save(self): """Save this command and result via the API.""" + exit_code = self.exit_code + + # Force record this command as success to avoid Build reporting errors + # on commands that are just for checking purposes and do not interferes + # in the Build + if self.record_as_success: + log.warning('Recording command exit_code as success') + exit_code = 0 + data = { 'build': self.build_env.build.get('id'), 'command': self.get_command(), 'description': self.description, 'output': self.output, - 'exit_code': self.exit_code, + 'exit_code': exit_code, 'start_time': self.start_time, 'end_time': self.end_time, } @@ -300,8 +310,8 @@ def run(self, *cmd, **kwargs): return self.run_command_class(cls=self.command_class, cmd=cmd, **kwargs) def run_command_class( - self, cls, cmd, record=None, warn_only=False, force_success=False, - **kwargs): + self, cls, cmd, record=None, warn_only=False, + record_as_success=False, **kwargs): """ Run command from this environment. @@ -310,16 +320,22 @@ def run_command_class( :param record: whether or not to record this particular command (``False`` implies ``warn_only=True``) :param warn_only: don't raise an exception on command failure - :param force_success: force command ``exit_code`` to be saved as ``0`` - (``True`` implies ``warn_only=True``) + :param record_as_success: force command ``exit_code`` to be saved as + ``0`` (``True`` implies ``warn_only=True`` and ``record=True``) """ if record is None: # ``self.record`` only exists when called from ``*BuildEnvironment`` record = getattr(self, 'record', False) - if not record or force_success: + if not record: warn_only = True + if record_as_success: + record = True + warn_only = True + # ``record_as_success`` is needed to instantiate the BuildCommand + kwargs.update({'record_as_success': record_as_success}) + # Remove PATH from env, and set it to bin_path if it isn't passed in env_path = self.environment.pop('BIN_PATH', None) if 'bin_path' not in kwargs and env_path: @@ -340,10 +356,6 @@ def run_command_class( self.record_command(build_cmd) if build_cmd.failed: - if force_success: - self._log_warning('Forcing command to exit gracefully (exit_code = 0)') - build_cmd.exit_code = 0 - msg = u'Command {cmd} failed'.format(cmd=build_cmd.get_command()) if build_cmd.output: @@ -445,8 +457,7 @@ def handle_exception(self, exc_type, exc_value, _): return True def record_command(self, command): - if self.record: - command.save() + command.save() def _log_warning(self, msg): # :'( From 7b3488149004e5bf11ba9f00956df9ab247e464b Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 24 Jan 2018 15:21:52 -0500 Subject: [PATCH 23/26] Replace force_success to record_as_success in calls --- readthedocs/vcs_support/backends/bzr.py | 2 +- readthedocs/vcs_support/backends/git.py | 4 ++-- readthedocs/vcs_support/backends/hg.py | 4 ++-- readthedocs/vcs_support/backends/svn.py | 4 ++-- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/readthedocs/vcs_support/backends/bzr.py b/readthedocs/vcs_support/backends/bzr.py index b761ade5056..7d7234b84f2 100644 --- a/readthedocs/vcs_support/backends/bzr.py +++ b/readthedocs/vcs_support/backends/bzr.py @@ -45,7 +45,7 @@ def clone(self): @property def tags(self): - retcode, stdout = self.run('bzr', 'tags', force_success=True)[:2] + retcode, stdout = self.run('bzr', 'tags', record_as_success=True)[:2] # error (or no tags found) if retcode != 0: return [] diff --git a/readthedocs/vcs_support/backends/git.py b/readthedocs/vcs_support/backends/git.py index 217c7d932d7..9612800ac31 100644 --- a/readthedocs/vcs_support/backends/git.py +++ b/readthedocs/vcs_support/backends/git.py @@ -80,7 +80,7 @@ def clone(self): @property def tags(self): - retcode, stdout, _ = self.run('git', 'show-ref', '--tags', force_success=True) + retcode, stdout, _ = self.run('git', 'show-ref', '--tags', record_as_success=True) # error (or no tags found) if retcode != 0: return [] @@ -205,7 +205,7 @@ def find_ref(self, ref): return ref def ref_exists(self, ref): - code, _, _ = self.run('git', 'show-ref', ref, force_success=True) + code, _, _ = self.run('git', 'show-ref', ref, record_as_success=True) return code == 0 @property diff --git a/readthedocs/vcs_support/backends/hg.py b/readthedocs/vcs_support/backends/hg.py index af556ad5684..6aebe57f259 100644 --- a/readthedocs/vcs_support/backends/hg.py +++ b/readthedocs/vcs_support/backends/hg.py @@ -38,7 +38,7 @@ def clone(self): @property def branches(self): - retcode, stdout = self.run('hg', 'branches', force_success=True)[:2] + retcode, stdout = self.run('hg', 'branches', record_as_success=True)[:2] # error (or no tags found) if retcode != 0: return [] @@ -51,7 +51,7 @@ def parse_branches(self, data): @property def tags(self): - retcode, stdout = self.run('hg', 'tags', force_success=True)[:2] + retcode, stdout = self.run('hg', 'tags', record_as_success=True)[:2] # error (or no tags found) if retcode != 0: return [] diff --git a/readthedocs/vcs_support/backends/svn.py b/readthedocs/vcs_support/backends/svn.py index f342f9afa76..9d456c86226 100644 --- a/readthedocs/vcs_support/backends/svn.py +++ b/readthedocs/vcs_support/backends/svn.py @@ -34,7 +34,7 @@ def update(self): super(Backend, self).update() # For some reason `svn status` gives me retcode 0 in non-svn # directories that's why I use `svn info` here. - retcode = self.run('svn', 'info', force_success=True)[0] + retcode = self.run('svn', 'info', record_as_success=True)[0] if retcode == 0: self.up() else: @@ -65,7 +65,7 @@ def co(self, identifier=None): @property def tags(self): retcode, stdout = self.run('svn', 'list', '%s/tags/' - % self.base_url, force_success=True)[:2] + % self.base_url, record_as_success=True)[:2] # error (or no tags found) if retcode != 0: return [] From 036d4601ff2c9d36ba0b4cb286cccb05ee3e103c Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 24 Jan 2018 17:18:08 -0500 Subject: [PATCH 24/26] Lint --- readthedocs/doc_builder/environments.py | 3 +-- readthedocs/projects/models.py | 1 - readthedocs/projects/tasks.py | 15 ++++----------- readthedocs/vcs_support/backends/git.py | 8 ++++---- 4 files changed, 9 insertions(+), 18 deletions(-) diff --git a/readthedocs/doc_builder/environments.py b/readthedocs/doc_builder/environments.py index 34057123855..7c4c3f621b8 100644 --- a/readthedocs/doc_builder/environments.py +++ b/readthedocs/doc_builder/environments.py @@ -473,13 +473,12 @@ def run(self, *cmd, **kwargs): }) return super(BuildEnvironment, self).run(*cmd, **kwargs) - def run_command_class(self, *cmd, **kwargs): + def run_command_class(self, *cmd, **kwargs): # pylint: disable=arguments-differ kwargs.update({ 'build_env': self, }) return super(BuildEnvironment, self).run_command_class(*cmd, **kwargs) - @property def successful(self): """Is build completed, without top level failures or failing commands.""" # noqa diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index 5cfc0a344bf..5ff628e899f 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -613,7 +613,6 @@ def vcs_repo(self, version=LATEST, environment=None): :param version: version slug for the backend (``LATEST`` by default) :type version: str """ - # TODO: this seems to be the only method that receives a # ``version.slug`` instead of a ``Version`` instance (I prefer an # instance here) diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index 4c2e4f9d96b..a7bc66fac8d 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -65,9 +65,7 @@ class SyncRepositoryMixin(object): - """ - Mixin that handles the VCS sync/update. - """ + """Mixin that handles the VCS sync/update.""" @staticmethod def get_version(project=None, version_pk=None): @@ -91,9 +89,7 @@ def get_version(project=None, version_pk=None): return APIVersion(**version_data) def sync_repo(self): - """ - Checkout/update the project's repository and hit ``sync_versions`` API. - """ + """Update the project's repository and hit ``sync_versions`` API.""" # Make Dirs if not os.path.exists(self.project.doc_path): os.makedirs(self.project.doc_path) @@ -158,15 +154,13 @@ def sync_repo(self): class SyncRepositoryTask(SyncRepositoryMixin, Task): - """ - Entry point to synchronize the VCS documentation. - """ + """Entry point to synchronize the VCS documentation.""" max_retries = 5 default_retry_delay = (7 * 60) name = __name__ + '.sync_repository' - def run(self, version_pk): + def run(self, version_pk): # pylint: disable=arguments-differ """ Run the VCS synchronization. @@ -273,7 +267,6 @@ def run(self, pk, version_pk=None, build_pk=None, record=True, if not setup_successful: return False - # Catch unhandled errors in the setup step except Exception as e: # noqa log.exception( diff --git a/readthedocs/vcs_support/backends/git.py b/readthedocs/vcs_support/backends/git.py index 9612800ac31..d2e623de1c6 100644 --- a/readthedocs/vcs_support/backends/git.py +++ b/readthedocs/vcs_support/backends/git.py @@ -65,16 +65,16 @@ def checkout_revision(self, revision=None): branch = self.default_branch or self.fallback_branch revision = 'origin/%s' % branch - code, out, err = self.run('git', 'checkout', - '--force', revision) + code, out, err = self.run( + 'git', 'checkout', '--force', revision) if code != 0: log.warning("Failed to checkout revision '%s': %s", revision, code) return [code, out, err] def clone(self): - code, _, _ = self.run('git', 'clone', '--recursive', - self.repo_url, '.') + code, _, _ = self.run( + 'git', 'clone', '--recursive', self.repo_url, '.') if code != 0: raise RepositoryError From 8c1fd17f982d61eb4d62d184787b0f9cf5c7ac7c Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 25 Jan 2018 10:56:04 -0500 Subject: [PATCH 25/26] Fix docs styling --- readthedocs/projects/tasks.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index a7bc66fac8d..71ddb8b7c83 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -227,7 +227,7 @@ def _log(self, msg): def run(self, pk, version_pk=None, build_pk=None, record=True, docker=False, search=True, force=False, localmedia=True, **__): """ - Run a documentation sync or sync n' build. + Run a documentation sync n' build. This is fully wrapped in exception handling to account for a number of failure cases. We first run a few commands in a local build environment, @@ -235,14 +235,13 @@ def run(self, pk, version_pk=None, build_pk=None, record=True, build output page where the build is marked as finished in between the local environment steps and the docker build steps. - If a failure is raised, or the build is not successful, return ``False``, - otherwise, ``True``. + If a failure is raised, or the build is not successful, return + ``False``, otherwise, ``True``. Unhandled exceptions raise a generic user facing error, which directs the user to bug us. It is therefore a benefit to have as few unhandled errors as possible. - :param pk int: Project id :param version_pk int: Project Version id (latest if None) :param build_pk int: Build id (if None, commands are not recorded) @@ -251,7 +250,9 @@ def run(self, pk, version_pk=None, build_pk=None, record=True, :param search bool: update search :param force bool: force Sphinx build :param localmedia bool: update localmedia + :returns: whether build was successful or not + :rtype: bool """ try: From 30b7cefbac7600a68899e2fa4b83f5fb295d2d26 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Fri, 16 Feb 2018 11:20:29 -0500 Subject: [PATCH 26/26] Remove unnecessary initializing variables --- readthedocs/doc_builder/environments.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/readthedocs/doc_builder/environments.py b/readthedocs/doc_builder/environments.py index 7c4c3f621b8..83e784e742c 100644 --- a/readthedocs/doc_builder/environments.py +++ b/readthedocs/doc_builder/environments.py @@ -409,17 +409,13 @@ class BuildEnvironment(BaseEnvironment): def __init__(self, project=None, version=None, build=None, config=None, record=True, environment=None, update_on_success=True): - self.project = project + super(BuildEnvironment, self).__init__(project, environment) self.version = version self.build = build self.config = config self.record = record - self.environment = environment or {} self.update_on_success = update_on_success - # TODO: remove this one, comes from super - self.commands = [] - self.failure = None self.start_time = datetime.utcnow()