Encapsulate command execution inside build environment state objects

These objects will allow for multiple command execution patterns without
changing the actual commands we're executing. This allows us to use Docker as a
thin wrapper around container execution, instead of treating the Docker
containers as a black box. This works towards per-command tracking of commands
as well, giving us a far better UI around build state.

Author: agjohnson

readthedocs/builds/constants.py

@@ -1,11 +1,17 @@
 from django.utils.translation import ugettext_lazy as _
 
+BUILD_STATE_TRIGGERED = 'triggered'
+BUILD_STATE_CLONING = 'cloning'
+BUILD_STATE_INSTALLING = 'installing'
+BUILD_STATE_BUILDING = 'building'
+BUILD_STATE_FINISHED = 'finished'
+
 BUILD_STATE = (
-    ('triggered', _('Triggered')),
-    ('cloning', _('Cloning')),
-    ('installing', _('Installing')),
-    ('building', _('Building')),
-    ('finished', _('Finished')),
+    (BUILD_STATE_TRIGGERED, _('Triggered')),
+    (BUILD_STATE_CLONING, _('Cloning')),
+    (BUILD_STATE_INSTALLING, _('Installing')),
+    (BUILD_STATE_BUILDING, _('Building')),
+    (BUILD_STATE_FINISHED, _('Finished')),
 )
 
 BUILD_TYPES = (

readthedocs/core/management/commands/update_api.py

@@ -20,4 +20,5 @@ def handle(self, *args, **options):
                 project_data = api.project(slug).get()
                 p = tasks.make_api_project(project_data)
                 log.info("Building %s" % p)
-                tasks.update_docs(pk=p.pk)
+                update_docs = tasks.UpdateDocsTask()
+                update_docs.run(pk=p.pk)

readthedocs/core/management/commands/update_docker.py

@@ -20,4 +20,6 @@ def handle(self, *args, **options):
                 project_data = api.project(slug).get()
                 p = tasks.make_api_project(project_data)
                 log.info("Building %s" % p)
-                tasks.update_docs(pk=p.pk, docker=True)
+                # TODO just move this to update_api.py
+                update_docs = tasks.UpdateDocsTask()
+                update_docs.run(pk=p.pk, docker=True)

readthedocs/core/management/commands/update_repos.py

@@ -48,9 +48,10 @@ def handle(self, *args, **options):
                     for version in Version.objects.filter(project__slug=slug,
                                                           active=True,
                                                           uploaded=False):
-                        tasks.update_docs(pk=version.project_id,
-                                          record=False,
-                                          version_pk=version.pk)
+                        update_docs = tasks.UpdateDocsTask()
+                        update_docs.run(pk=version.project_id,
+                                        record=False,
+                                        version_pk=version.pk)
                 else:
                     p = Project.all_objects.get(slug=slug)
                     log.info("Building %s" % p)
@@ -60,14 +61,15 @@ def handle(self, *args, **options):
                 log.info("Updating all versions")
                 for version in Version.objects.filter(active=True,
                                                       uploaded=False):
-                    tasks.update_docs(pk=version.project_id,
-                                      record=record,
-                                      force=force,
-                                      version_pk=version.pk)
+                    update_tasks = tasks.UpdateDocsTask()
+                    update_tasks.run(pk=version.project_id,
+                                     record=record,
+                                     force=force,
+                                     version_pk=version.pk)
             else:
                 log.info("Updating all docs")
-                tasks.update_docs_pull(record=record,
-                                       force=force)
+                update_docs = tasks.UpdateDocsTask()
+                update_docs.run(record=record, force=force)
 
     @property
     def help(self):

readthedocs/core/management/commands/update_versions.py

@@ -1,6 +1,7 @@
-from readthedocs.builds.models import Version
 from django.core.management.base import BaseCommand
-from readthedocs.projects.tasks import update_docs
+
+from readthedocs.builds.models import Version
+from readthedocs.projects.tasks import UpdateDocsTask
 
 
 class Command(BaseCommand):
@@ -10,5 +11,6 @@ class Command(BaseCommand):
 
     def handle(self, *args, **options):
         for version in Version.objects.filter(active=True, built=False):
-            update_docs(version.project_id, record=False,
-                        version_pk=version.pk)
+            update_docs = UpdateDocsTask()
+            update_docs.run(version.project_id, record=False,
+                            version_pk=version.pk)

readthedocs/core/utils/__init__.py

@@ -59,7 +59,7 @@ def trigger_build(project, version=None, record=True, force=False, basic=False):
     An API to wrap the triggering of a build.
     """
     # Avoid circular import
-    from readthedocs.projects.tasks import update_docs
+    from readthedocs.projects.tasks import UpdateDocsTask
 
     if project.skip:
         return None
@@ -75,10 +75,12 @@ def trigger_build(project, version=None, record=True, force=False, basic=False):
             state='triggered',
             success=True,
         )
+        update_docs = UpdateDocsTask()
         update_docs.delay(pk=project.pk, version_pk=version.pk, record=record,
                           force=force, basic=basic, build_pk=build.pk)
     else:
         build = None
+        update_docs = UpdateDocsTask()
         update_docs.delay(pk=project.pk, version_pk=version.pk, record=record,
                           force=force, basic=basic)
 

readthedocs/doc_builder/backends/mkdocs.py

@@ -7,7 +7,6 @@
 from django.template import Context, loader as template_loader
 
 from readthedocs.doc_builder.base import BaseBuilder, restoring_chdir
-from readthedocs.projects.utils import run
 
 log = logging.getLogger(__name__)
 
@@ -127,21 +126,18 @@ def append_conf(self, **kwargs):
         include_file.write(include_string)
         include_file.close()
 
-    @restoring_chdir
     def build(self, **kwargs):
         checkout_path = self.version.project.checkout_path(self.version.slug)
-        # site_path = os.path.join(checkout_path, 'site')
-        os.chdir(checkout_path)
-        # Actual build
-        build_command = (
-            "{command} {builder} --clean --site-dir={build_dir} --theme=readthedocs"
-            .format(
-                command=self.version.project.venv_bin(version=self.version.slug, bin='mkdocs'),
-                builder=self.builder,
-                build_dir=self.build_dir,
-            ))
-        results = run(build_command, shell=True)
-        return results
+        # TODO does this have the self.project?
+        # TODO --theme fails on the JSON builder
+        build_command = [
+            self.project.venv_bin(version=self.version.slug, bin='mkdocs'),
+            self.builder,
+            '--clean',
+            '--site-dir', self.build_dir,
+            '--theme', 'readthedocs',
+        ]
+        self.run(*build_command, cwd=checkout_path)
 
 
 class MkdocsHTML(BaseMkdocs):

readthedocs/doc_builder/backends/sphinx.py

@@ -12,7 +12,7 @@
 
 from readthedocs.builds import utils as version_utils
 from readthedocs.doc_builder.base import BaseBuilder, restoring_chdir
-from readthedocs.projects.utils import run, safe_write
+from readthedocs.projects.utils import safe_write
 from readthedocs.projects.exceptions import ProjectImportError
 from readthedocs.restapi.client import api
 
@@ -123,18 +123,20 @@ def append_conf(self, **kwargs):
     def build(self, **kwargs):
         self.clean()
         project = self.version.project
-        os.chdir(project.conf_dir(self.version.slug))
-        force_str = " -E " if self._force else ""
-        build_command = "%s -T %s -b %s -d _build/doctrees -D language=%s . %s " % (
-            project.venv_bin(version=self.version.slug,
-                             bin='sphinx-build'),
-            force_str,
-            self.sphinx_builder,
-            project.language,
-            self.sphinx_build_dir,
-        )
-        results = run(build_command, shell=True)
-        return results
+        build_command = [
+            project.venv_bin(version=self.version.slug, bin='sphinx-build'),
+            '-T'
+        ]
+        if self._force:
+            build_command.append('-E')
+        build_command.extend([
+            '-b', self.sphinx_builder,
+            '-d', '_build/doctrees',
+            '-D', 'language={lang}'.format(lang=project.language),
+            '.',
+            self.sphinx_build_dir
+        ])
+        self.run(*build_command, cwd=project.conf_dir(self.version.slug))
 
 
 class HtmlBuilder(BaseSphinx):
@@ -212,7 +214,7 @@ def move(self, **kwargs):
         if from_globs:
             from_file = from_globs[0]
             to_file = os.path.join(self.target, "%s.epub" % self.version.project.slug)
-            run('mv -f %s %s' % (from_file, to_file))
+            self.run('mv', '-f', from_file, to_file)
 
 
 class PdfBuilder(BaseSphinx):
@@ -223,42 +225,49 @@ class PdfBuilder(BaseSphinx):
     @restoring_chdir
     def build(self, **kwargs):
         self.clean()
-        project = self.version.project
-        os.chdir(project.conf_dir(self.version.slug))
+        cwd = self.project.conf_dir(self.version.slug)
         # Default to this so we can return it always.
-        results = {}
-        latex_results = run('%s -b latex -D language=%s -d _build/doctrees . _build/latex'
-                            % (project.venv_bin(version=self.version.slug,
-                                                bin='sphinx-build'), project.language))
-
-        if latex_results[0] == 0:
-            os.chdir('_build/latex')
-            tex_files = glob('*.tex')
-
-            if tex_files:
-                # Run LaTeX -> PDF conversions
-                pdflatex_cmds = [('pdflatex -interaction=nonstopmode %s'
-                                  % tex_file) for tex_file in tex_files]
-                makeindex_cmds = [('makeindex -s python.ist %s.idx'
-                                   % os.path.splitext(tex_file)[0]) for tex_file in tex_files]
-                pdf_results = run(*pdflatex_cmds)
-                ind_results = run(*makeindex_cmds)
-                pdf_results = run(*pdflatex_cmds)
-            else:
-                pdf_results = (0, "No tex files found", "No tex files found")
-                ind_results = (0, "No tex files found", "No tex files found")
-
-            results = [
-                latex_results[0] + ind_results[0] + pdf_results[0],
-                latex_results[1] + ind_results[1] + pdf_results[1],
-                latex_results[2] + ind_results[2] + pdf_results[2],
-            ]
-            pdf_match = PDF_RE.search(results[1])
-            if pdf_match:
-                self.pdf_file_name = pdf_match.group(1).strip()
-        else:
-            results = latex_results
-        return results
+        self.run(
+            self.project.venv_bin(version=self.version.slug, bin='sphinx-build'),
+            '-b', 'latex',
+            '-D', 'language={lang}'.format(lang=self.project.language),
+            '-d' ,'_build/doctrees',
+            '.',
+            '_build/latex',
+            cwd=cwd
+        )
+        latex_cwd = os.path.join(cwd, '_build', 'latex')
+        tex_files = glob(os.path.join(latex_cwd, '*.tex'))
+
+        if not tex_files:
+            # TODO status code here
+            raise BuildEnvironmentError('No TeX files were found')
+
+        # Run LaTeX -> PDF conversions
+        pdflatex_cmds = [
+            ['pdflatex',
+                '-interaction=nonstopmode',
+                tex_file]
+            for tex_file in tex_files]
+        makeindex_cmds = [
+            ['makeindex',
+                '-s',
+                'python.ist',
+                '{0}.idx'.format(os.path.splitext(tex_file)[0])]
+            for tex_file in tex_files]
+        for cmd in pdflatex_cmds:
+            self.run(*cmd, cwd=latex_cwd)
+        for cmd in makeindex_cmds:
+            self.run(*cmd, cwd=latex_cwd)
+        for cmd in pdflatex_cmds:
+            self.run(*cmd, cwd=latex_cwd)
+
+        # TODO this ain't going to work without having access to the command
+        # returned here. We should probably return the command from run
+        # TODO pdf_match = PDF_RE.search(results[1])
+        pdf_match = PDF_RE.search('')
+        if pdf_match:
+            self.pdf_file_name = pdf_match.group(1).strip()
 
     def move(self, **kwargs):
         if not os.path.exists(self.target):
@@ -283,4 +292,4 @@ def move(self, **kwargs):
                 from_file = None
         if from_file:
             to_file = os.path.join(self.target, "%s.pdf" % self.version.project.slug)
-            run('mv -f %s %s' % (from_file, to_file))
+            self.run('mv', '-f', from_file, to_file)

readthedocs/doc_builder/base.py

@@ -30,10 +30,16 @@ class BaseBuilder(object):
     _force = False
     # old_artifact_path = ..
 
-    def __init__(self, version, force=False):
+    def __init__(self, version, force=False, build_env=None):
+        self.project = build_env.project
+        # TODO do we really need to push the version in here?
         self.version = version
         self._force = force
-        self.target = self.version.project.artifact_path(version=self.version.slug, type=self.type)
+        self.target = self.version.project.artifact_path(
+            version=self.version.slug,
+            type=self.type
+        )
+        self.build_env = build_env
 
     def force(self, **kwargs):
         """
@@ -115,3 +121,7 @@ def create_index(self, extension='md', **kwargs):
 
                 index_file.write(index_text.format(dir=docs_dir, ext=extension))
                 index_file.close()
+
+    def run(self, *args, **kwargs):
+        '''Proxy run to build environment'''
+        self.build_env.run(*args, **kwargs)