From a52fa2c1d58695f9fe474c856fe6b47a433eb46e Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 15 Mar 2022 18:18:05 +0100 Subject: [PATCH 01/14] Build: implment `build.jobs` config file key Allow people to use `build.jobs` to execute pre/post steps. ```yaml build: jobs: pre_install: - echo `date` - python path/to/myscript.py pre_build: - sed -i **/*.rst -e "s|{version}|v3.5.1|g" ``` --- readthedocs/config/config.py | 36 +++++++++++++++++- readthedocs/config/models.py | 18 ++++++++- readthedocs/doc_builder/director.py | 59 ++++++++++++++++------------- 3 files changed, 84 insertions(+), 29 deletions(-) diff --git a/readthedocs/config/config.py b/readthedocs/config/config.py index 43d21ce99df..7c6846b43d3 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,16 @@ def validate_build_config_with_tools(self): code=CONFIG_REQUIRED, ) + build["jobs"] = {} + for job in BuildJobs.__slots__: + build["jobs"][job] = [] + + 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 +1294,10 @@ def build(self): return BuildWithTools( os=build['os'], tools=tools, - apt_packages=build['apt_packages'], + jobs=BuildJobs( + **{job: commands for job, commands in build["jobs"].items()} + ), + apt_packages=build["apt_packages"], ) return Build(**build) diff --git a/readthedocs/config/models.py b/readthedocs/config/models.py index 8e0ed70881a..b514c1540d1 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,22 @@ class BuildTool(Base): __slots__ = ('version', 'full_version') +class BuildJobs(Base): + + __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", + ) + + class Python(Base): __slots__ = ('version', 'install', 'use_system_site_packages') diff --git a/readthedocs/doc_builder/director.py b/readthedocs/doc_builder/director.py index dc7646db99d..da7b9125d19 100644 --- a/readthedocs/doc_builder/director.py +++ b/readthedocs/doc_builder/director.py @@ -1,4 +1,5 @@ import os +import shlex from collections import defaultdict import structlog @@ -65,7 +66,7 @@ def setup_vcs(self): ), ) - environment = self.data.environment_class( + self.vcs_environment = self.data.environment_class( project=self.data.project, version=self.data.version, build=self.data.build, @@ -74,17 +75,17 @@ def setup_vcs(self): # ca-certificate package which is compatible with Lets Encrypt container_image=settings.RTD_DOCKER_BUILD_SETTINGS["os"]["ubuntu-20.04"], ) - with environment: + with self.vcs_environment: before_vcs.send( sender=self.data.version, - environment=environment, + environment=self.vcs_environment, ) # Create the VCS repository where all the commands are going to be # executed for a particular VCS type self.vcs_repository = self.data.project.vcs_repo( version=self.data.version.slug, - environment=environment, + environment=self.vcs_environment, verbose_name=self.data.version.verbose_name, version_type=self.data.version.type, ) @@ -207,15 +208,17 @@ def checkout(self): self.vcs_repository.update_submodules(self.data.config) def post_checkout(self): - commands = [] # self.data.config.build.jobs.post_checkout + commands = self.data.config.build.jobs.post_checkout for command in commands: - self.build_environment.run(command) + # TODO: we could make a helper `self.run(environment, command)` + # that handles split and escape command + self.vcs_environment.run(*shlex.split(command), escape_command=False) # System dependencies (``build.apt_packages``) def pre_system_dependencies(self): - commands = [] # self.data.config.build.jobs.pre_system_dependencies + commands = self.data.config.build.jobs.pre_system_dependencies for command in commands: - self.build_environment.run(command) + self.build_environment.run(*shlex.split(command), escape_command=False) # NOTE: `system_dependencies` should not be possible to override by the # user because it's executed as ``RTD_DOCKER_USER`` (e.g. ``root``) user. @@ -253,58 +256,60 @@ def system_dependencies(self): ) def post_system_dependencies(self): - pass + commands = self.data.config.build.jobs.post_system_dependencies + for command in commands: + self.build_environment.run(*shlex.split(command), escape_command=False) # Language environment def pre_create_environment(self): - commands = [] # self.data.config.build.jobs.pre_create_environment + commands = self.data.config.build.jobs.pre_create_environment for command in commands: - self.build_environment.run(command) + self.build_environment.run(*shlex.split(command), escape_command=False) def create_environment(self): commands = [] # self.data.config.build.jobs.create_environment for command in commands: - self.build_environment.run(command) + self.build_environment.run(*shlex.split(command), escape_command=False) if not commands: self.language_environment.setup_base() def post_create_environment(self): - commands = [] # self.data.config.build.jobs.post_create_environment + commands = self.data.config.build.jobs.post_create_environment for command in commands: - self.build_environment.run(command) + self.build_environment.run(*shlex.split(command), escape_command=False) # Install def pre_install(self): - commands = [] # self.data.config.build.jobs.pre_install + commands = self.data.config.build.jobs.pre_install for command in commands: - self.build_environment.run(command) + self.build_environment.run(*shlex.split(command), escape_command=False) def install(self): commands = [] # self.data.config.build.jobs.install for command in commands: - self.build_environment.run(command) + self.build_environment.run(*shlex.split(command), escape_command=False) 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 + commands = self.data.config.build.jobs.post_install for command in commands: - self.build_environment.run(command) + self.build_environment.run(*shlex.split(command), escape_command=False) # Build def pre_build(self): - commands = [] # self.data.config.build.jobs.pre_build + commands = self.data.config.build.jobs.pre_build for command in commands: - self.build_environment.run(command) + self.build_environment.run(*shlex.split(command), escape_command=False) def build_html(self): commands = [] # self.data.config.build.jobs.build.html if commands: for command in commands: - self.build_environment.run(command) + self.build_environment.run(*shlex.split(command), escape_command=False) return True return self.build_docs_class(self.data.config.doctype) @@ -316,7 +321,7 @@ def build_pdf(self): commands = [] # self.data.config.build.jobs.build.pdf if commands: for command in commands: - self.build_environment.run(command) + self.build_environment.run(*shlex.split(command), escape_command=False) return True # Mkdocs has no pdf generation currently. @@ -335,7 +340,7 @@ def build_htmlzip(self): commands = [] # self.data.config.build.jobs.build.htmlzip if commands: for command in commands: - self.build_environment.run(command) + self.build_environment.run(*shlex.split(command), escape_command=False) return True # We don't generate a zip for mkdocs currently. @@ -350,7 +355,7 @@ def build_epub(self): commands = [] # self.data.config.build.jobs.build.epub if commands: for command in commands: - self.build_environment.run(command) + self.build_environment.run(*shlex.split(command), escape_command=False) return True # Mkdocs has no epub generation currently. @@ -359,9 +364,9 @@ def build_epub(self): return False def post_build(self): - commands = [] # self.data.config.build.jobs.post_build + commands = self.data.config.build.jobs.post_build for command in commands: - self.build_environment.run(command) + self.build_environment.run(*shlex.split(command), escape_command=False) # Helpers # From 00f92796e1849c029217a467baf57ae2e1ce7e25 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 16 Mar 2022 09:59:44 +0100 Subject: [PATCH 02/14] Build director: add pre/post build to the flow --- readthedocs/doc_builder/director.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/readthedocs/doc_builder/director.py b/readthedocs/doc_builder/director.py index da7b9125d19..246f53accd0 100644 --- a/readthedocs/doc_builder/director.py +++ b/readthedocs/doc_builder/director.py @@ -169,6 +169,9 @@ def build(self): 3. build PDF 4. build ePub """ + + self.pre_build() + self.data.outcomes = defaultdict(lambda: False) self.data.outcomes["html"] = self.build_html() self.data.outcomes["search"] = self.is_type_sphinx() @@ -176,6 +179,8 @@ def build(self): self.data.outcomes["pdf"] = self.build_pdf() self.data.outcomes["epub"] = self.build_epub() + self.post_build() + after_build.send( sender=self.data.version, ) From 50980c762d086e8060e3ed144c7f02997a6c61aa Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 16 Mar 2022 10:00:14 +0100 Subject: [PATCH 03/14] Build director: run pre/post user commands --- readthedocs/doc_builder/director.py | 37 +++++++++++++++-------------- 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/readthedocs/doc_builder/director.py b/readthedocs/doc_builder/director.py index 246f53accd0..b172b0747a4 100644 --- a/readthedocs/doc_builder/director.py +++ b/readthedocs/doc_builder/director.py @@ -1,5 +1,4 @@ import os -import shlex from collections import defaultdict import structlog @@ -215,15 +214,13 @@ def checkout(self): def post_checkout(self): commands = self.data.config.build.jobs.post_checkout for command in commands: - # TODO: we could make a helper `self.run(environment, command)` - # that handles split and escape command - self.vcs_environment.run(*shlex.split(command), escape_command=False) + self.run_user_cmd(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(*shlex.split(command), escape_command=False) + self.run_user_cmd(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. @@ -263,18 +260,18 @@ def system_dependencies(self): def post_system_dependencies(self): commands = self.data.config.build.jobs.post_system_dependencies for command in commands: - self.build_environment.run(*shlex.split(command), escape_command=False) + self.run_user_cmd(command) # Language environment def pre_create_environment(self): commands = self.data.config.build.jobs.pre_create_environment for command in commands: - self.build_environment.run(*shlex.split(command), escape_command=False) + self.run_user_cmd(command) def create_environment(self): commands = [] # self.data.config.build.jobs.create_environment for command in commands: - self.build_environment.run(*shlex.split(command), escape_command=False) + self.run_user_cmd(command) if not commands: self.language_environment.setup_base() @@ -282,18 +279,18 @@ def create_environment(self): def post_create_environment(self): commands = self.data.config.build.jobs.post_create_environment for command in commands: - self.build_environment.run(*shlex.split(command), escape_command=False) + self.run_user_cmd(command) # Install def pre_install(self): commands = self.data.config.build.jobs.pre_install for command in commands: - self.build_environment.run(*shlex.split(command), escape_command=False) + self.run_user_cmd(command) def install(self): commands = [] # self.data.config.build.jobs.install for command in commands: - self.build_environment.run(*shlex.split(command), escape_command=False) + self.run_user_cmd(command) if not commands: self.language_environment.install_core_requirements() @@ -302,19 +299,19 @@ def install(self): def post_install(self): commands = self.data.config.build.jobs.post_install for command in commands: - self.build_environment.run(*shlex.split(command), escape_command=False) + self.run_user_cmd(command) # Build def pre_build(self): commands = self.data.config.build.jobs.pre_build for command in commands: - self.build_environment.run(*shlex.split(command), escape_command=False) + self.run_user_cmd(command) def build_html(self): commands = [] # self.data.config.build.jobs.build.html if commands: for command in commands: - self.build_environment.run(*shlex.split(command), escape_command=False) + self.run_user_cmd(command) return True return self.build_docs_class(self.data.config.doctype) @@ -326,7 +323,7 @@ def build_pdf(self): commands = [] # self.data.config.build.jobs.build.pdf if commands: for command in commands: - self.build_environment.run(*shlex.split(command), escape_command=False) + self.run_user_cmd(command) return True # Mkdocs has no pdf generation currently. @@ -345,7 +342,7 @@ def build_htmlzip(self): commands = [] # self.data.config.build.jobs.build.htmlzip if commands: for command in commands: - self.build_environment.run(*shlex.split(command), escape_command=False) + self.run_user_cmd(command) return True # We don't generate a zip for mkdocs currently. @@ -360,7 +357,7 @@ def build_epub(self): commands = [] # self.data.config.build.jobs.build.epub if commands: for command in commands: - self.build_environment.run(*shlex.split(command), escape_command=False) + self.run_user_cmd(command) return True # Mkdocs has no epub generation currently. @@ -371,12 +368,16 @@ def build_epub(self): def post_build(self): commands = self.data.config.build.jobs.post_build for command in commands: - self.build_environment.run(*shlex.split(command), escape_command=False) + self.run_user_cmd(command) # Helpers # # TODO: move somewhere or change names to make them private or something to # easily differentiate them from the normal flow. + def run_user_cmd(self, cmd): + cwd = self.data.project.checkout_path(self.data.version.slug) + self.build_environment.run(cmd.split(), escape_command=False, cwd=cwd) + def build_docs_class(self, builder_class): """ Build docs with additional doc backends. From f9c92ef5cb3c71bc0be2deacd3f8e64794d0c15c Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 16 Mar 2022 11:14:45 +0100 Subject: [PATCH 04/14] Build director: refactor how `build.jobs` are executed Put all the job steps under a helper called `run_build_job` to simplify the code. --- readthedocs/doc_builder/director.py | 139 +++++++--------------------- 1 file changed, 35 insertions(+), 104 deletions(-) diff --git a/readthedocs/doc_builder/director.py b/readthedocs/doc_builder/director.py index b172b0747a4..411b1d1950c 100644 --- a/readthedocs/doc_builder/director.py +++ b/readthedocs/doc_builder/director.py @@ -89,9 +89,18 @@ 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: @@ -139,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,7 +178,7 @@ def build(self): 4. build ePub """ - self.pre_build() + self.run_build_job("pre_build") self.data.outcomes = defaultdict(lambda: False) self.data.outcomes["html"] = self.build_html() @@ -178,23 +187,13 @@ def build(self): self.data.outcomes["pdf"] = self.build_pdf() self.data.outcomes["epub"] = self.build_epub() - self.post_build() + 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.", @@ -211,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.run_user_cmd(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.run_user_cmd(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): @@ -257,75 +246,23 @@ def system_dependencies(self): user=settings.RTD_DOCKER_SUPER_USER, ) - def post_system_dependencies(self): - commands = self.data.config.build.jobs.post_system_dependencies - for command in commands: - self.run_user_cmd(command) - # Language environment - def pre_create_environment(self): - commands = self.data.config.build.jobs.pre_create_environment - for command in commands: - self.run_user_cmd(command) - def create_environment(self): - commands = [] # self.data.config.build.jobs.create_environment - for command in commands: - self.run_user_cmd(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.run_user_cmd(command) + self.language_environment.setup_base() # Install - def pre_install(self): - commands = self.data.config.build.jobs.pre_install - for command in commands: - self.run_user_cmd(command) - def install(self): - commands = [] # self.data.config.build.jobs.install - for command in commands: - self.run_user_cmd(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.run_user_cmd(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.run_user_cmd(command) - def build_html(self): - commands = [] # self.data.config.build.jobs.build.html - if commands: - for command in commands: - self.run_user_cmd(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.run_user_cmd(command) - return True - # Mkdocs has no pdf generation currently. if self.is_type_sphinx(): return self.build_docs_class("sphinx_pdf") @@ -339,12 +276,6 @@ def build_htmlzip(self): ): return False - commands = [] # self.data.config.build.jobs.build.htmlzip - if commands: - for command in commands: - self.run_user_cmd(command) - return True - # We don't generate a zip for mkdocs currently. if self.is_type_sphinx(): return self.build_docs_class("sphinx_singlehtmllocalmedia") @@ -354,30 +285,30 @@ 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.run_user_cmd(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): + 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) + commands = getattr(self.data.config.build.jobs, job, []) for command in commands: - self.run_user_cmd(command) + environment = self.vcs_environment + if job not in ("pre_checkout", "post_checkout"): + environment = self.build_environment + environment.run(*command.split(), escape_command=False, cwd=cwd) # Helpers # # TODO: move somewhere or change names to make them private or something to # easily differentiate them from the normal flow. - def run_user_cmd(self, cmd): - cwd = self.data.project.checkout_path(self.data.version.slug) - self.build_environment.run(cmd.split(), escape_command=False, cwd=cwd) - def build_docs_class(self, builder_class): """ Build docs with additional doc backends. From 17f0ca21f1317af14e8479fa998cbc9d68a8c486 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 16 Mar 2022 11:15:25 +0100 Subject: [PATCH 05/14] Environment variables Pass `READTHEDOCS_VERSION_TYPE` and `READTHEDOCS_VERSION_NAME` that were missed because a merge conflict. These variables were added in https://github.com/readthedocs/readthedocs.org/pull/8237 --- readthedocs/doc_builder/director.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/readthedocs/doc_builder/director.py b/readthedocs/doc_builder/director.py index 411b1d1950c..8f690994202 100644 --- a/readthedocs/doc_builder/director.py +++ b/readthedocs/doc_builder/director.py @@ -343,6 +343,8 @@ def get_rtd_env_vars(self): env = { "READTHEDOCS": "True", "READTHEDOCS_VERSION": self.data.version.slug, + "READTHEDOCS_VERSION_TYPE": self.data.version.type, + "READTHEDOCS_VERSION_NAME": self.data.version.verbose_name, "READTHEDOCS_PROJECT": self.data.project.slug, "READTHEDOCS_LANGUAGE": self.data.project.language, } From ade425d903239542926dcb1d89dbfb499e3c40c6 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 16 Mar 2022 11:36:00 +0100 Subject: [PATCH 06/14] Lint: comprehention list is not needed --- readthedocs/config/config.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/readthedocs/config/config.py b/readthedocs/config/config.py index 7c6846b43d3..cb85cdb0787 100644 --- a/readthedocs/config/config.py +++ b/readthedocs/config/config.py @@ -1294,9 +1294,7 @@ def build(self): return BuildWithTools( os=build['os'], tools=tools, - jobs=BuildJobs( - **{job: commands for job, commands in build["jobs"].items()} - ), + jobs=BuildJobs(**build["jobs"]), apt_packages=build["apt_packages"], ) return Build(**build) From b4739fd1be37d2841c6c284a28036dfbc7646692 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 16 Mar 2022 13:04:02 +0100 Subject: [PATCH 07/14] Build director: define the environment just once for a set of cmds --- readthedocs/doc_builder/director.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/readthedocs/doc_builder/director.py b/readthedocs/doc_builder/director.py index 8f690994202..75479a77148 100644 --- a/readthedocs/doc_builder/director.py +++ b/readthedocs/doc_builder/director.py @@ -298,11 +298,12 @@ def run_build_job(self, job): 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: - environment = self.vcs_environment - if job not in ("pre_checkout", "post_checkout"): - environment = self.build_environment environment.run(*command.split(), escape_command=False, cwd=cwd) # Helpers From 1fcd354d4a69a2a47efea2bdb0fcb58e4f0df1de Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 16 Mar 2022 13:05:13 +0100 Subject: [PATCH 08/14] Test: add tests for `build.jobs` config --- readthedocs/config/tests/test_config.py | 96 ++++++++++++++++++- .../projects/tests/test_build_tasks.py | 40 ++++++++ 2 files changed, 135 insertions(+), 1 deletion(-) 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/projects/tests/test_build_tasks.py b/readthedocs/projects/tests/test_build_tasks.py index 81c9def784b..385d77c7d91 100644 --- a/readthedocs/projects/tests/test_build_tasks.py +++ b/readthedocs/projects/tests/test_build_tasks.py @@ -732,6 +732,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") From 03224d7ffde28d6599c44d258e7d173f0e1a9531 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 29 Mar 2022 10:01:37 +0200 Subject: [PATCH 09/14] Config: define default jobs using the common pattern --- readthedocs/config/config.py | 3 --- readthedocs/config/models.py | 5 +++++ 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/readthedocs/config/config.py b/readthedocs/config/config.py index cb85cdb0787..a36d42fa53e 100644 --- a/readthedocs/config/config.py +++ b/readthedocs/config/config.py @@ -802,9 +802,6 @@ def validate_build_config_with_tools(self): ) build["jobs"] = {} - for job in BuildJobs.__slots__: - build["jobs"][job] = [] - for job, commands in jobs.items(): with self.catch_validation_error(f"build.jobs.{job}"): build["jobs"][job] = [ diff --git a/readthedocs/config/models.py b/readthedocs/config/models.py index b514c1540d1..e23e1e8b404 100644 --- a/readthedocs/config/models.py +++ b/readthedocs/config/models.py @@ -64,6 +64,11 @@ class BuildJobs(Base): "post_build", ) + def __init__(self, **kwargs): + for step in self.__slots__: + kwargs.setdefault(step, []) + super().__init__(**kwargs) + class Python(Base): From 1b0e654592862f7265caec378abca270da0728d6 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Tue, 29 Mar 2022 10:11:41 +0200 Subject: [PATCH 10/14] Build director: add docstring explaining `run_build_job` --- readthedocs/doc_builder/director.py | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/readthedocs/doc_builder/director.py b/readthedocs/doc_builder/director.py index 75479a77148..c7f7d894f79 100644 --- a/readthedocs/doc_builder/director.py +++ b/readthedocs/doc_builder/director.py @@ -291,6 +291,33 @@ def build_epub(self): return False 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 environment variables + + 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 From 5247b12ddc09c7b7d2db8e54ffca3e0d993df7e9 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 30 Mar 2022 12:29:58 +0200 Subject: [PATCH 11/14] Update readthedocs/config/models.py Co-authored-by: Eric Holscher <25510+ericholscher@users.noreply.github.com> --- readthedocs/config/models.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/readthedocs/config/models.py b/readthedocs/config/models.py index e23e1e8b404..be20acfb829 100644 --- a/readthedocs/config/models.py +++ b/readthedocs/config/models.py @@ -65,6 +65,12 @@ class BuildJobs(Base): ) 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) From f3113757b60cc78ff6a24d9c4073a59a37309e7e Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 30 Mar 2022 12:35:25 +0200 Subject: [PATCH 12/14] Docstring: make it clear --- readthedocs/doc_builder/director.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/doc_builder/director.py b/readthedocs/doc_builder/director.py index c7f7d894f79..f282ec368a2 100644 --- a/readthedocs/doc_builder/director.py +++ b/readthedocs/doc_builder/director.py @@ -303,7 +303,7 @@ def run_build_job(self, job): - 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 environment variables + - all the user's commands receive same environment variables as regular commands Example: From b551a32d3ef225984158bf073acc4b692174820d Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 30 Mar 2022 12:36:54 +0200 Subject: [PATCH 13/14] Docstring: fix indentation --- readthedocs/config/models.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/readthedocs/config/models.py b/readthedocs/config/models.py index be20acfb829..3b9c2873a4b 100644 --- a/readthedocs/config/models.py +++ b/readthedocs/config/models.py @@ -65,12 +65,12 @@ class BuildJobs(Base): ) 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. - """ + """ + 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) From 102534ddeefdeafbfe9405b544fc7d373c0b9633 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 30 Mar 2022 13:05:28 +0200 Subject: [PATCH 14/14] Docstring: simple docstring to avoid lint issue --- readthedocs/config/models.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/readthedocs/config/models.py b/readthedocs/config/models.py index 3b9c2873a4b..fa64ab983ef 100644 --- a/readthedocs/config/models.py +++ b/readthedocs/config/models.py @@ -51,6 +51,8 @@ class BuildTool(Base): class BuildJobs(Base): + """Object used for `build.jobs` key.""" + __slots__ = ( "pre_checkout", "post_checkout",