From dbae5b4a49baf14a90e27a956aa143b47a5c44f0 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 4 Sep 2025 16:48:07 -0500 Subject: [PATCH 01/13] Build: skip checkout for latest when no default version is given --- readthedocs/api/v2/views/integrations.py | 7 ++-- readthedocs/projects/models.py | 46 +++++++++++++++++++----- 2 files changed, 42 insertions(+), 11 deletions(-) diff --git a/readthedocs/api/v2/views/integrations.py b/readthedocs/api/v2/views/integrations.py index 7717ab8a17a..6e1c0196a09 100644 --- a/readthedocs/api/v2/views/integrations.py +++ b/readthedocs/api/v2/views/integrations.py @@ -20,6 +20,7 @@ from readthedocs.builds.constants import BRANCH from readthedocs.builds.constants import LATEST +from readthedocs.builds.constants import LATEST_VERBOSE_NAME from readthedocs.builds.constants import TAG from readthedocs.core.signals import webhook_bitbucket from readthedocs.core.signals import webhook_github @@ -313,7 +314,7 @@ def get_closed_external_version_response(self, project): def update_default_branch(self, default_branch): """ - Update the `Version.identifer` for `latest` with the VCS's `default_branch`. + Update the `Version.identifier` for `latest` with the VCS's `default_branch`. The VCS's `default_branch` is the branch cloned when there is no specific branch specified (e.g. `git clone `). @@ -335,7 +336,9 @@ def update_default_branch(self, default_branch): # Always check for the machine attribute, since latest can be user created. # RTD doesn't manage those. self.project.versions.filter(slug=LATEST, machine=True).update( - identifier=default_branch + identifier=default_branch, + verbose_name=LATEST_VERBOSE_NAME, + type=BRANCH, ) diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index 8261238608e..898effad0ac 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -1177,15 +1177,21 @@ def get_original_latest_version(self): When latest is machine created, it's basically an alias for the default branch/tag (like main/master), - Returns None if the current default version doesn't point to a valid version. + Returns None if latest doesn't point to a valid version, + or if isn't managed by RTD (machine=False). """ - default_version_name = self.get_default_branch() + # For latest, the identifier is the name of the branch/tag. + latest_version_identifier = ( + self.versions.filter(slug=LATEST, machine=True) + .values_list("identifier", flat=True) + .first() + ) + if not latest_version_identifier: + return None return ( self.versions(manager=INTERNAL) .exclude(slug=LATEST) - .filter( - verbose_name=default_version_name, - ) + .filter(verbose_name=latest_version_identifier) .first() ) @@ -1203,8 +1209,22 @@ def update_latest_version(self): return # default_branch can be a tag or a branch name! - default_version_name = self.get_default_branch() - original_latest = self.get_original_latest_version() + default_version_name = self.get_default_branch(fallback_to_vcs=False) + # If the default_branch is not set, it means that the user + # wants to use the default branch of the respository, + # but we don't know what it is here, latest will be updated + # on the next build, when we have cloned the repository. + if not default_version_name: + return + + # Search for a branch or tag with the name of the default branch, + # so we can sync latest with it. + original_latest = ( + self.versions(manager=INTERNAL) + .exclude(slug=LATEST) + .filter(verbose_name=default_version_name) + .first() + ) latest.verbose_name = LATEST_VERBOSE_NAME latest.type = original_latest.type if original_latest else BRANCH # For latest, the identifier is the name of the branch/tag. @@ -1304,14 +1324,22 @@ def get_default_version(self): return self.default_version return LATEST - def get_default_branch(self): - """Get the version representing 'latest'.""" + def get_default_branch(self, fallback_to_vcs=True): + """ + Get the name of the branch or tag that the user wants to use as 'latest'. + + In case the user explicitly set a default branch, we use that, + otherwise we try to get it from the remote repository. + """ if self.default_branch: return self.default_branch if self.remote_repository and self.remote_repository.default_branch: return self.remote_repository.default_branch + if not fallback_to_vcs: + return None + vcs_class = self.vcs_class() if vcs_class: return vcs_class.fallback_branch From 24854def58af700f98e5186f7999617671f409cc Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 16 Sep 2025 16:16:29 -0500 Subject: [PATCH 02/13] Fix tests --- .../rtd_tests/tests/test_sync_versions.py | 49 +++++++++++-------- 1 file changed, 29 insertions(+), 20 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_sync_versions.py b/readthedocs/rtd_tests/tests/test_sync_versions.py index d7339f28cfa..e6f5195c9c5 100644 --- a/readthedocs/rtd_tests/tests/test_sync_versions.py +++ b/readthedocs/rtd_tests/tests/test_sync_versions.py @@ -294,7 +294,10 @@ def test_update_latest_version_type(self): self.assertEqual(latest_version.verbose_name, "latest") self.assertEqual(latest_version.machine, False) - # Latest is back as machine created, and as a branch. + # Latest is back as machine created, + # but its type and identifier are not changed, + # as the user doesn't have a default branch set. + # The correct identifier and type will be set on the next build. sync_versions_task( self.pip.pk, branches_data=branches_data, @@ -303,8 +306,8 @@ def test_update_latest_version_type(self): latest_version = self.pip.versions.get(slug=LATEST) self.assertIsNone(self.pip.default_branch) - self.assertEqual(latest_version.type, BRANCH) - self.assertEqual(latest_version.identifier, "master") + self.assertEqual(latest_version.type, TAG) + self.assertEqual(latest_version.identifier, "abc123") self.assertEqual(latest_version.verbose_name, "latest") self.assertEqual(latest_version.machine, True) @@ -729,7 +732,7 @@ def test_machine_attr_when_user_define_latest_tag_and_delete_it(self): branches_data = [ { - "identifier": "origin/master", + "identifier": "master", "verbose_name": "master", }, ] @@ -757,7 +760,7 @@ def test_machine_attr_when_user_define_latest_tag_and_delete_it(self): # Deleting the tag should return the RTD's latest branches_data = [ { - "identifier": "origin/master", + "identifier": "master", "verbose_name": "master", }, ] @@ -768,12 +771,15 @@ def test_machine_attr_when_user_define_latest_tag_and_delete_it(self): tags_data=[], ) - # The latest isn't stuck with the previous commit + # latest isn't stuck with the previous commit, + # but its type and identifier are not changed, + # as the user doesn't have a default branch set. + # The correct identifier and type will be set on the next build. version_latest = self.pip.versions.get(slug="latest") self.assertIsNone(self.pip.default_branch) self.assertTrue(version_latest.machine) self.assertEqual( - "master", + "1abc2def3", version_latest.identifier, ) self.assertTrue(version_latest.machine) @@ -801,20 +807,20 @@ def test_machine_attr_when_user_define_latest_tag_and_delete_it(self): self.assertEqual(version_latest.type, TAG) def test_machine_attr_when_user_define_latest_branch_and_delete_it(self): - """The user creates a branch named ``latest`` on an existing repo, when - syncing the versions, the RTD's ``latest`` is lost (set to - machine=False) and doesn't update automatically anymore, when the branch - is deleted on the user repository, the RTD's ``latest`` is back (set to - machine=True). + """ + The user creates a branch named ``latest`` on an existing repo, when + syncing the versions, the RTD's ``latest`` is lost (set to machine=False) + and doesn't update automatically anymore, when the branch is deleted on + the user repository, the RTD's ``latest`` is back (set to machine=True). """ branches_data = [ { - "identifier": "origin/master", + "identifier": "master", "verbose_name": "master", }, # User new latest { - "identifier": "origin/latest", + "identifier": "latest", "verbose_name": "latest", }, ] @@ -830,14 +836,14 @@ def test_machine_attr_when_user_define_latest_branch_and_delete_it(self): self.assertIsNone(self.pip.default_branch) self.assertFalse(version_latest.machine) self.assertEqual( - "origin/latest", + "latest", version_latest.identifier, ) # Deleting the branch should return the RTD's latest branches_data = [ { - "identifier": "origin/master", + "identifier": "master", "verbose_name": "master", }, ] @@ -848,12 +854,15 @@ def test_machine_attr_when_user_define_latest_branch_and_delete_it(self): tags_data=[], ) - # The latest isn't stuck with the previous branch + # The latest isn't stuck with the previous branch, + # but the identifier is still `latest`, + # as the user doesn't have a default branch set. + # The correct identifier will be set on the next build. version_latest = self.pip.versions.get(slug="latest") self.assertIsNone(self.pip.default_branch) self.assertTrue(version_latest.machine) self.assertEqual( - "master", + "latest", version_latest.identifier, ) self.assertTrue(version_latest.machine) @@ -861,11 +870,11 @@ def test_machine_attr_when_user_define_latest_branch_and_delete_it(self): # Test with an explicit default branch. branches_data = [ { - "identifier": "origin/master", + "identifier": "master", "verbose_name": "master", }, { - "identifier": "origin/default-branch", + "identifier": "default-branch", "verbose_name": "default-branch", }, ] From 95720983d161c584a381d528379ca10169092ce8 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 16 Sep 2025 18:36:30 -0500 Subject: [PATCH 03/13] Keep latest in sync with default branch --- readthedocs/builds/models.py | 4 +++ readthedocs/doc_builder/director.py | 12 +++++-- readthedocs/projects/tasks/builds.py | 43 ++++++++++++++++++------- readthedocs/vcs_support/backends/git.py | 32 ++++++++++++++---- 4 files changed, 71 insertions(+), 20 deletions(-) diff --git a/readthedocs/builds/models.py b/readthedocs/builds/models.py index 74b7a7756a2..78708773ab9 100644 --- a/readthedocs/builds/models.py +++ b/readthedocs/builds/models.py @@ -222,6 +222,10 @@ def is_public(self): def is_external(self): return self.type == EXTERNAL + @property + def is_machine_latest(self): + return self.machine and self.slug == LATEST + @property def explicit_name(self): """ diff --git a/readthedocs/doc_builder/director.py b/readthedocs/doc_builder/director.py index ada7d680b13..1c5f38aaa58 100644 --- a/readthedocs/doc_builder/director.py +++ b/readthedocs/doc_builder/director.py @@ -15,7 +15,7 @@ import yaml from django.conf import settings -from readthedocs.builds.constants import EXTERNAL +from readthedocs.builds.constants import EXTERNAL, LATEST from readthedocs.config.config import CONFIG_FILENAME_REGEX from readthedocs.config.find import find_one from readthedocs.core.utils.filesystem import safe_open @@ -223,7 +223,15 @@ def checkout(self): dismissable=True, ) - identifier = self.data.build_commit or self.data.version.identifier + commit = self.data.build_commit + is_rtd_latest = self.data.version.slug == LATEST and self.data.version.machine + # If Feature.DONT_CLEAN_BUILD is enabled, we need to explicitly call checkout + # with the default branch, otherwise we could end up in the wrong branch. + if not commit and is_rtd_latest and not self.data.project.default_branch: + log.info("Using default branch for checkout.") + identifier = self.vcs_repository.get_default_branch() + else: + identifier = commit or self.data.version.identifier log.info("Checking out.", identifier=identifier) self.vcs_repository.checkout(identifier) diff --git a/readthedocs/projects/tasks/builds.py b/readthedocs/projects/tasks/builds.py index bdd2a346872..7a8cd2f3455 100644 --- a/readthedocs/projects/tasks/builds.py +++ b/readthedocs/projects/tasks/builds.py @@ -23,7 +23,7 @@ from readthedocs.api.v2.client import setup_api from readthedocs.builds import tasks as build_tasks -from readthedocs.builds.constants import ARTIFACT_TYPES +from readthedocs.builds.constants import ARTIFACT_TYPES, BRANCH from readthedocs.builds.constants import ARTIFACT_TYPES_WITHOUT_MULTIPLE_FILES_SUPPORT from readthedocs.builds.constants import BUILD_FINAL_STATES from readthedocs.builds.constants import BUILD_STATE_BUILDING @@ -115,6 +115,10 @@ class TaskData: config: BuildConfigV2 = None project: APIProject = None version: APIVersion = None + # Default branch for the project. + # This is used to update the latest version in case the project doesn't + # have an explicit default branch set. + default_branch: str | None = None # Dictionary returned from the API. build: dict = field(default_factory=dict) @@ -644,18 +648,26 @@ def on_success(self, retval, task_id, args, kwargs): # NOTE: we are updating the db version instance *only* when # TODO: remove this condition and *always* update the DB Version instance if "html" in valid_artifacts: + data = { + "built": True, + "documentation_type": self.data.version.documentation_type, + "has_pdf": "pdf" in valid_artifacts, + "has_epub": "epub" in valid_artifacts, + "has_htmlzip": "htmlzip" in valid_artifacts, + "build_data": self.data.version.build_data, + "addons": self.data.version.addons, + } + # Update the latest version to point to the current VCS default branch + # if the project doesn't have an explicit default branch set. + if ( + self.data.version.is_machine_latest + and not self.data.project.default_branch + and self.data.default_branch + ): + data["identifier"] = self.data.default_branch + data["type"] = BRANCH try: - self.data.api_client.version(self.data.version.pk).patch( - { - "built": True, - "documentation_type": self.data.version.documentation_type, - "has_pdf": "pdf" in valid_artifacts, - "has_epub": "epub" in valid_artifacts, - "has_htmlzip": "htmlzip" in valid_artifacts, - "build_data": self.data.version.build_data, - "addons": self.data.version.addons, - } - ) + self.data.api_client.version(self.data.version.pk).patch(data) except HttpClientError: # NOTE: I think we should fail the build if we cannot update # the version at this point. Otherwise, we will have inconsistent data @@ -803,6 +815,13 @@ def execute(self): with self.data.build_director.vcs_environment: self.data.build_director.setup_vcs() + # Get the default branch of the repository if the project doesn't + # have an explicit default branch set and we are building latest. + # The identifier from latest will be updated with this value + # if the build succeeds. + if self.data.version.is_machine_latest and not self.data.project.default_branch: + self.data.default_branch = self.data.build_director.vcs_repository.get_default_branch() + # Sync tags/branches from VCS repository into Read the Docs' # `Version` objects in the database. This method runs commands # (e.g. "hg tags") inside the VCS environment, so it requires to be diff --git a/readthedocs/vcs_support/backends/git.py b/readthedocs/vcs_support/backends/git.py index 3c994d96b28..328a2525f9a 100644 --- a/readthedocs/vcs_support/backends/git.py +++ b/readthedocs/vcs_support/backends/git.py @@ -291,12 +291,13 @@ def fetch(self): "--depth", str(self.repo_depth), ] - remote_reference = self.get_remote_fetch_refspec() - - if remote_reference: - # TODO: We are still fetching the latest 50 commits. - # A PR might have another commit added after the build has started... - cmd.append(remote_reference) + omit_remote_reference = self.version.is_machine_latest and not self.project.default_branch + if not omit_remote_reference: + remote_reference = self.get_remote_fetch_refspec() + if remote_reference: + # TODO: We are still fetching the latest 50 commits. + # A PR might have another commit added after the build has started... + cmd.append(remote_reference) # Log a warning, except for machine versions since it's a known bug that # we haven't stored a remote refspec in Version for those "stable" versions. @@ -388,6 +389,25 @@ def checkout_revision(self, revision): }, ) from exc + def get_default_branch(self): + """ + Return the default branch of the repository. + + The default branch is the branch that is checked out when cloning the + repository. This is usually master or main, it can be configured + in the repository settings. + + The ``git symbolic-ref`` command will produce an output like: + + .. code-block:: text + + origin/main + """ + cmd = ["git", "symbolic-ref", "--short", "refs/remotes/origin/HEAD"] + _, stdout, _ = self.run(*cmd, demux=True, record=False) + default_branch = stdout.strip().removeprefix("origin/") + return default_branch + def lsremote(self, include_tags=True, include_branches=True): """ Use ``git ls-remote`` to list branches and tags without cloning the repository. From 082b1be1183d289ae1fe42e76aa2e8b77df990f5 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 17 Sep 2025 10:41:39 -0500 Subject: [PATCH 04/13] Format --- readthedocs/doc_builder/director.py | 3 ++- readthedocs/projects/tasks/builds.py | 7 +++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/readthedocs/doc_builder/director.py b/readthedocs/doc_builder/director.py index 1c5f38aaa58..6500bb02a5b 100644 --- a/readthedocs/doc_builder/director.py +++ b/readthedocs/doc_builder/director.py @@ -15,7 +15,8 @@ import yaml from django.conf import settings -from readthedocs.builds.constants import EXTERNAL, LATEST +from readthedocs.builds.constants import EXTERNAL +from readthedocs.builds.constants import LATEST from readthedocs.config.config import CONFIG_FILENAME_REGEX from readthedocs.config.find import find_one from readthedocs.core.utils.filesystem import safe_open diff --git a/readthedocs/projects/tasks/builds.py b/readthedocs/projects/tasks/builds.py index 7a8cd2f3455..d3fdc4db63b 100644 --- a/readthedocs/projects/tasks/builds.py +++ b/readthedocs/projects/tasks/builds.py @@ -23,8 +23,9 @@ from readthedocs.api.v2.client import setup_api from readthedocs.builds import tasks as build_tasks -from readthedocs.builds.constants import ARTIFACT_TYPES, BRANCH +from readthedocs.builds.constants import ARTIFACT_TYPES from readthedocs.builds.constants import ARTIFACT_TYPES_WITHOUT_MULTIPLE_FILES_SUPPORT +from readthedocs.builds.constants import BRANCH from readthedocs.builds.constants import BUILD_FINAL_STATES from readthedocs.builds.constants import BUILD_STATE_BUILDING from readthedocs.builds.constants import BUILD_STATE_CANCELLED @@ -820,7 +821,9 @@ def execute(self): # The identifier from latest will be updated with this value # if the build succeeds. if self.data.version.is_machine_latest and not self.data.project.default_branch: - self.data.default_branch = self.data.build_director.vcs_repository.get_default_branch() + self.data.default_branch = ( + self.data.build_director.vcs_repository.get_default_branch() + ) # Sync tags/branches from VCS repository into Read the Docs' # `Version` objects in the database. This method runs commands From 77ae2329a7032582eb394123a31f5493558d3bd7 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 17 Sep 2025 13:16:56 -0500 Subject: [PATCH 05/13] Fix tests --- readthedocs/doc_builder/director.py | 12 +- readthedocs/projects/models.py | 6 +- readthedocs/projects/tasks/builds.py | 12 +- .../projects/tests/test_build_tasks.py | 310 +++++++++++++++++- 4 files changed, 318 insertions(+), 22 deletions(-) diff --git a/readthedocs/doc_builder/director.py b/readthedocs/doc_builder/director.py index 6500bb02a5b..2a8a8bfcdfa 100644 --- a/readthedocs/doc_builder/director.py +++ b/readthedocs/doc_builder/director.py @@ -16,7 +16,6 @@ from django.conf import settings from readthedocs.builds.constants import EXTERNAL -from readthedocs.builds.constants import LATEST from readthedocs.config.config import CONFIG_FILENAME_REGEX from readthedocs.config.find import find_one from readthedocs.core.utils.filesystem import safe_open @@ -224,15 +223,12 @@ def checkout(self): dismissable=True, ) - commit = self.data.build_commit - is_rtd_latest = self.data.version.slug == LATEST and self.data.version.machine + # We can't skip the checkout step. # If Feature.DONT_CLEAN_BUILD is enabled, we need to explicitly call checkout # with the default branch, otherwise we could end up in the wrong branch. - if not commit and is_rtd_latest and not self.data.project.default_branch: - log.info("Using default branch for checkout.") - identifier = self.vcs_repository.get_default_branch() - else: - identifier = commit or self.data.version.identifier + identifier = ( + self.data.build_commit or self.data.default_branch or self.data.version.identifier + ) log.info("Checking out.", identifier=identifier) self.vcs_repository.checkout(identifier) diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index 898effad0ac..112d24e727f 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -1211,9 +1211,9 @@ def update_latest_version(self): # default_branch can be a tag or a branch name! default_version_name = self.get_default_branch(fallback_to_vcs=False) # If the default_branch is not set, it means that the user - # wants to use the default branch of the respository, - # but we don't know what it is here, latest will be updated - # on the next build, when we have cloned the repository. + # wants to use the default branch of the respository, but + # we don't know what that is here, `latest` will be updated + # on the next build. if not default_version_name: return diff --git a/readthedocs/projects/tasks/builds.py b/readthedocs/projects/tasks/builds.py index d3fdc4db63b..9ebe7cd8257 100644 --- a/readthedocs/projects/tasks/builds.py +++ b/readthedocs/projects/tasks/builds.py @@ -116,9 +116,9 @@ class TaskData: config: BuildConfigV2 = None project: APIProject = None version: APIVersion = None - # Default branch for the project. - # This is used to update the latest version in case the project doesn't - # have an explicit default branch set. + # Default branch for the repository. + # Only set when building the latest version, and the project + # doesn't have an explicit default branch. default_branch: str | None = None # Dictionary returned from the API. @@ -660,11 +660,7 @@ def on_success(self, retval, task_id, args, kwargs): } # Update the latest version to point to the current VCS default branch # if the project doesn't have an explicit default branch set. - if ( - self.data.version.is_machine_latest - and not self.data.project.default_branch - and self.data.default_branch - ): + if self.data.default_branch: data["identifier"] = self.data.default_branch data["type"] = BRANCH try: diff --git a/readthedocs/projects/tests/test_build_tasks.py b/readthedocs/projects/tests/test_build_tasks.py index a6fd7482a55..ebff3941878 100644 --- a/readthedocs/projects/tests/test_build_tasks.py +++ b/readthedocs/projects/tests/test_build_tasks.py @@ -315,6 +315,8 @@ def test_build_updates_documentation_type(self, load_yaml_config): "has_pdf": True, "has_epub": True, "has_htmlzip": False, + "identifier": mock.ANY, + "type": "branch", } @pytest.mark.parametrize( @@ -641,6 +643,8 @@ def test_successful_build( "has_pdf": True, "has_epub": True, "has_htmlzip": True, + "identifier": mock.ANY, + "type": "branch", } # Set project has valid clone assert self.requests_mock.request_history[9]._request.method == "PATCH" @@ -931,6 +935,8 @@ def test_successful_build_with_temporary_s3_credentials( "has_pdf": True, "has_epub": True, "has_htmlzip": True, + "identifier": mock.ANY, + "type": "branch", } # Set project has valid clone assert self.requests_mock.request_history[11]._request.method == "PATCH" @@ -1094,7 +1100,7 @@ def test_cancelled_build( } @mock.patch("readthedocs.doc_builder.director.load_yaml_config") - def test_build_commands_executed( + def test_build_commands_executed_latest_version( self, load_yaml_config, ): @@ -1132,7 +1138,298 @@ def test_build_commands_executed( "--prune-tags", "--depth", "50", - "refs/heads/master:refs/remotes/origin/master", + ), + mock.call( + "git", + "show-ref", + "--verify", + "--quiet", + "--", + "refs/remotes/origin/a1b2c3", + record=False, + ), + mock.call("git", "checkout", "--force", "origin/a1b2c3"), + mock.call( + "git", + "symbolic-ref", + "--short", + "refs/remotes/origin/HEAD", + demux=True, + record=False, + ), + mock.call( + "git", + "ls-remote", + "--tags", + "--heads", + mock.ANY, + demux=True, + record=False, + ), + ] + ) + + python_version = settings.RTD_DOCKER_BUILD_SETTINGS["tools"]["python"]["3"] + self.mocker.mocks["environment.run"].assert_has_calls( + [ + # TODO: check for this in the VCS environment. + # We can't check it here because this is the build environment. + # + # mock.call( + # "cat", + # "readthedocs.yml", + # cwd="/tmp/readthedocs-tests/git-repository", + # ), + mock.call("asdf", "install", "python", python_version), + mock.call("asdf", "global", "python", python_version), + mock.call("asdf", "reshim", "python", record=False), + mock.call( + "python", + "-mpip", + "install", + "-U", + "virtualenv", + "setuptools", + ), + mock.call( + "python", + "-mvirtualenv", + "$READTHEDOCS_VIRTUALENV_PATH", + bin_path=None, + cwd=None, + ), + mock.call( + mock.ANY, + "-m", + "pip", + "install", + "--upgrade", + "--no-cache-dir", + "pip", + "setuptools", + bin_path=mock.ANY, + cwd=mock.ANY, + ), + mock.call( + mock.ANY, + "-m", + "pip", + "install", + "--upgrade", + "--no-cache-dir", + "sphinx", + bin_path=mock.ANY, + cwd=mock.ANY, + ), + # FIXME: shouldn't this one be present here? It's not now because + # we are mocking `append_conf` which is the one that triggers this + # command. + # + # mock.call( + # 'cat', + # 'docs/conf.py', + # cwd=mock.ANY, + # ), + mock.call( + mock.ANY, + "-m", + "sphinx", + "-T", + "-b", + "html", + "-d", + "_build/doctrees", + "-D", + "language=en", + ".", + "$READTHEDOCS_OUTPUT/html", + cwd=mock.ANY, + bin_path=mock.ANY, + ), + mock.call( + mock.ANY, + "-m", + "sphinx", + "-T", + "-b", + "singlehtml", + "-d", + "_build/doctrees", + "-D", + "language=en", + ".", + "$READTHEDOCS_OUTPUT/htmlzip", + cwd=mock.ANY, + bin_path=mock.ANY, + ), + mock.call( + "mktemp", + "--directory", + record=False, + ), + mock.call( + "mv", + mock.ANY, + mock.ANY, + cwd=mock.ANY, + record=False, + ), + mock.call( + "mkdir", + "--parents", + mock.ANY, + cwd=mock.ANY, + record=False, + ), + mock.call( + "zip", + "--recurse-paths", + "--symlinks", + mock.ANY, + mock.ANY, + cwd=mock.ANY, + record=False, + ), + mock.call( + mock.ANY, + "-m", + "sphinx", + "-T", + "-b", + "latex", + "-d", + "_build/doctrees", + "-D", + "language=en", + ".", + "$READTHEDOCS_OUTPUT/pdf", + cwd=mock.ANY, + bin_path=mock.ANY, + ), + mock.call("cat", "latexmkrc", cwd=mock.ANY), + # NOTE: pdf `mv` commands and others are not here because the + # PDF resulting file is not found in the process (`_post_build`) + mock.call( + mock.ANY, + "-m", + "sphinx", + "-T", + "-b", + "epub", + "-d", + "_build/doctrees", + "-D", + "language=en", + ".", + "$READTHEDOCS_OUTPUT/epub", + cwd=mock.ANY, + bin_path=mock.ANY, + ), + mock.call( + "mv", + mock.ANY, + "/tmp/project-latest.epub", + cwd=mock.ANY, + record=False, + ), + mock.call( + "rm", + "--recursive", + "$READTHEDOCS_OUTPUT/epub", + cwd=mock.ANY, + record=False, + ), + mock.call( + "mkdir", + "--parents", + "$READTHEDOCS_OUTPUT/epub", + cwd=mock.ANY, + record=False, + ), + mock.call( + "mv", + "/tmp/project-latest.epub", + mock.ANY, + cwd=mock.ANY, + record=False, + ), + mock.call( + "test", + "-x", + "_build/html", + record=False, + cwd=mock.ANY, + ), + # FIXME: I think we are hitting this issue here: + # https://github.com/pytest-dev/pytest-mock/issues/234 + mock.call("lsb_release", "--description", record=False, demux=True), + mock.call("python", "--version", record=False, demux=True), + mock.call( + "dpkg-query", + "--showformat", + "${package} ${version}\\n", + "--show", + record=False, + demux=True, + ), + mock.call( + "python", + "-m", + "pip", + "list", + "--pre", + "--local", + "--format", + "json", + record=False, + demux=True, + ), + ] + ) + + @mock.patch("readthedocs.doc_builder.director.load_yaml_config") + def test_build_commands_executed_non_machine_version( + self, + load_yaml_config, + ): + self.version.machine = False + self.version.save() + + load_yaml_config.return_value = get_build_config( + { + "version": 2, + "formats": "all", + "sphinx": { + "configuration": "docs/conf.py", + }, + }, + validate=True, + ) + + # Create the artifact paths, so it's detected by the builder + os.makedirs(self.project.artifact_path(version=self.version.slug, type_="html")) + os.makedirs(self.project.artifact_path(version=self.version.slug, type_="json")) + os.makedirs( + self.project.artifact_path(version=self.version.slug, type_="htmlzip") + ) + os.makedirs(self.project.artifact_path(version=self.version.slug, type_="epub")) + os.makedirs(self.project.artifact_path(version=self.version.slug, type_="pdf")) + + self._trigger_update_docs_task() + + self.mocker.mocks["git.Backend.run"].assert_has_calls( + [ + mock.call("git", "clone", "--depth", "1", mock.ANY, "."), + mock.call( + "git", + "fetch", + "origin", + "--force", + "--prune", + "--prune-tags", + "--depth", + "50", + "refs/heads/latest:refs/remotes/origin/latest", ), mock.call( "git", @@ -1432,7 +1729,6 @@ def test_build_commands_executed_with_clone_token( "--prune-tags", "--depth", "50", - "refs/heads/master:refs/remotes/origin/master", ), mock.call( "git", @@ -1444,6 +1740,14 @@ def test_build_commands_executed_with_clone_token( record=False, ), mock.call("git", "checkout", "--force", "origin/a1b2c3"), + mock.call( + "git", + "symbolic-ref", + "--short", + "refs/remotes/origin/HEAD", + demux=True, + record=False, + ), mock.call( "git", "ls-remote", From f636eb9215a38e7dc8b63e955087239c8ac54582 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 17 Sep 2025 16:33:22 -0500 Subject: [PATCH 06/13] Test --- readthedocs/rtd_tests/tests/test_backend.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/readthedocs/rtd_tests/tests/test_backend.py b/readthedocs/rtd_tests/tests/test_backend.py index dd825d981b8..2b71af7cb4e 100644 --- a/readthedocs/rtd_tests/tests/test_backend.py +++ b/readthedocs/rtd_tests/tests/test_backend.py @@ -145,6 +145,26 @@ def test_git_lsremote_branches_only(self): {branch: branch for branch in default_branches + branches}, {branch.verbose_name: branch.identifier for branch in repo_branches}, ) + + def test_get_default_branch(self): + version = self.project.versions.first() + repo = self.project.vcs_repo(environment=self.build_environment, version=version) + repo.update() + default_branch = repo.get_default_branch() + assert default_branch == "master" + + version = get( + Version, + project=self.project, + type=BRANCH, + identifier="submodule", + verbose_name="submodule", + ) + repo = self.project.vcs_repo(environment=self.build_environment, version=version) + repo.update() + repo.checkout("submodule") + default_branch = repo.get_default_branch() + assert default_branch == "master" def test_git_update_and_checkout(self): version = self.project.versions.first() From 8701e3c3f4ae204045f475a95a431ab4ca7163f7 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 17 Sep 2025 17:02:05 -0500 Subject: [PATCH 07/13] Fix tests --- readthedocs/proxito/tests/test_hosting.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/readthedocs/proxito/tests/test_hosting.py b/readthedocs/proxito/tests/test_hosting.py index 13157608f98..8d013ab8294 100644 --- a/readthedocs/proxito/tests/test_hosting.py +++ b/readthedocs/proxito/tests/test_hosting.py @@ -408,7 +408,7 @@ def test_number_of_queries_versions_with_downloads(self): active=True, ) - with self.assertNumQueries(13): + with self.assertNumQueries(14): r = self.client.get( reverse("proxito_readthedocs_docs_addons"), { @@ -851,7 +851,7 @@ def test_number_of_queries_project_version_slug(self): active=True, ) - with self.assertNumQueries(14): + with self.assertNumQueries(16): r = self.client.get( reverse("proxito_readthedocs_docs_addons"), { @@ -880,7 +880,7 @@ def test_number_of_queries_url(self): active=True, ) - with self.assertNumQueries(15): + with self.assertNumQueries(17): r = self.client.get( reverse("proxito_readthedocs_docs_addons"), { @@ -932,7 +932,7 @@ def test_number_of_queries_url_subproject(self): assert r.status_code == 200 # Test parent project has fewer queries - with self.assertNumQueries(14): + with self.assertNumQueries(16): r = self.client.get( reverse("proxito_readthedocs_docs_addons"), { @@ -958,7 +958,7 @@ def test_number_of_queries_url_translations(self): language=language, ) - with self.assertNumQueries(24): + with self.assertNumQueries(26): r = self.client.get( reverse("proxito_readthedocs_docs_addons"), { @@ -1108,7 +1108,7 @@ def test_file_tree_diff(self, get_manifest): ], ), ] - with self.assertNumQueries(19): + with self.assertNumQueries(20): r = self.client.get( reverse("proxito_readthedocs_docs_addons"), { From 335fff839534fea048223d29f1008df6d649860f Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 17 Sep 2025 17:17:41 -0500 Subject: [PATCH 08/13] Format --- readthedocs/rtd_tests/tests/test_backend.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/rtd_tests/tests/test_backend.py b/readthedocs/rtd_tests/tests/test_backend.py index 2b71af7cb4e..d403208d868 100644 --- a/readthedocs/rtd_tests/tests/test_backend.py +++ b/readthedocs/rtd_tests/tests/test_backend.py @@ -145,7 +145,7 @@ def test_git_lsremote_branches_only(self): {branch: branch for branch in default_branches + branches}, {branch.verbose_name: branch.identifier for branch in repo_branches}, ) - + def test_get_default_branch(self): version = self.project.versions.first() repo = self.project.vcs_repo(environment=self.build_environment, version=version) From c7e008717cc94d5fa0bc546b6f7155695f1009b4 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 17 Sep 2025 17:25:50 -0500 Subject: [PATCH 09/13] We don't need this --- .../projects/tests/test_build_tasks.py | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/readthedocs/projects/tests/test_build_tasks.py b/readthedocs/projects/tests/test_build_tasks.py index ebff3941878..77cb7c9b3b1 100644 --- a/readthedocs/projects/tests/test_build_tasks.py +++ b/readthedocs/projects/tests/test_build_tasks.py @@ -1456,14 +1456,6 @@ def test_build_commands_executed_non_machine_version( python_version = settings.RTD_DOCKER_BUILD_SETTINGS["tools"]["python"]["3"] self.mocker.mocks["environment.run"].assert_has_calls( [ - # TODO: check for this in the VCS environment. - # We can't check it here because this is the build environment. - # - # mock.call( - # "cat", - # "readthedocs.yml", - # cwd="/tmp/readthedocs-tests/git-repository", - # ), mock.call("asdf", "install", "python", python_version), mock.call("asdf", "global", "python", python_version), mock.call("asdf", "reshim", "python", record=False), @@ -1505,15 +1497,6 @@ def test_build_commands_executed_non_machine_version( bin_path=mock.ANY, cwd=mock.ANY, ), - # FIXME: shouldn't this one be present here? It's not now because - # we are mocking `append_conf` which is the one that triggers this - # command. - # - # mock.call( - # 'cat', - # 'docs/conf.py', - # cwd=mock.ANY, - # ), mock.call( mock.ANY, "-m", @@ -1644,8 +1627,6 @@ def test_build_commands_executed_non_machine_version( record=False, cwd=mock.ANY, ), - # FIXME: I think we are hitting this issue here: - # https://github.com/pytest-dev/pytest-mock/issues/234 mock.call("lsb_release", "--description", record=False, demux=True), mock.call("python", "--version", record=False, demux=True), mock.call( From c9e7138bc7b22fc6c32421a3a39b70539e6d54a5 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 17 Sep 2025 17:48:53 -0500 Subject: [PATCH 10/13] Default branch should be resolved before checkout --- readthedocs/doc_builder/director.py | 11 +++++++++++ readthedocs/projects/tasks/builds.py | 9 --------- readthedocs/projects/tests/test_build_tasks.py | 4 ++-- 3 files changed, 13 insertions(+), 11 deletions(-) diff --git a/readthedocs/doc_builder/director.py b/readthedocs/doc_builder/director.py index 2a8a8bfcdfa..4cc9eb6308c 100644 --- a/readthedocs/doc_builder/director.py +++ b/readthedocs/doc_builder/director.py @@ -223,6 +223,17 @@ def checkout(self): dismissable=True, ) + # Get the default branch of the repository if the project doesn't + # have an explicit default branch set and we are building latest. + # The identifier from latest will be updated with this value + # if the build succeeds. + if self.data.version.is_machine_latest and not self.data.project.default_branch: + self.data.default_branch = self.data.build_director.vcs_repository.get_default_branch() + log.info( + "Default branch for the repository detected.", + default_branch=self.data.default_branch, + ) + # We can't skip the checkout step. # If Feature.DONT_CLEAN_BUILD is enabled, we need to explicitly call checkout # with the default branch, otherwise we could end up in the wrong branch. diff --git a/readthedocs/projects/tasks/builds.py b/readthedocs/projects/tasks/builds.py index 9ebe7cd8257..95cf54c4d54 100644 --- a/readthedocs/projects/tasks/builds.py +++ b/readthedocs/projects/tasks/builds.py @@ -812,15 +812,6 @@ def execute(self): with self.data.build_director.vcs_environment: self.data.build_director.setup_vcs() - # Get the default branch of the repository if the project doesn't - # have an explicit default branch set and we are building latest. - # The identifier from latest will be updated with this value - # if the build succeeds. - if self.data.version.is_machine_latest and not self.data.project.default_branch: - self.data.default_branch = ( - self.data.build_director.vcs_repository.get_default_branch() - ) - # Sync tags/branches from VCS repository into Read the Docs' # `Version` objects in the database. This method runs commands # (e.g. "hg tags") inside the VCS environment, so it requires to be diff --git a/readthedocs/projects/tests/test_build_tasks.py b/readthedocs/projects/tests/test_build_tasks.py index 77cb7c9b3b1..5132a1813ea 100644 --- a/readthedocs/projects/tests/test_build_tasks.py +++ b/readthedocs/projects/tests/test_build_tasks.py @@ -1148,7 +1148,6 @@ def test_build_commands_executed_latest_version( "refs/remotes/origin/a1b2c3", record=False, ), - mock.call("git", "checkout", "--force", "origin/a1b2c3"), mock.call( "git", "symbolic-ref", @@ -1157,6 +1156,7 @@ def test_build_commands_executed_latest_version( demux=True, record=False, ), + mock.call("git", "checkout", "--force", "origin/a1b2c3"), mock.call( "git", "ls-remote", @@ -1720,7 +1720,6 @@ def test_build_commands_executed_with_clone_token( "refs/remotes/origin/a1b2c3", record=False, ), - mock.call("git", "checkout", "--force", "origin/a1b2c3"), mock.call( "git", "symbolic-ref", @@ -1729,6 +1728,7 @@ def test_build_commands_executed_with_clone_token( demux=True, record=False, ), + mock.call("git", "checkout", "--force", "origin/a1b2c3"), mock.call( "git", "ls-remote", From 1723c27d2c5a4e03d16fbf9bffc0eca7975c29c4 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 17 Sep 2025 17:52:05 -0500 Subject: [PATCH 11/13] Fix order again --- .../projects/tests/test_build_tasks.py | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/readthedocs/projects/tests/test_build_tasks.py b/readthedocs/projects/tests/test_build_tasks.py index 5132a1813ea..9aee9efb521 100644 --- a/readthedocs/projects/tests/test_build_tasks.py +++ b/readthedocs/projects/tests/test_build_tasks.py @@ -1139,6 +1139,14 @@ def test_build_commands_executed_latest_version( "--depth", "50", ), + mock.call( + "git", + "symbolic-ref", + "--short", + "refs/remotes/origin/HEAD", + demux=True, + record=False, + ), mock.call( "git", "show-ref", @@ -1148,14 +1156,6 @@ def test_build_commands_executed_latest_version( "refs/remotes/origin/a1b2c3", record=False, ), - mock.call( - "git", - "symbolic-ref", - "--short", - "refs/remotes/origin/HEAD", - demux=True, - record=False, - ), mock.call("git", "checkout", "--force", "origin/a1b2c3"), mock.call( "git", @@ -1711,6 +1711,14 @@ def test_build_commands_executed_with_clone_token( "--depth", "50", ), + mock.call( + "git", + "symbolic-ref", + "--short", + "refs/remotes/origin/HEAD", + demux=True, + record=False, + ), mock.call( "git", "show-ref", @@ -1720,14 +1728,6 @@ def test_build_commands_executed_with_clone_token( "refs/remotes/origin/a1b2c3", record=False, ), - mock.call( - "git", - "symbolic-ref", - "--short", - "refs/remotes/origin/HEAD", - demux=True, - record=False, - ), mock.call("git", "checkout", "--force", "origin/a1b2c3"), mock.call( "git", From 722617cf8bf338148a39c2faeb7c35b0a75daa5e Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 9 Oct 2025 16:58:16 -0500 Subject: [PATCH 12/13] Comment --- readthedocs/vcs_support/backends/git.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/readthedocs/vcs_support/backends/git.py b/readthedocs/vcs_support/backends/git.py index 328a2525f9a..01a58f3ff95 100644 --- a/readthedocs/vcs_support/backends/git.py +++ b/readthedocs/vcs_support/backends/git.py @@ -291,6 +291,8 @@ def fetch(self): "--depth", str(self.repo_depth), ] + # Skip adding a remote reference if we are building "latest", + # and the user hasn't defined a default branch (which means we need to use the default branch from the repo). omit_remote_reference = self.version.is_machine_latest and not self.project.default_branch if not omit_remote_reference: remote_reference = self.get_remote_fetch_refspec() From a3374b5943f8c33fb284b804d8306b584adcbf85 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 9 Oct 2025 17:57:34 -0500 Subject: [PATCH 13/13] Skip checkout --- readthedocs/doc_builder/director.py | 19 +++++++++--------- .../projects/tests/test_build_tasks.py | 20 ------------------- 2 files changed, 10 insertions(+), 29 deletions(-) diff --git a/readthedocs/doc_builder/director.py b/readthedocs/doc_builder/director.py index 9c2f3918c1c..aa5df137e94 100644 --- a/readthedocs/doc_builder/director.py +++ b/readthedocs/doc_builder/director.py @@ -227,21 +227,22 @@ def checkout(self): # have an explicit default branch set and we are building latest. # The identifier from latest will be updated with this value # if the build succeeds. - if self.data.version.is_machine_latest and not self.data.project.default_branch: + is_latest_without_default_branch = ( + self.data.version.is_machine_latest and not self.data.project.default_branch + ) + if is_latest_without_default_branch: self.data.default_branch = self.data.build_director.vcs_repository.get_default_branch() log.info( "Default branch for the repository detected.", default_branch=self.data.default_branch, ) - # We can't skip the checkout step. - # If Feature.DONT_CLEAN_BUILD is enabled, we need to explicitly call checkout - # with the default branch, otherwise we could end up in the wrong branch. - identifier = ( - self.data.build_commit or self.data.default_branch or self.data.version.identifier - ) - log.info("Checking out.", identifier=identifier) - self.vcs_repository.checkout(identifier) + # We can skip the checkout step since we just cloned the repository, + # and the default branch is already checked out. + if not is_latest_without_default_branch: + identifier = self.data.build_commit or self.data.version.identifier + log.info("Checking out.", identifier=identifier) + self.vcs_repository.checkout(identifier) # The director is responsible for understanding which config file to use for a build. # In order to reproduce a build 1:1, we may use readthedocs_yaml_path defined by the build diff --git a/readthedocs/projects/tests/test_build_tasks.py b/readthedocs/projects/tests/test_build_tasks.py index 84f50062af1..23a1fabe969 100644 --- a/readthedocs/projects/tests/test_build_tasks.py +++ b/readthedocs/projects/tests/test_build_tasks.py @@ -1159,16 +1159,6 @@ def test_build_commands_executed_latest_version( demux=True, record=False, ), - mock.call( - "git", - "show-ref", - "--verify", - "--quiet", - "--", - "refs/remotes/origin/a1b2c3", - record=False, - ), - mock.call("git", "checkout", "--force", "origin/a1b2c3"), mock.call( "git", "ls-remote", @@ -1731,16 +1721,6 @@ def test_build_commands_executed_with_clone_token( demux=True, record=False, ), - mock.call( - "git", - "show-ref", - "--verify", - "--quiet", - "--", - "refs/remotes/origin/a1b2c3", - record=False, - ), - mock.call("git", "checkout", "--force", "origin/a1b2c3"), mock.call( "git", "ls-remote",