Build: allow partial override of build steps (#11710)

I was replacing some of the config models with dataclasses, but I found
myself re-implementing some helpers that pydantic provides, so I'm
introducing that new dep, it has everything we need, we may be able to
use it to simplify our validation once all models are migrated to
pydantic.

### About incompatible options

I decided to just not allow formats when `build.jobs.build` is used,
seems just easier that way. But not sure if users may want to just
override the html build step while still using the default build pdf
step, if that's the case, we may need to support using formats... or
have another way of saying "use the default".

build.jobs.create_environment is kind of incompatible with python/conda.
Since users will need to manually create the environment,
but they may still want to use the defaults from build.jobs.install,
or maybe they can't even use the defaults, as they are really tied to
the default create_environment step.

Which bring us to the next point,
if build.jobs.create_environment is overridden,
users will likely need to override build.jobs.install and
build.jobs.build as well...
Maybe just save the user some time and require them to override all of
them?
Or maybe just let them figure it out by themselves with the errors?
We could gather more information once we see some real usage of this...

### Override them all

I chose to make build.html required if users override that step, seems
logical to do so.

~~Do we want to allow users to build all formats? I'm starting with html
and pdf, that seem the most used. But shouldn't be a problem to support
all of them. I guess my only question would be about naming, `htmlzip`
has always been a weird name, maybe just zip?~~ I just went ahead and
allowed all, with the same name as formats.

### Docs

I didn't add docs yet because there is the question... should we just
expose this to users? Or maybe just test it internally for now?

Closes #11551

Author: stsewd

.circleci/config.yml

@@ -81,7 +81,7 @@ jobs:
       - restore_cache:
           keys:
           - pre-commit-cache-{{ checksum "pre-commit-cache-key.txt" }}
-      - run: pip install --user 'tox<5'
+      - run: pip install --user tox
       - run: tox -e pre-commit
       - run: tox -e migrations
       - node/install:

readthedocs/config/config.py

@@ -16,6 +16,7 @@
 from .find import find_one
 from .models import (
     BuildJobs,
+    BuildJobsBuildTypes,
     BuildTool,
     BuildWithOs,
     Conda,
@@ -318,11 +319,9 @@ def validate_build_config_with_os(self):
             # 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.
+            valid_jobs = list(BuildJobs.model_fields.keys())
             for job in jobs.keys():
-                validate_choice(
-                    job,
-                    BuildJobs.__slots__,
-                )
+                validate_choice(job, valid_jobs)
 
         commands = []
         with self.catch_validation_error("build.commands"):
@@ -346,6 +345,14 @@ def validate_build_config_with_os(self):
             )
 
         build["jobs"] = {}
+
+        with self.catch_validation_error("build.jobs.build"):
+            build["jobs"]["build"] = self.validate_build_jobs_build(jobs)
+        # Remove the build.jobs.build key from the build.jobs dict,
+        # since it's the only key that should be a dictionary,
+        # it was already validated above.
+        jobs.pop("build", None)
+
         for job, job_commands in jobs.items():
             with self.catch_validation_error(f"build.jobs.{job}"):
                 build["jobs"][job] = [
@@ -370,6 +377,29 @@ def validate_build_config_with_os(self):
         build["apt_packages"] = self.validate_apt_packages()
         return build
 
+    def validate_build_jobs_build(self, build_jobs):
+        result = {}
+        build_jobs_build = build_jobs.get("build", {})
+        validate_dict(build_jobs_build)
+
+        allowed_build_types = list(BuildJobsBuildTypes.model_fields.keys())
+        for build_type, build_commands in build_jobs_build.items():
+            validate_choice(build_type, allowed_build_types)
+            if build_type != "html" and build_type not in self.formats:
+                raise ConfigError(
+                    message_id=ConfigError.BUILD_JOBS_BUILD_TYPE_MISSING_IN_FORMATS,
+                    format_values={
+                        "build_type": build_type,
+                    },
+                )
+            with self.catch_validation_error(f"build.jobs.build.{build_type}"):
+                result[build_type] = [
+                    validate_string(build_command)
+                    for build_command in validate_list(build_commands)
+                ]
+
+        return result
+
     def validate_apt_packages(self):
         apt_packages = []
         with self.catch_validation_error("build.apt_packages"):

readthedocs/config/exceptions.py

@@ -13,6 +13,9 @@ class ConfigError(BuildUserError):
     INVALID_VERSION = "config:base:invalid-version"
     NOT_BUILD_TOOLS_OR_COMMANDS = "config:build:missing-build-tools-commands"
     BUILD_JOBS_AND_COMMANDS = "config:build:jobs-and-commands"
+    BUILD_JOBS_BUILD_TYPE_MISSING_IN_FORMATS = (
+        "config:build:jobs:build:missing-in-formats"
+    )
     APT_INVALID_PACKAGE_NAME_PREFIX = "config:apt:invalid-package-name-prefix"
     APT_INVALID_PACKAGE_NAME = "config:apt:invalid-package-name"
     USE_PIP_FOR_EXTRA_REQUIREMENTS = "config:python:pip-required"

readthedocs/config/models.py

@@ -1,4 +1,5 @@
 """Models for the response of the configuration object."""
+from pydantic import BaseModel
 
 from readthedocs.config.utils import to_dict
 
@@ -37,33 +38,41 @@ class BuildTool(Base):
     __slots__ = ("version", "full_version")
 
 
-class BuildJobs(Base):
+class BuildJobsBuildTypes(BaseModel):
+
+    """Object used for `build.jobs.build` key."""
+
+    html: list[str] | None = None
+    pdf: list[str] | None = None
+    epub: list[str] | None = None
+    htmlzip: list[str] | None = None
+
+    def as_dict(self):
+        # Just to keep compatibility with the old implementation.
+        return self.model_dump()
+
+
+class BuildJobs(BaseModel):
 
     """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",
-    )
+    pre_checkout: list[str] = []
+    post_checkout: list[str] = []
+    pre_system_dependencies: list[str] = []
+    post_system_dependencies: list[str] = []
+    pre_create_environment: list[str] = []
+    create_environment: list[str] | None = None
+    post_create_environment: list[str] = []
+    pre_install: list[str] = []
+    install: list[str] | None = None
+    post_install: list[str] = []
+    pre_build: list[str] = []
+    build: BuildJobsBuildTypes = BuildJobsBuildTypes()
+    post_build: list[str] = []
 
-    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)
+    def as_dict(self):
+        # Just to keep compatibility with the old implementation.
+        return self.model_dump()
 
 
 class Python(Base):

readthedocs/config/notifications.py

@@ -124,6 +124,18 @@
         ),
         type=ERROR,
     ),
+    Message(
+        id=ConfigError.BUILD_JOBS_BUILD_TYPE_MISSING_IN_FORMATS,
+        header=_("Invalid configuration option"),
+        body=_(
+            textwrap.dedent(
+                """
+                The <code>{{ build_type }}</code> build type was defined in <code>build.jobs.build</code>, but it wasn't included in <code>formats</code>.
+                """
+            ).strip(),
+        ),
+        type=ERROR,
+    ),
     Message(
         id=ConfigError.APT_INVALID_PACKAGE_NAME_PREFIX,
         header=_("Invalid APT package name"),

readthedocs/config/tests/test_config.py

@@ -14,6 +14,7 @@
 from readthedocs.config.exceptions import ConfigError, ConfigValidationError
 from readthedocs.config.models import (
     BuildJobs,
+    BuildJobsBuildTypes,
     BuildWithOs,
     PythonInstall,
     PythonInstallRequirements,
@@ -627,17 +628,120 @@ def test_jobs_build_config(self):
         assert build.build.jobs.pre_create_environment == [
             "echo pre_create_environment"
         ]
+        assert build.build.jobs.create_environment is None
         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.install is None
         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.build == BuildJobsBuildTypes()
         assert build.build.jobs.post_build == ["echo post_build"]
 
+    def test_build_jobs_partial_override(self):
+        build = get_build_config(
+            {
+                "formats": ["pdf", "htmlzip", "epub"],
+                "build": {
+                    "os": "ubuntu-20.04",
+                    "tools": {"python": "3"},
+                    "jobs": {
+                        "create_environment": ["echo make_environment"],
+                        "install": ["echo install"],
+                        "build": {
+                            "html": ["echo build html"],
+                            "pdf": ["echo build pdf"],
+                            "epub": ["echo build epub"],
+                            "htmlzip": ["echo build htmlzip"],
+                        },
+                    },
+                },
+            },
+        )
+        build.validate()
+        assert isinstance(build.build, BuildWithOs)
+        assert isinstance(build.build.jobs, BuildJobs)
+        assert build.build.jobs.create_environment == ["echo make_environment"]
+        assert build.build.jobs.install == ["echo install"]
+        assert build.build.jobs.build.html == ["echo build html"]
+        assert build.build.jobs.build.pdf == ["echo build pdf"]
+        assert build.build.jobs.build.epub == ["echo build epub"]
+        assert build.build.jobs.build.htmlzip == ["echo build htmlzip"]
+
+    def test_build_jobs_build_should_match_formats(self):
+        build = get_build_config(
+            {
+                "formats": ["pdf"],
+                "build": {
+                    "os": "ubuntu-24.04",
+                    "tools": {"python": "3"},
+                    "jobs": {
+                        "build": {
+                            "epub": ["echo build epub"],
+                        },
+                    },
+                },
+            },
+        )
+        with raises(ConfigError) as excinfo:
+            build.validate()
+        assert (
+            excinfo.value.message_id
+            == ConfigError.BUILD_JOBS_BUILD_TYPE_MISSING_IN_FORMATS
+        )
+
+    def test_build_jobs_build_defaults(self):
+        build = get_build_config(
+            {
+                "build": {
+                    "os": "ubuntu-24.04",
+                    "tools": {"python": "3"},
+                    "jobs": {
+                        "build": {
+                            "html": ["echo build html"],
+                        },
+                    },
+                },
+            },
+        )
+        build.validate()
+        assert build.build.jobs.build.html == ["echo build html"]
+        assert build.build.jobs.build.pdf is None
+        assert build.build.jobs.build.htmlzip is None
+        assert build.build.jobs.build.epub is None
+
+    def test_build_jobs_partial_override_empty_commands(self):
+        build = get_build_config(
+            {
+                "formats": ["pdf"],
+                "build": {
+                    "os": "ubuntu-24.04",
+                    "tools": {"python": "3"},
+                    "jobs": {
+                        "create_environment": [],
+                        "install": [],
+                        "build": {
+                            "html": [],
+                            "pdf": [],
+                        },
+                    },
+                },
+            },
+        )
+        build.validate()
+        assert isinstance(build.build, BuildWithOs)
+        assert isinstance(build.build.jobs, BuildJobs)
+        assert build.build.jobs.create_environment == []
+        assert build.build.jobs.install == []
+        assert build.build.jobs.build.html == []
+        assert build.build.jobs.build.pdf == []
+        assert build.build.jobs.build.epub == None
+        assert build.build.jobs.build.htmlzip == None
+
     @pytest.mark.parametrize(
         "value",
         [
@@ -1757,10 +1861,18 @@ def test_as_dict_new_build_config(self, tmpdir):
                     "pre_system_dependencies": [],
                     "post_system_dependencies": [],
                     "pre_create_environment": [],
+                    "create_environment": None,
                     "post_create_environment": [],
                     "pre_install": [],
+                    "install": None,
                     "post_install": [],
                     "pre_build": [],
+                    "build": {
+                        "html": None,
+                        "pdf": None,
+                        "epub": None,
+                        "htmlzip": None,
+                    },
                     "post_build": [],
                 },
                 "apt_packages": [],

readthedocs/core/utils/objects.py

@@ -0,0 +1,22 @@
+# Sentinel value to check if a default value was provided,
+# so we can differentiate when None is provided as a default value
+# and when it was not provided at all.
+_DEFAULT = object()
+
+
+def get_dotted_attribute(obj, attribute, default=_DEFAULT):
+    """
+    Allow to get nested attributes from an object using a dot notation.
+
+    This behaves similar to getattr, but allows to get nested attributes.
+    Similar, if a default value is provided, it will be returned if the
+    attribute is not found, otherwise it will raise an AttributeError.
+    """
+    for attr in attribute.split("."):
+        if hasattr(obj, attr):
+            obj = getattr(obj, attr)
+        elif default is not _DEFAULT:
+            return default
+        else:
+            raise AttributeError(f"Object {obj} has no attribute {attr}")
+    return obj