diff --git a/readthedocs/config/config.py b/readthedocs/config/config.py index 43d21ce99df..a36d42fa53e 100644 --- a/readthedocs/config/config.py +++ b/readthedocs/config/config.py @@ -15,6 +15,7 @@ from .find import find_one from .models import ( Build, + BuildJobs, BuildTool, BuildWithTools, Conda, @@ -751,6 +752,10 @@ def validate_conda(self): conda['environment'] = validate_path(environment, self.base_path) return conda + # NOTE: I think we should rename `BuildWithTools` to `BuildWithOs` since + # `os` is the main and mandatory key that makes the diference + # + # NOTE: `build.jobs` can't be used without using `build.os` def validate_build_config_with_tools(self): """ Validates the build object (new format). @@ -769,6 +774,22 @@ def validate_build_config_with_tools(self): for tool in tools.keys(): validate_choice(tool, self.settings['tools'].keys()) + jobs = {} + with self.catch_validation_error("build.jobs"): + # FIXME: should we use `default={}` or kept the `None` here and + # shortcircuit the rest of the logic? + jobs = self.pop_config("build.jobs", default={}) + validate_dict(jobs) + # NOTE: besides validating that each key is one of the expected + # ones, we could validate the value of each of them is a list of + # commands. However, I don't think we should validate the "command" + # looks like a real command. + for job in jobs.keys(): + validate_choice( + job, + BuildJobs.__slots__, + ) + if not tools: self.error( key='build.tools', @@ -780,6 +801,13 @@ def validate_build_config_with_tools(self): code=CONFIG_REQUIRED, ) + build["jobs"] = {} + for job, commands in jobs.items(): + with self.catch_validation_error(f"build.jobs.{job}"): + build["jobs"][job] = [ + validate_string(command) for command in validate_list(commands) + ] + build['tools'] = {} for tool, version in tools.items(): with self.catch_validation_error(f'build.tools.{tool}'): @@ -1263,7 +1291,8 @@ def build(self): return BuildWithTools( os=build['os'], tools=tools, - apt_packages=build['apt_packages'], + jobs=BuildJobs(**build["jobs"]), + apt_packages=build["apt_packages"], ) return Build(**build) diff --git a/readthedocs/config/models.py b/readthedocs/config/models.py index 8e0ed70881a..fa64ab983ef 100644 --- a/readthedocs/config/models.py +++ b/readthedocs/config/models.py @@ -37,7 +37,7 @@ def __init__(self, **kwargs): class BuildWithTools(Base): - __slots__ = ('os', 'tools', 'apt_packages') + __slots__ = ("os", "tools", "jobs", "apt_packages") def __init__(self, **kwargs): kwargs.setdefault('apt_packages', []) @@ -49,6 +49,35 @@ class BuildTool(Base): __slots__ = ('version', 'full_version') +class BuildJobs(Base): + + """Object used for `build.jobs` key.""" + + __slots__ = ( + "pre_checkout", + "post_checkout", + "pre_system_dependencies", + "post_system_dependencies", + "pre_create_environment", + "post_create_environment", + "pre_install", + "post_install", + "pre_build", + "post_build", + ) + + def __init__(self, **kwargs): + """ + Create an empty list as a default for all possible builds.jobs configs. + + This is necessary because it makes the code cleaner when we add items to these lists, + without having to check for a dict to be created first. + """ + for step in self.__slots__: + kwargs.setdefault(step, []) + super().__init__(**kwargs) + + class Python(Base): __slots__ = ('version', 'install', 'use_system_site_packages') diff --git a/readthedocs/config/tests/test_config.py b/readthedocs/config/tests/test_config.py index 9850b9c79f2..0b341376d86 100644 --- a/readthedocs/config/tests/test_config.py +++ b/readthedocs/config/tests/test_config.py @@ -1,11 +1,11 @@ import os -from django.conf import settings import re import textwrap from collections import OrderedDict from unittest.mock import DEFAULT, patch import pytest +from django.conf import settings from pytest import raises from readthedocs.config import ( @@ -33,6 +33,7 @@ ) from readthedocs.config.models import ( Build, + BuildJobs, BuildWithTools, Conda, PythonInstall, @@ -1012,6 +1013,87 @@ def test_new_build_config_conflict_with_build_python_version(self): build.validate() assert excinfo.value.key == 'python.version' + @pytest.mark.parametrize("value", ["", None, "pre_invalid"]) + def test_jobs_build_config_invalid_jobs(self, value): + build = self.get_build_config( + { + "build": { + "os": "ubuntu-20.04", + "tools": {"python": "3.8"}, + "jobs": {value: ["echo 1234", "git fetch --unshallow"]}, + }, + }, + ) + with raises(InvalidConfig) as excinfo: + build.validate() + assert excinfo.value.key == "build.jobs" + + @pytest.mark.parametrize("value", ["", None, "echo 123", 42]) + def test_jobs_build_config_invalid_job_commands(self, value): + build = self.get_build_config( + { + "build": { + "os": "ubuntu-20.04", + "tools": {"python": "3.8"}, + "jobs": { + "pre_install": value, + }, + }, + }, + ) + with raises(InvalidConfig) as excinfo: + build.validate() + assert excinfo.value.key == "build.jobs.pre_install" + + def test_jobs_build_config(self): + build = self.get_build_config( + { + "build": { + "os": "ubuntu-20.04", + "tools": {"python": "3.8"}, + "jobs": { + "pre_checkout": ["echo pre_checkout"], + "post_checkout": ["echo post_checkout"], + "pre_system_dependencies": ["echo pre_system_dependencies"], + "post_system_dependencies": ["echo post_system_dependencies"], + "pre_create_environment": ["echo pre_create_environment"], + "post_create_environment": ["echo post_create_environment"], + "pre_install": ["echo pre_install", "echo `date`"], + "post_install": ["echo post_install"], + "pre_build": [ + "echo pre_build", + 'sed -i -e "s|{VERSION}|${READTHEDOCS_VERSION_NAME}|g"', + ], + "post_build": ["echo post_build"], + }, + }, + }, + ) + build.validate() + assert isinstance(build.build, BuildWithTools) + assert isinstance(build.build.jobs, BuildJobs) + assert build.build.jobs.pre_checkout == ["echo pre_checkout"] + assert build.build.jobs.post_checkout == ["echo post_checkout"] + assert build.build.jobs.pre_system_dependencies == [ + "echo pre_system_dependencies" + ] + assert build.build.jobs.post_system_dependencies == [ + "echo post_system_dependencies" + ] + assert build.build.jobs.pre_create_environment == [ + "echo pre_create_environment" + ] + assert build.build.jobs.post_create_environment == [ + "echo post_create_environment" + ] + assert build.build.jobs.pre_install == ["echo pre_install", "echo `date`"] + assert build.build.jobs.post_install == ["echo post_install"] + assert build.build.jobs.pre_build == [ + "echo pre_build", + 'sed -i -e "s|{VERSION}|${READTHEDOCS_VERSION_NAME}|g"', + ] + assert build.build.jobs.post_build == ["echo post_build"] + @pytest.mark.parametrize( 'value', [ @@ -2297,6 +2379,18 @@ def test_as_dict_new_build_config(self, tmpdir): 'full_version': settings.RTD_DOCKER_BUILD_SETTINGS['tools']['nodejs']['16'], }, }, + "jobs": { + "pre_checkout": [], + "post_checkout": [], + "pre_system_dependencies": [], + "post_system_dependencies": [], + "pre_create_environment": [], + "post_create_environment": [], + "pre_install": [], + "post_install": [], + "pre_build": [], + "post_build": [], + }, 'apt_packages': [], }, 'conda': None, diff --git a/readthedocs/doc_builder/director.py b/readthedocs/doc_builder/director.py index 17fd01f4036..c04bec6c219 100644 --- a/readthedocs/doc_builder/director.py +++ b/readthedocs/doc_builder/director.py @@ -79,9 +79,17 @@ def setup_vcs(self): version_type=self.data.version.type, ) - self.pre_checkout() + # We can't do too much on ``pre_checkout`` because we haven't + # cloned the repository yet and we don't know what the user wrote + # in the `.readthedocs.yaml` yet. + # + # We could implement something different in the future if we download + # the `.readthedocs.yaml` file without cloning. + # See https://github.com/readthedocs/readthedocs.org/issues/8935 + # + # self.run_build_job("pre_checkout") self.checkout() - self.post_checkout() + self.run_build_job("post_checkout") commit = self.data.build_commit or self.vcs_repository.commit if commit: @@ -140,21 +148,21 @@ def setup_environment(self): environment=self.build_environment, ) - self.pre_system_dependencies() + self.run_build_job("pre_system_dependencies") self.system_dependencies() - self.post_system_dependencies() + self.run_build_job("post_system_dependencies") # Install all ``build.tools`` specified by the user if self.data.config.using_build_tools: self.language_environment.install_build_tools() - self.pre_create_environment() + self.run_build_job("pre_create_environment") self.create_environment() - self.post_create_environment() + self.run_build_job("post_create_environment") - self.pre_install() + self.run_build_job("pre_install") self.install() - self.post_install() + self.run_build_job("post_install") # TODO: remove this and document how to do it on `build.jobs.post_install` if self.data.project.has_feature(Feature.LIST_PACKAGES_INSTALLED_ENV): @@ -169,6 +177,9 @@ def build(self): 3. build PDF 4. build ePub """ + + self.run_build_job("pre_build") + self.data.outcomes = defaultdict(lambda: False) self.data.outcomes["html"] = self.build_html() self.data.outcomes["search"] = self.is_type_sphinx() @@ -176,21 +187,13 @@ def build(self): self.data.outcomes["pdf"] = self.build_pdf() self.data.outcomes["epub"] = self.build_epub() + self.run_build_job("post_build") + after_build.send( sender=self.data.version, ) # VCS checkout - def pre_checkout(self): - # We can't do too much here because we haven't cloned the repository - # yet and we don't know what the user wrote in the `.readthedocs.yaml` - # yet. - # - # We could implement something different in the future if we download - # the `.readthedocs.yaml` file without cloning. - # See https://github.com/readthedocs/readthedocs.org/issues/8935 - pass - def checkout(self): log.info( "Clonning repository.", @@ -207,17 +210,7 @@ def checkout(self): if self.vcs_repository.supports_submodules: self.vcs_repository.update_submodules(self.data.config) - def post_checkout(self): - commands = [] # self.data.config.build.jobs.post_checkout - for command in commands: - self.build_environment.run(command) - # System dependencies (``build.apt_packages``) - def pre_system_dependencies(self): - commands = [] # self.data.config.build.jobs.pre_system_dependencies - for command in commands: - self.build_environment.run(command) - # NOTE: `system_dependencies` should not be possible to override by the # user because it's executed as ``RTD_DOCKER_USER`` (e.g. ``root``) user. def system_dependencies(self): @@ -253,73 +246,23 @@ def system_dependencies(self): user=settings.RTD_DOCKER_SUPER_USER, ) - def post_system_dependencies(self): - pass - # Language environment - def pre_create_environment(self): - commands = [] # self.data.config.build.jobs.pre_create_environment - for command in commands: - self.build_environment.run(command) - def create_environment(self): - commands = [] # self.data.config.build.jobs.create_environment - for command in commands: - self.build_environment.run(command) - - if not commands: - self.language_environment.setup_base() - - def post_create_environment(self): - commands = [] # self.data.config.build.jobs.post_create_environment - for command in commands: - self.build_environment.run(command) + self.language_environment.setup_base() # Install - def pre_install(self): - commands = [] # self.data.config.build.jobs.pre_install - for command in commands: - self.build_environment.run(command) - def install(self): - commands = [] # self.data.config.build.jobs.install - for command in commands: - self.build_environment.run(command) - - if not commands: - self.language_environment.install_core_requirements() - self.language_environment.install_requirements() - - def post_install(self): - commands = [] # self.data.config.build.jobs.post_install - for command in commands: - self.build_environment.run(command) + self.language_environment.install_core_requirements() + self.language_environment.install_requirements() # Build - def pre_build(self): - commands = [] # self.data.config.build.jobs.pre_build - for command in commands: - self.build_environment.run(command) - def build_html(self): - commands = [] # self.data.config.build.jobs.build.html - if commands: - for command in commands: - self.build_environment.run(command) - return True - return self.build_docs_class(self.data.config.doctype) def build_pdf(self): if "pdf" not in self.data.config.formats or self.data.version.type == EXTERNAL: return False - commands = [] # self.data.config.build.jobs.build.pdf - if commands: - for command in commands: - self.build_environment.run(command) - return True - # Mkdocs has no pdf generation currently. if self.is_type_sphinx(): return self.build_docs_class("sphinx_pdf") @@ -333,12 +276,6 @@ def build_htmlzip(self): ): return False - commands = [] # self.data.config.build.jobs.build.htmlzip - if commands: - for command in commands: - self.build_environment.run(command) - return True - # We don't generate a zip for mkdocs currently. if self.is_type_sphinx(): return self.build_docs_class("sphinx_singlehtmllocalmedia") @@ -348,21 +285,53 @@ def build_epub(self): if "epub" not in self.data.config.formats or self.data.version.type == EXTERNAL: return False - commands = [] # self.data.config.build.jobs.build.epub - if commands: - for command in commands: - self.build_environment.run(command) - return True - # Mkdocs has no epub generation currently. if self.is_type_sphinx(): return self.build_docs_class("sphinx_epub") return False - def post_build(self): - commands = [] # self.data.config.build.jobs.post_build + def run_build_job(self, job): + """ + Run a command specified by the user under `build.jobs.` config key. + + It uses the "VCS environment" for pre_/post_ checkout jobs and "build + environment" for the rest of them. + + Note that user's commands: + + - are not escaped + - are run with under the path where the repository was cloned + - are run as RTD_DOCKER_USER user + - users can't run commands as `root` user + - all the user's commands receive same environment variables as regular commands + + Example: + + build: + jobs: + pre_install: + - echo `date` + - python path/to/myscript.py + pre_build: + - sed -i **/*.rst -e "s|{version}|v3.5.1|g" + + In this case, `self.data.config.build.jobs.pre_build` will contains + `sed` command. + """ + if ( + getattr(self.data.config.build, "jobs", None) is None + or getattr(self.data.config.build.jobs, job, None) is None + ): + return + + cwd = self.data.project.checkout_path(self.data.version.slug) + environment = self.vcs_environment + if job not in ("pre_checkout", "post_checkout"): + environment = self.build_environment + + commands = getattr(self.data.config.build.jobs, job, []) for command in commands: - self.build_environment.run(command) + environment.run(*command.split(), escape_command=False, cwd=cwd) # Helpers # diff --git a/readthedocs/projects/tests/test_build_tasks.py b/readthedocs/projects/tests/test_build_tasks.py index 57e53f2771a..bc62a3df3a2 100644 --- a/readthedocs/projects/tests/test_build_tasks.py +++ b/readthedocs/projects/tests/test_build_tasks.py @@ -733,6 +733,46 @@ def test_build_tools(self, load_yaml_config): ] ) + @mock.patch("readthedocs.doc_builder.director.load_yaml_config") + def test_build_jobs(self, load_yaml_config): + config = BuildConfigV2( + {}, + { + "version": 2, + "build": { + "os": "ubuntu-20.04", + "tools": {"python": "3.7"}, + "jobs": { + "post_checkout": ["git fetch --unshallow"], + "pre_build": ["echo `date`"], + }, + }, + }, + source_file="readthedocs.yml", + ) + config.validate() + load_yaml_config.return_value = config + + self._trigger_update_docs_task() + + self.mocker.mocks["environment.run"].assert_has_calls( + [ + mock.call( + "git", "fetch", "--unshallow", escape_command=False, cwd=mock.ANY + ), + # Don't care about the intermediate commands. They are checked + # in other tests + mock.ANY, + mock.ANY, + mock.ANY, + mock.ANY, + mock.ANY, + mock.ANY, + mock.ANY, + mock.call("echo", "`date`", escape_command=False, cwd=mock.ANY), + ] + ) + @mock.patch("readthedocs.doc_builder.python_environments.tarfile") @mock.patch("readthedocs.doc_builder.python_environments.build_tools_storage") @mock.patch("readthedocs.doc_builder.director.load_yaml_config")