More logic changes to error reporting, cleanup

readthedocs/doc_builder/backends/sphinx.py

@@ -18,7 +18,7 @@
 
 from readthedocs.builds import utils as version_utils
 from readthedocs.projects.utils import safe_write
-from readthedocs.projects.exceptions import ProjectImportError
+from readthedocs.projects.exceptions import ProjectConfigurationError
 from readthedocs.restapi.client import api
 
 from ..base import BaseBuilder, restoring_chdir
@@ -40,7 +40,7 @@ def __init__(self, *args, **kwargs):
             self.old_artifact_path = os.path.join(
                 self.project.conf_dir(self.version.slug),
                 self.sphinx_build_dir)
-        except ProjectImportError:
+        except ProjectConfigurationError:
             docs_dir = self.docs_dir()
             self.old_artifact_path = os.path.join(docs_dir, self.sphinx_build_dir)
 
@@ -121,16 +121,22 @@ def append_conf(self, **__):
         """Modify given ``conf.py`` file from a whitelisted user's project."""
         try:
             self.version.get_conf_py_path()
-        except ProjectImportError:
+        except ProjectConfigurationError:
             master_doc = self.create_index(extension='rst')
             self._write_config(master_doc=master_doc)
 
         try:
             outfile_path = self.project.conf_file(self.version.slug)
             outfile = codecs.open(outfile_path, encoding='utf-8', mode='a')
-        except (ProjectImportError, IOError):
+        except (ProjectConfigurationError, IOError):
             trace = sys.exc_info()[2]
-            six.reraise(ProjectImportError('Conf file not found'), None, trace)
+            six.reraise(
+                ProjectConfigurationError(
+                    ProjectConfigurationError.NOT_FOUND
+                ),
+                None,
+                trace
+            )
 
         # Append config to project conf file
         tmpl = template_loader.get_template('doc_builder/conf.py.tmpl')

readthedocs/doc_builder/environments.py

@@ -23,6 +23,7 @@
 from readthedocs.builds.models import BuildCommandResultMixin
 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)
@@ -275,15 +276,17 @@ class BuildEnvironment(object):
     :param build: Build instance
     :param record: Record status of build object
     :param environment: shell environment variables
+    :param finalize: finalize the build by setting a finished state on exit
     """
 
     def __init__(self, project=None, version=None, build=None, record=True,
-                 environment=None):
+                 environment=None, finalize=True):
         self.project = project
         self.version = version
         self.build = build
         self.record = record
         self.environment = environment or {}
+        self.finalize = finalize
 
         self.commands = []
         self.failure = None
@@ -294,7 +297,7 @@ def __enter__(self):
 
     def __exit__(self, exc_type, exc_value, tb):
         ret = self.handle_exception(exc_type, exc_value, tb)
-        self.build['state'] = BUILD_STATE_FINISHED
+        self.update_build(BUILD_STATE_FINISHED)
         log.info(LOG_TEMPLATE
                  .format(project=self.project.slug,
                          version=self.version.slug,
@@ -442,12 +445,13 @@ def update_build(self, state=None):
             if isinstance(val, six.binary_type):
                 self.build[key] = val.decode('utf-8', 'ignore')
 
-        try:
-            api_v2.build(self.build['id']).put(self.build)
-        except HttpClientError as e:
-            log.error("Unable to post a new build: %s", e.content)
-        except Exception:
-            log.exception("Unknown build exception")
+        if self.finalize:
+            try:
+                api_v2.build(self.build['id']).put(self.build)
+            except HttpClientError as e:
+                log.error("Unable to post a new build: %s", e.content)
+            except Exception:
+                log.exception("Unknown build exception")
 
 
 class LocalEnvironment(BuildEnvironment):
@@ -521,8 +525,15 @@ def __enter__(self):
                                       .format(self.container_id))))
                     client = self.get_client()
                     client.remove_container(self.container_id)
-        except DockerAPIError:
+        except (DockerAPIError, ConnectionError):
+            # If there is an exception here, we swallow the exception as this
+            # was just during a sanity check anyways.
             pass
+        except BuildEnvironmentError:
+            # There may have been a problem connecting to Docker altogether, or
+            # some other handled exception here.
+            self.__exit__(*sys.exc_info())
+            raise
 
         # Create the checkout path if it doesn't exist to avoid Docker creation
         if not os.path.exists(self.project.doc_path):
@@ -537,28 +548,40 @@ def __enter__(self):
 
     def __exit__(self, exc_type, exc_value, tb):
         """End of environment context"""
-        ret = self.handle_exception(exc_type, exc_value, tb)
+        try:
+            # Update buildenv state given any container error states first
+            self.update_build_from_container_state()
 
-        # Update buildenv state given any container error states first
-        self.update_build_from_container_state()
+            client = self.get_client()
+            try:
+                client.kill(self.container_id)
+            except DockerAPIError:
+                pass
+            try:
+                log.info('Removing container %s', self.container_id)
+                client.remove_container(self.container_id)
+            # Catch direct failures from Docker API, but also requests exceptions
+            # with the HTTP request. These should not
+            except (DockerAPIError, ConnectionError):
+                log.exception(
+                    LOG_TEMPLATE
+                    .format(
+                        project=self.project.slug,
+                        version=self.version.slug,
+                        msg="Couldn't remove container",
+                    ),
+                )
+            self.container = None
+        except BuildEnvironmentError:
+            # Several interactions with Docker can result in a top level failure
+            # here. We'll catch this and report if there were no reported errors
+            # already. These errors are not as important as a failure at deeper
+            # code
+            if not all([exc_type, exc_value, tb]):
+                exc_type, exc_value, tb = sys.exc_info()
 
-        client = self.get_client()
-        try:
-            client.kill(self.container_id)
-        except DockerAPIError:
-            pass
-        try:
-            log.info('Removing container %s', self.container_id)
-            client.remove_container(self.container_id)
-        except DockerAPIError:
-            log.error(LOG_TEMPLATE
-                      .format(
-                          project=self.project.slug,
-                          version=self.version.slug,
-                          msg="Couldn't remove container"),
-                      exc_info=True)
-        self.container = None
-        self.build['state'] = BUILD_STATE_FINISHED
+        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,
@@ -655,11 +678,22 @@ def create_container(self):
                 mem_limit=self.container_mem_limit,
             )
             client.start(container=self.container_id)
+        except ConnectionError as e:
+            log.exception(
+                LOG_TEMPLATE.format(
+                    project=self.project.slug,
+                    version=self.version.slug,
+                    msg=e,
+                ),
+            )
+            raise BuildEnvironmentError('There was a problem connecting to Docker')
         except DockerAPIError as e:
-            log.error(LOG_TEMPLATE
-                      .format(
-                          project=self.project.slug,
-                          version=self.version.slug,
-                          msg=e.explanation),
-                      exc_info=True)
+            log.exception(
+                LOG_TEMPLATE
+                .format(
+                    project=self.project.slug,
+                    version=self.version.slug,
+                    msg=e.explanation,
+                ),
+            )
             raise BuildEnvironmentError('Build environment creation failed')

readthedocs/doc_builder/exceptions.py

@@ -3,9 +3,15 @@
 
 class BuildEnvironmentException(Exception):
 
-    def __init__(self, *args, **kwargs):
+    message = None
+
+    def __init__(self, message=None, **kwargs):
         self.status_code = kwargs.pop('status_code', 1)
-        super(BuildEnvironmentException, self).__init__(*args, **kwargs)
+        message = message or self.get_default_message()
+        super(BuildEnvironmentException, self).__init__(message, **kwargs)
+
+    def get_default_message(self):
+        return self.message
 
 
 class BuildEnvironmentError(BuildEnvironmentException):

readthedocs/projects/exceptions.py

@@ -1,15 +1,46 @@
 """Project exceptions"""
 
+from django.conf import settings
+from django.utils.translation import ugettext_noop as _
 
-class ProjectImportError (Exception):
+from readthedocs.doc_builder.exceptions import BuildEnvironmentError
 
-    """Failure to import a project from a repository."""
 
-    pass
+class ProjectConfigurationError(BuildEnvironmentError):
+
+    """Error raised trying to configure a project for build"""
+
+    NOT_FOUND = _(
+        'A configuration file was not found. '
+        'Make sure you have a conf.py file in your repository.'
+    )
+
+
+class RepositoryError(BuildEnvironmentError):
+
+    """Failure during repository operation."""
+
+    PRIVATE_REPO = _(
+        'There was a problem connecting to your repository, '
+        'ensure that your repository URL is correct.'
+    )
+    PUBLIC_REPO = _(
+        'There was a problem connecting to your repository, '
+        'ensure that your repository URL is correct and your repository is public.'
+    )
+
+    def get_default_message(self):
+        if settings.ALLOW_PRIVATE_REPOS:
+            return self.PRIVATE_REPO
+        return self.PUBLIC_REPO
 
 
 class ProjectSpamError(Exception):
 
-    """Error raised when a project field has detected spam"""
+    """Error raised when a project field has detected spam
+
+    This error is not raised to users, we use this for banning users in the
+    background.
+    """
 
     pass

readthedocs/projects/models.py

@@ -22,7 +22,7 @@
 from readthedocs.core.utils import broadcast, slugify
 from readthedocs.core.validators import validate_domain_name
 from readthedocs.projects import constants
-from readthedocs.projects.exceptions import ProjectImportError
+from readthedocs.projects.exceptions import ProjectConfigurationError
 from readthedocs.projects.querysets import (
     ProjectQuerySet,
     RelatedProjectQuerySet,
@@ -531,11 +531,9 @@ def conf_file(self, version=LATEST):
         for filename in files:
             if filename.find('doc', 70) != -1:
                 return filename
-        # Having this be translatable causes this odd error:
-        # ProjectImportError(<django.utils.functional.__proxy__ object at
-        # 0x1090cded0>,)
-        raise ProjectImportError(
-            u"Conf File Missing. Please make sure you have a conf.py in your project.")
+        raise ProjectConfigurationError(
+            ProjectConfigurationError.NOT_FOUND
+        )
 
     def conf_dir(self, version=LATEST):
         conf_file = self.conf_file(version)