From 62f44865f55db8ed4b72fb01e3f53258add239e8 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 8 Aug 2018 12:47:20 -0300 Subject: [PATCH 1/7] Minimum refactor to share code --- readthedocs/doc_builder/environments.py | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/readthedocs/doc_builder/environments.py b/readthedocs/doc_builder/environments.py index eb5002a5bd5..c3faf493846 100644 --- a/readthedocs/doc_builder/environments.py +++ b/readthedocs/doc_builder/environments.py @@ -726,13 +726,7 @@ def __exit__(self, exc_type, exc_value, tb): if not all([exc_type, exc_value, tb]): exc_type, exc_value, tb = sys.exc_info() - ret = self.handle_exception(exc_type, exc_value, tb) - self.update_build(BUILD_STATE_FINISHED) - log.info(LOG_TEMPLATE - .format(project=self.project.slug, - version=self.version.slug, - msg='Build finished')) - return ret + return super(DockerBuildEnvironment, self).__exit__(exc_type, exc_value, tb) def get_client(self): """Create Docker client connection.""" From 68fe637649d9d4c3de12a2bb038611824e1140ce Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 8 Aug 2018 13:01:54 -0300 Subject: [PATCH 2/7] Mark BuildEnvironmentError exceptions as Warning and do not log them There are some exceptions risen while building documentation that are considered an ERROR from the Build perspective, but for our application are just a WARNING and shouldn't be logged as ERROR (sent to Sentry). It's not an ERROR in our application but a WARNING that the user's documentation couldn't be built. --- readthedocs/doc_builder/environments.py | 57 +++++++++++++++++-------- readthedocs/doc_builder/exceptions.py | 28 ++++++++++++ readthedocs/projects/tasks.py | 25 ++++------- 3 files changed, 76 insertions(+), 34 deletions(-) diff --git a/readthedocs/doc_builder/environments.py b/readthedocs/doc_builder/environments.py index c3faf493846..9695ef78b94 100644 --- a/readthedocs/doc_builder/environments.py +++ b/readthedocs/doc_builder/environments.py @@ -28,7 +28,7 @@ from requests.exceptions import ConnectionError from .exceptions import (BuildEnvironmentException, BuildEnvironmentError, - BuildEnvironmentWarning, BuildEnvironmentCreationFailed) + BuildEnvironmentWarning, BuildEnvironmentCreationFailed, VersionLockedError, ProjectBuildsSkippedError, YAMLParseError, BuildTimeoutError) from .constants import (DOCKER_SOCKET, DOCKER_VERSION, DOCKER_IMAGE, DOCKER_LIMITS, DOCKER_TIMEOUT_EXIT_CODE, DOCKER_OOM_EXIT_CODE, SPHINX_TEMPLATE_DIR, @@ -40,9 +40,11 @@ __all__ = ( 'api_v2', - 'BuildCommand', 'DockerBuildCommand', + 'BuildCommand', + 'DockerBuildCommand', 'LocalEnvironment', - 'LocalBuildEnvironment', 'DockerBuildEnvironment', + 'LocalBuildEnvironment', + 'DockerBuildEnvironment', ) @@ -409,6 +411,16 @@ class BuildEnvironment(BaseEnvironment): successful """ + # Exceptions considered ERROR from a Build perspective but as a WARNING for + # the application itself. These exception are logged as warning and not sent + # to Sentry. + WARNING_EXCEPTIONS = ( + VersionLockedError, + ProjectBuildsSkippedError, + YAMLParseError, + BuildTimeoutError, + ) + def __init__(self, project=None, version=None, build=None, config=None, record=True, environment=None, update_on_success=True): super(BuildEnvironment, self).__init__(project, environment) @@ -445,21 +457,30 @@ def handle_exception(self, exc_type, exc_value, _): a failure and the context will be gracefully exited. """ if exc_type is not None: - if not issubclass(exc_type, BuildEnvironmentWarning): - log.error(LOG_TEMPLATE - .format(project=self.project.slug, - version=self.version.slug, - msg=exc_value), - exc_info=True, - extra={ - 'stack': True, - 'tags': { - 'build': self.build.get('id'), - 'project': self.project.slug, - 'version': self.version.slug, - }, - }) - self.failure = exc_value + log_level_function = None + if issubclass(exc_type, BuildEnvironmentWarning): + log_level_function = log.warning + elif exc_type in self.WARNING_EXCEPTIONS: + log_level_function = log.warning + else: + log_level_function = log.error + + log_level_function( + LOG_TEMPLATE.format( + project=self.project.slug, + version=self.version.slug, + msg=exc_value, + ), + exc_info=True, + extra={ + 'stack': True, + 'tags': { + 'build': self.build.get('id'), + 'project': self.project.slug, + 'version': self.version.slug, + }, + }) + self.failure = exc_value return True def record_command(self, command): diff --git a/readthedocs/doc_builder/exceptions.py b/readthedocs/doc_builder/exceptions.py index 78a62be6c45..d421bc24289 100644 --- a/readthedocs/doc_builder/exceptions.py +++ b/readthedocs/doc_builder/exceptions.py @@ -31,5 +31,33 @@ class BuildEnvironmentCreationFailed(BuildEnvironmentError): ) +class VersionLockedError(BuildEnvironmentError): + + message = ugettext_noop( + 'Version locked, retrying in 5 minutes.', + ) + + +class ProjectBuildsSkippedError(BuildEnvironmentError): + + message = ugettext_noop( + 'Builds for this project are temporarily disabled', + ) + + +class YAMLParseError(BuildEnvironmentError): + + GENERIC_WITH_PARSE_EXCEPTION = ugettext_noop( + 'Problem parsing YAML configuration. {exception}', + ) + + +class BuildTimeoutError(BuildEnvironmentError): + + message = ugettext_noop( + 'Build exited due to time out', + ) + + class BuildEnvironmentWarning(BuildEnvironmentException): pass diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index 871857b7e48..ca8ed7e66df 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -45,7 +45,7 @@ from readthedocs.doc_builder.constants import DOCKER_LIMITS from readthedocs.doc_builder.environments import (LocalBuildEnvironment, DockerBuildEnvironment) -from readthedocs.doc_builder.exceptions import BuildEnvironmentError +from readthedocs.doc_builder.exceptions import BuildEnvironmentError, VersionLockedError, ProjectBuildsSkippedError, YAMLParseError, BuildTimeoutError from readthedocs.doc_builder.loader import get_builder_class from readthedocs.doc_builder.python_environments import Virtualenv, Conda from readthedocs.projects.models import APIProject @@ -409,23 +409,19 @@ def run_setup(self, record=True): # Environment used for code checkout & initial configuration reading with self.setup_env: if self.project.skip: - raise BuildEnvironmentError( - _('Builds for this project are temporarily disabled')) + raise ProjectBuildsSkippedError try: self.setup_vcs() except vcs_support_utils.LockTimeout as e: self.task.retry(exc=e, throw=False) - raise BuildEnvironmentError( - 'Version locked, retrying in 5 minutes.', - status_code=423 - ) - + raise VersionLockedError try: self.config = load_yaml_config(version=self.version) except ConfigError as e: - raise BuildEnvironmentError( - 'Problem parsing YAML configuration. {0}'.format(str(e)) - ) + raise YAMLParseError( + YAMLParseError.GENERIC_WITH_PARSE_EXCEPTION.format( + exception=str(e), + )) if self.setup_env.failure or self.config is None: self._log('Failing build because of setup failure: %s' % self.setup_env.failure) @@ -485,12 +481,9 @@ def run_build(self, docker, record): build_id = self.build.get('id') except vcs_support_utils.LockTimeout as e: self.task.retry(exc=e, throw=False) - raise BuildEnvironmentError( - 'Version locked, retrying in 5 minutes.', - status_code=423 - ) + raise VersionLockedError except SoftTimeLimitExceeded: - raise BuildEnvironmentError(_('Build exited due to time out')) + raise BuildTimeoutError # Finalize build and update web servers if build_id: From 0a13e9067012e9db0c0eb0cd1c74b392751edd4d Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 8 Aug 2018 13:09:29 -0300 Subject: [PATCH 3/7] Style by `pre-commit run` Note: the changes were selected manually since there were many that break the style that we want :( --- readthedocs/doc_builder/environments.py | 160 ++++++++++++-------- readthedocs/doc_builder/exceptions.py | 23 ++- readthedocs/projects/tasks.py | 190 ++++++++++++++++-------- 3 files changed, 235 insertions(+), 138 deletions(-) diff --git a/readthedocs/doc_builder/environments.py b/readthedocs/doc_builder/environments.py index 9695ef78b94..ca0fd77574f 100644 --- a/readthedocs/doc_builder/environments.py +++ b/readthedocs/doc_builder/environments.py @@ -2,22 +2,26 @@ """Documentation Builder Environments.""" -from __future__ import absolute_import -from builtins import str -from builtins import object +from __future__ import ( + absolute_import, division, print_function, unicode_literals) + +import logging import os import re -import sys -import logging +import socket import subprocess +import sys import traceback -import socket from datetime import datetime +import six +from builtins import object, str from django.conf import settings from django.utils.translation import ugettext_lazy as _ from docker import APIClient -from docker.errors import APIError as DockerAPIError, DockerException +from docker.errors import APIError as DockerAPIError +from docker.errors import DockerException +from requests.exceptions import ConnectionError from slumber.exceptions import HttpClientError from readthedocs.builds.constants import BUILD_STATE_FINISHED @@ -25,19 +29,18 @@ from readthedocs.core.utils import slugify from readthedocs.projects.constants import LOG_TEMPLATE from readthedocs.restapi.client import api as api_v2 -from requests.exceptions import ConnectionError -from .exceptions import (BuildEnvironmentException, BuildEnvironmentError, - BuildEnvironmentWarning, BuildEnvironmentCreationFailed, VersionLockedError, ProjectBuildsSkippedError, YAMLParseError, BuildTimeoutError) -from .constants import (DOCKER_SOCKET, DOCKER_VERSION, DOCKER_IMAGE, - DOCKER_LIMITS, DOCKER_TIMEOUT_EXIT_CODE, - DOCKER_OOM_EXIT_CODE, SPHINX_TEMPLATE_DIR, - MKDOCS_TEMPLATE_DIR, DOCKER_HOSTNAME_MAX_LEN) -import six +from .constants import ( + DOCKER_HOSTNAME_MAX_LEN, DOCKER_IMAGE, DOCKER_LIMITS, DOCKER_OOM_EXIT_CODE, + DOCKER_SOCKET, DOCKER_TIMEOUT_EXIT_CODE, DOCKER_VERSION, + MKDOCS_TEMPLATE_DIR, SPHINX_TEMPLATE_DIR) +from .exceptions import ( + BuildEnvironmentCreationFailed, BuildEnvironmentError, + BuildEnvironmentException, BuildEnvironmentWarning, BuildTimeoutError, + ProjectBuildsSkippedError, VersionLockedError, YAMLParseError) log = logging.getLogger(__name__) - __all__ = ( 'api_v2', 'BuildCommand', @@ -220,8 +223,12 @@ def run(self): :type cmd_input: str :param combine_output: combine STDERR into STDOUT """ - log.info("Running in container %s: '%s' [%s]", - self.build_env.container_id, self.get_command(), self.cwd) + log.info( + "Running in container %s: '%s' [%s]", + self.build_env.container_id, + self.get_command(), + self.cwd, + ) self.start_time = datetime.utcnow() client = self.build_env.get_client() @@ -230,7 +237,7 @@ def run(self): container=self.build_env.container_id, cmd=self.get_wrapped_command(), stdout=True, - stderr=True + stderr=True, ) output = client.exec_start(exec_id=exec_cmd['Id'], stream=False) @@ -439,10 +446,13 @@ def __enter__(self): def __exit__(self, exc_type, exc_value, tb): ret = self.handle_exception(exc_type, exc_value, tb) self.update_build(BUILD_STATE_FINISHED) - log.info(LOG_TEMPLATE - .format(project=self.project.slug, - version=self.version.slug, - msg='Build finished')) + log.info( + LOG_TEMPLATE.format( + project=self.project.slug, + version=self.version.slug, + msg='Build finished', + ) + ) return ret def handle_exception(self, exc_type, exc_value, _): @@ -479,7 +489,8 @@ def handle_exception(self, exc_type, exc_value, _): 'project': self.project.slug, 'version': self.version.slug, }, - }) + }, + ) self.failure = exc_value return True @@ -488,11 +499,13 @@ def record_command(self, command): def _log_warning(self, msg): # :'( - log.warning(LOG_TEMPLATE.format( - project=self.project.slug, - version=self.version.slug, - msg=msg, - )) + log.warning( + LOG_TEMPLATE.format( + project=self.project.slug, + version=self.version.slug, + msg=msg, + ) + ) def run(self, *cmd, **kwargs): kwargs.update({ @@ -550,15 +563,18 @@ def update_build(self, state=None): # TODO drop exit_code and provide a more meaningful UX for error # reporting - if self.failure and isinstance(self.failure, - BuildEnvironmentException): + if self.failure and isinstance( + self.failure, + BuildEnvironmentException, + ): self.build['exit_code'] = self.failure.status_code elif self.commands: - self.build['exit_code'] = max([cmd.exit_code - for cmd in self.commands]) + self.build['exit_code'] = max([ + cmd.exit_code for cmd in self.commands + ]) - self.build['setup'] = self.build['setup_error'] = "" - self.build['output'] = self.build['error'] = "" + self.build['setup'] = self.build['setup_error'] = '' + self.build['output'] = self.build['error'] = '' if self.start_time: build_length = (datetime.utcnow() - self.start_time) @@ -567,9 +583,13 @@ def update_build(self, state=None): if self.failure is not None: # Surface a generic error if the class is not a # BuildEnvironmentError - if not isinstance(self.failure, - (BuildEnvironmentException, - BuildEnvironmentWarning)): + if not isinstance( + self.failure, + ( + BuildEnvironmentException, + BuildEnvironmentWarning, + ), + ): log.error( 'Build failed with unhandled exception: %s', str(self.failure), @@ -585,7 +605,7 @@ def update_build(self, state=None): self.failure = BuildEnvironmentError( BuildEnvironmentError.GENERIC_WITH_BUILD_ID.format( build_id=self.build['id'], - ) + ), ) self.build['error'] = str(self.failure) @@ -608,11 +628,11 @@ def update_build(self, state=None): api_v2.build(self.build['id']).put(self.build) except HttpClientError as e: log.exception( - "Unable to update build: id=%d", + 'Unable to update build: id=%d', self.build['id'], ) except Exception: - log.exception("Unknown build exception") + log.exception('Unknown build exception') class LocalBuildEnvironment(BuildEnvironment): @@ -653,7 +673,7 @@ def __init__(self, *args, **kwargs): build=self.build.get('id'), project_id=self.project.pk, project_name=self.project.slug, - )[:DOCKER_HOSTNAME_MAX_LEN] + )[:DOCKER_HOSTNAME_MAX_LEN], ) if self.config and self.config.build_image: self.container_image = self.config.build_image @@ -675,18 +695,25 @@ def __enter__(self): if state is not None: if state.get('Running') is True: exc = BuildEnvironmentError( - _('A build environment is currently ' - 'running for this version')) + _( + 'A build environment is currently ' + 'running for this version', + ), + ) self.failure = exc self.build['state'] = BUILD_STATE_FINISHED raise exc else: - log.warning(LOG_TEMPLATE - .format( - project=self.project.slug, - version=self.version.slug, - msg=("Removing stale container {0}" - .format(self.container_id)))) + log.warning( + LOG_TEMPLATE.format( + project=self.project.slug, + version=self.version.slug, + msg=( + 'Removing stale container {0}' + .format(self.container_id) + ), + ) + ) client = self.get_client() client.remove_container(self.container_id) except (DockerAPIError, ConnectionError): @@ -731,8 +758,7 @@ def __exit__(self, exc_type, exc_value, tb): # request. These errors should not surface to the user. except (DockerAPIError, ConnectionError): log.exception( - LOG_TEMPLATE - .format( + LOG_TEMPLATE.format( project=self.project.slug, version=self.version.slug, msg="Couldn't remove container", @@ -772,7 +798,7 @@ def get_client(self): raise BuildEnvironmentError( BuildEnvironmentError.GENERIC_WITH_BUILD_ID.format( build_id=self.build['id'], - ) + ), ) def get_container_host_config(self): @@ -851,14 +877,18 @@ def update_build_from_container_state(self): if state is not None and state.get('Running') is False: if state.get('ExitCode') == DOCKER_TIMEOUT_EXIT_CODE: self.failure = BuildEnvironmentError( - _('Build exited due to time out')) + _('Build exited due to time out'), + ) elif state.get('OOMKilled', False): self.failure = BuildEnvironmentError( - _('Build exited due to excessive memory consumption')) + _('Build exited due to excessive memory consumption'), + ) elif state.get('Error'): - self.failure = BuildEnvironmentError( - (_('Build exited due to unknown error: {0}') - .format(state.get('Error')))) + self.failure = BuildEnvironmentError(( + _('Build exited due to unknown error: {0}') + .format(state.get('Error')) + ), + ) def create_container(self): """Create docker container.""" @@ -870,9 +900,12 @@ def create_container(self): ) self.container = client.create_container( image=self.container_image, - command=('/bin/sh -c "sleep {time}; exit {exit}"' - .format(time=self.container_time_limit, - exit=DOCKER_TIMEOUT_EXIT_CODE)), + command=( + '/bin/sh -c "sleep {time}; exit {exit}"'.format( + time=self.container_time_limit, + exit=DOCKER_TIMEOUT_EXIT_CODE, + ) + ), name=self.container_id, hostname=self.container_id, host_config=self.get_container_host_config(), @@ -897,12 +930,11 @@ def create_container(self): raise BuildEnvironmentError( BuildEnvironmentError.GENERIC_WITH_BUILD_ID.format( build_id=self.build['id'], - ) + ), ) except DockerAPIError as e: log.exception( - LOG_TEMPLATE - .format( + LOG_TEMPLATE.format( project=self.project.slug, version=self.version.slug, msg=e.explanation, diff --git a/readthedocs/doc_builder/exceptions.py b/readthedocs/doc_builder/exceptions.py index d421bc24289..d9b548052a7 100644 --- a/readthedocs/doc_builder/exceptions.py +++ b/readthedocs/doc_builder/exceptions.py @@ -1,5 +1,8 @@ +# -*- coding: utf-8 -*- """Exceptions raised when building documentation.""" +from __future__ import division, print_function, unicode_literals + from django.utils.translation import ugettext_noop @@ -19,30 +22,24 @@ def get_default_message(self): class BuildEnvironmentError(BuildEnvironmentException): GENERIC_WITH_BUILD_ID = ugettext_noop( - "There was a problem with Read the Docs while building your documentation. " - "Please report this to us with your build id ({build_id})." + 'There was a problem with Read the Docs while building your documentation. ' + 'Please report this to us with your build id ({build_id}).', ) class BuildEnvironmentCreationFailed(BuildEnvironmentError): - message = ugettext_noop( - "Build environment creation failed" - ) + message = ugettext_noop('Build environment creation failed') class VersionLockedError(BuildEnvironmentError): - message = ugettext_noop( - 'Version locked, retrying in 5 minutes.', - ) + message = ugettext_noop('Version locked, retrying in 5 minutes.') class ProjectBuildsSkippedError(BuildEnvironmentError): - message = ugettext_noop( - 'Builds for this project are temporarily disabled', - ) + message = ugettext_noop('Builds for this project are temporarily disabled') class YAMLParseError(BuildEnvironmentError): @@ -54,9 +51,7 @@ class YAMLParseError(BuildEnvironmentError): class BuildTimeoutError(BuildEnvironmentError): - message = ugettext_noop( - 'Build exited due to time out', - ) + message = ugettext_noop('Build exited due to time out') class BuildEnvironmentWarning(BuildEnvironmentException): diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index ca8ed7e66df..c41154c5f9a 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -1,3 +1,4 @@ +# -*- coding: utf-8 -*- """ Tasks related to projects. @@ -25,29 +26,27 @@ from django.core.urlresolvers import reverse from django.db.models import Q from django.utils.translation import ugettext_lazy as _ -from readthedocs.config import ConfigError from slumber.exceptions import HttpClientError -from .constants import LOG_TEMPLATE -from .exceptions import RepositoryError -from .models import ImportedFile, Project, Domain, Feature -from .signals import before_vcs, after_vcs, before_build, after_build, files_changed from readthedocs.builds.constants import ( BUILD_STATE_BUILDING, BUILD_STATE_CLONING, BUILD_STATE_FINISHED, BUILD_STATE_INSTALLING, LATEST, LATEST_VERBOSE_NAME, STABLE_VERBOSE_NAME) from readthedocs.builds.models import APIVersion, Build, Version from readthedocs.builds.signals import build_complete from readthedocs.builds.syncers import Syncer +from readthedocs.config import ConfigError from readthedocs.core.resolver import resolve_path from readthedocs.core.symlink import PublicSymlink, PrivateSymlink 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 (LocalBuildEnvironment, - DockerBuildEnvironment) -from readthedocs.doc_builder.exceptions import BuildEnvironmentError, VersionLockedError, ProjectBuildsSkippedError, YAMLParseError, BuildTimeoutError +from readthedocs.doc_builder.environments import ( + DockerBuildEnvironment, LocalBuildEnvironment) +from readthedocs.doc_builder.exceptions import ( + BuildEnvironmentError, BuildTimeoutError, ProjectBuildsSkippedError, + VersionLockedError, YAMLParseError) from readthedocs.doc_builder.loader import get_builder_class -from readthedocs.doc_builder.python_environments import Virtualenv, Conda +from readthedocs.doc_builder.python_environments import Conda, Virtualenv from readthedocs.projects.models import APIProject from readthedocs.restapi.client import api as api_v2 from readthedocs.restapi.utils import index_search_request @@ -55,6 +54,11 @@ from readthedocs.vcs_support import utils as vcs_support_utils from readthedocs.worker import app +from .constants import LOG_TEMPLATE +from .exceptions import RepositoryError +from .models import Domain, Feature, ImportedFile, Project +from .signals import ( + after_build, after_vcs, before_build, before_vcs, files_changed) log = logging.getLogger(__name__) @@ -455,8 +459,14 @@ def run_build(self, docker, record): env_cls = DockerBuildEnvironment else: 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) + self.build_env = env_cls( + project=self.project, + version=self.version, + config=self.config, + build=self.build, + record=record, + environment=env_vars, + ) # Environment used for building code, usually with Docker with self.build_env: @@ -468,9 +478,11 @@ def run_build(self, docker, record): if self.config.use_conda: self._log('Using conda') python_env_cls = Conda - self.python_env = python_env_cls(version=self.version, - build_env=self.build_env, - config=self.config) + self.python_env = python_env_cls( + version=self.version, + build_env=self.build_env, + config=self.config, + ) try: self.setup_python_environment() @@ -706,11 +718,11 @@ def build_docs_search(self): """ Build search data with separate build. - Unless the project has the feature to allow - building the JSON search artifacts in the html build step. + Unless the project has the feature to allow building the JSON search + artifacts in the html build step. """ build_json_in_html_builder = self.project.has_feature( - Feature.BUILD_JSON_ARTIFACTS_WITH_HTML + Feature.BUILD_JSON_ARTIFACTS_WITH_HTML, ) if self.build_search and build_json_in_html_builder: # Already built in the html step @@ -753,7 +765,10 @@ def build_docs_class(self, builder_class): only raise a warning exception here. A hard error will halt the build process. """ - builder = get_builder_class(builder_class)(self.build_env, python_env=self.python_env) + builder = get_builder_class(builder_class)( + self.build_env, + python_env=self.python_env + ) success = builder.build() builder.move() return success @@ -828,42 +843,69 @@ def move_files(version_pk, hostname, html=False, localmedia=False, search=False, :type epub: bool """ version = Version.objects.get(pk=version_pk) - log.debug(LOG_TEMPLATE.format(project=version.project.slug, version=version.slug, - msg='Moving files')) + log.debug( + LOG_TEMPLATE.format( + project=version.project.slug, + version=version.slug, + msg='Moving files', + ) + ) if html: from_path = version.project.artifact_path( - version=version.slug, type_=version.project.documentation_type) + version=version.slug, + type_=version.project.documentation_type, + ) target = version.project.rtd_build_path(version.slug) Syncer.copy(from_path, target, host=hostname) if 'sphinx' in version.project.documentation_type: if search: from_path = version.project.artifact_path( - version=version.slug, type_='sphinx_search') + version=version.slug, + type_='sphinx_search', + ) to_path = version.project.get_production_media_path( - type_='json', version_slug=version.slug, include_file=False) + type_='json', + version_slug=version.slug, + include_file=False, + ) Syncer.copy(from_path, to_path, host=hostname) if localmedia: from_path = version.project.artifact_path( - version=version.slug, type_='sphinx_localmedia') + version=version.slug, + type_='sphinx_localmedia', + ) to_path = version.project.get_production_media_path( - type_='htmlzip', version_slug=version.slug, include_file=False) + type_='htmlzip', + version_slug=version.slug, + include_file=False, + ) Syncer.copy(from_path, to_path, host=hostname) # Always move PDF's because the return code lies. if pdf: - from_path = version.project.artifact_path(version=version.slug, - type_='sphinx_pdf') + from_path = version.project.artifact_path( + version=version.slug, + type_='sphinx_pdf', + ) to_path = version.project.get_production_media_path( - type_='pdf', version_slug=version.slug, include_file=False) + type_='pdf', + version_slug=version.slug, + include_file=False, + ) Syncer.copy(from_path, to_path, host=hostname) if epub: - from_path = version.project.artifact_path(version=version.slug, - type_='sphinx_epub') + from_path = version.project.artifact_path( + version=version.slug, + type_='sphinx_epub', + ) to_path = version.project.get_production_media_path( - type_='epub', version_slug=version.slug, include_file=False) + type_='epub', + version_slug=version.slug, + include_file=False, + ) Syncer.copy(from_path, to_path, host=hostname) @@ -968,22 +1010,36 @@ def fileify(version_pk, commit): project = version.project if not commit: - log.info(LOG_TEMPLATE - .format(project=project.slug, version=version.slug, - msg=('Imported File not being built because no commit ' - 'information'))) + log.info( + LOG_TEMPLATE.format( + project=project.slug, + version=version.slug, + msg=( + 'Imported File not being built because no commit ' + 'information' + ), + ) + ) return path = project.rtd_build_path(version.slug) if path: - log.info(LOG_TEMPLATE - .format(project=version.project.slug, version=version.slug, - msg='Creating ImportedFiles')) + log.info( + LOG_TEMPLATE.format( + project=version.project.slug, + version=version.slug, + msg='Creating ImportedFiles', + ) + ) _manage_imported_files(version, path, commit) else: - log.info(LOG_TEMPLATE - .format(project=project.slug, version=version.slug, - msg='No ImportedFile files')) + log.info( + LOG_TEMPLATE.format( + project=project.slug, + version=version.slug, + msg='No ImportedFile files', + ) + ) def _manage_imported_files(version, path, commit): @@ -1049,8 +1105,13 @@ def email_notification(version, build, email): :param build: :py:class:`Build` instance that failed :param email: Email recipient address """ - log.debug(LOG_TEMPLATE.format(project=version.project.slug, version=version.slug, - msg='sending email to: %s' % email)) + log.debug( + LOG_TEMPLATE.format( + project=version.project.slug, + version=version.slug, + msg='sending email to: %s' % email, + ) + ) # We send only what we need from the Django model objects here to avoid # serialization problems in the ``readthedocs.core.tasks.send_email_task`` @@ -1106,11 +1167,15 @@ def webhook_notification(version, build, hook_url): 'id': build.id, 'success': build.success, 'date': build.date.strftime('%Y-%m-%d %H:%M:%S'), - } + }, }) - log.debug(LOG_TEMPLATE - .format(project=project.slug, version='', - msg='sending notification to: %s' % hook_url)) + log.debug( + LOG_TEMPLATE.format( + project=project.slug, + version='', + msg='sending notification to: %s' % hook_url, + ) + ) try: requests.post(hook_url, data=data) except Exception: @@ -1137,11 +1202,13 @@ def update_static_metadata(project_pk, path=None): if not path: path = project.static_metadata_path() - log.info(LOG_TEMPLATE.format( - project=project.slug, - version='', - msg='Updating static metadata', - )) + log.info( + LOG_TEMPLATE.format( + project=project.slug, + version='', + msg='Updating static metadata', + ) + ) translations = [trans.language for trans in project.translations.all()] languages = set(translations) # Convert to JSON safe types @@ -1158,11 +1225,13 @@ def update_static_metadata(project_pk, path=None): json.dump(metadata, fh) fh.close() except (AttributeError, IOError) as e: - log.debug(LOG_TEMPLATE.format( - project=project.slug, - version='', - msg='Cannot write to metadata.json: {0}'.format(e) - )) + log.debug( + LOG_TEMPLATE.format( + project=project.slug, + version='', + msg='Cannot write to metadata.json: {0}'.format(e), + ) + ) # Random Tasks @@ -1174,7 +1243,7 @@ def remove_dir(path): This is mainly a wrapper around shutil.rmtree so that app servers can kill things on the build server. """ - log.info("Removing %s", path) + log.info('Removing %s', path) shutil.rmtree(path, ignore_errors=True) @@ -1224,7 +1293,8 @@ def finish_inactive_builds(): if build.project.container_time_limit: custom_delta = datetime.timedelta( - seconds=int(build.project.container_time_limit)) + seconds=int(build.project.container_time_limit), + ) if build.date + custom_delta > datetime.datetime.now(): # Do not mark as FINISHED builds with a custom time limit that wasn't # expired yet (they are still building the project version) @@ -1235,7 +1305,7 @@ def finish_inactive_builds(): build.error = _( 'This build was terminated due to inactivity. If you ' 'continue to encounter this error, file a support ' - 'request with and reference this build id ({0}).'.format(build.pk) + 'request with and reference this build id ({0}).'.format(build.pk), ) build.save() builds_finished += 1 From 1c8a7b4d23ed642e3e557e04931c09c5b87e318f Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 8 Aug 2018 16:05:06 -0300 Subject: [PATCH 4/7] Mantain the status code for VersionLockedError exception --- readthedocs/doc_builder/exceptions.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/readthedocs/doc_builder/exceptions.py b/readthedocs/doc_builder/exceptions.py index d9b548052a7..360e2845256 100644 --- a/readthedocs/doc_builder/exceptions.py +++ b/readthedocs/doc_builder/exceptions.py @@ -9,9 +9,10 @@ class BuildEnvironmentException(Exception): message = None + status_code = None def __init__(self, message=None, **kwargs): - self.status_code = kwargs.pop('status_code', 1) + self.status_code = kwargs.pop('status_code', None) or self.status_code or 1 message = message or self.get_default_message() super(BuildEnvironmentException, self).__init__(message, **kwargs) @@ -35,6 +36,7 @@ class BuildEnvironmentCreationFailed(BuildEnvironmentError): class VersionLockedError(BuildEnvironmentError): message = ugettext_noop('Version locked, retrying in 5 minutes.') + status_code = 423 class ProjectBuildsSkippedError(BuildEnvironmentError): From 25fc38d839f5d8148dbb2be722b0484021ab999c Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 8 Aug 2018 16:23:00 -0300 Subject: [PATCH 5/7] Do not mark the build as failure when it's a BuildEnvironmentWarning --- 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 ca0fd77574f..fd126d2224b 100644 --- a/readthedocs/doc_builder/environments.py +++ b/readthedocs/doc_builder/environments.py @@ -472,8 +472,10 @@ def handle_exception(self, exc_type, exc_value, _): log_level_function = log.warning elif exc_type in self.WARNING_EXCEPTIONS: log_level_function = log.warning + self.failure = exc_value else: log_level_function = log.error + self.failure = exc_value log_level_function( LOG_TEMPLATE.format( @@ -491,7 +493,6 @@ def handle_exception(self, exc_type, exc_value, _): }, }, ) - self.failure = exc_value return True def record_command(self, command): From ba3f9df2e0f578c119814ba00a1c5f7220243489 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 8 Aug 2018 16:23:20 -0300 Subject: [PATCH 6/7] Improve docstring --- readthedocs/doc_builder/environments.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/readthedocs/doc_builder/environments.py b/readthedocs/doc_builder/environments.py index fd126d2224b..004c04dcd8a 100644 --- a/readthedocs/doc_builder/environments.py +++ b/readthedocs/doc_builder/environments.py @@ -457,14 +457,18 @@ def __exit__(self, exc_type, exc_value, tb): def handle_exception(self, exc_type, exc_value, _): """ - Exception handling for __enter__ and __exit__ + Exception handling for __enter__ and __exit__. This reports on the exception we're handling and special cases - subclasses of BuildEnvironmentException. For + subclasses of BuildEnvironmentException. For :py:class:`BuildEnvironmentWarning`, exit this context gracefully, but - don't mark the build as a failure. For all other exception classes, + don't mark the build as a failure. For all other exception classes, including :py:class:`BuildEnvironmentError`, the build will be marked as a failure and the context will be gracefully exited. + + If the exception's type is :py:class:`BuildEnvironmentWarning` or it's + an exception marked as ``WARNING_EXCEPTIONS`` we log the problem as a + WARNING, otherwise we log it as an ERROR. """ if exc_type is not None: log_level_function = None From 29f1f65517213349e3426b751c6c3344830c0fee Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 8 Aug 2018 16:45:48 -0300 Subject: [PATCH 7/7] Lint --- readthedocs/doc_builder/environments.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/readthedocs/doc_builder/environments.py b/readthedocs/doc_builder/environments.py index 004c04dcd8a..0a2d570ba8b 100644 --- a/readthedocs/doc_builder/environments.py +++ b/readthedocs/doc_builder/environments.py @@ -590,10 +590,10 @@ def update_build(self, state=None): # BuildEnvironmentError if not isinstance( self.failure, - ( - BuildEnvironmentException, - BuildEnvironmentWarning, - ), + ( + BuildEnvironmentException, + BuildEnvironmentWarning, + ), ): log.error( 'Build failed with unhandled exception: %s',