From fda0a56d604654e36e815301d126c9cef144c589 Mon Sep 17 00:00:00 2001 From: nhadji Date: Mon, 27 Jul 2026 11:02:19 +0200 Subject: [PATCH 1/2] Bug 2048352 - Split the push workflow into VCS agnostic base classes Extract the version control agnostic parts of the mercurial push workflow into BaseRepository and BaseWorker (new vcs module): building and applying a stack of Phabricator patches, committing try_task_config.json, the skippable files detection, the push retry loop and the worker output handling. The Mercurial specific parts stay in MercurialRepository (renamed from Repository to avoid confusion with the upcoming Git implementation) and MercurialWorker: robustcheckout and batch clones, revision lookups with the Lando git2hg fallback, patch application through hg import, ssh push and the treestatus wait. No behavior change: this prepares the introduction of a Git implementation of the same workflow as subclasses of the new bases. --- bot/code_review_bot/mercurial.py | 406 +++++++------------------------ bot/code_review_bot/vcs.py | 355 +++++++++++++++++++++++++++ bot/code_review_bot/workflow.py | 8 +- bot/tests/conftest.py | 6 +- bot/tests/test_mercurial.py | 26 +- 5 files changed, 465 insertions(+), 336 deletions(-) create mode 100644 bot/code_review_bot/vcs.py diff --git a/bot/code_review_bot/mercurial.py b/bot/code_review_bot/mercurial.py index 8a0a764b2..ae0b37e99 100644 --- a/bot/code_review_bot/mercurial.py +++ b/bot/code_review_bot/mercurial.py @@ -5,7 +5,6 @@ import atexit import fcntl import io -import json import os import tempfile import time @@ -13,33 +12,19 @@ import hglib import requests -import rs_parsepatch import structlog from libmozdata.lando import LandoCommitMapAPI, LandoMissingCommit from libmozdata.phabricator import PhabricatorPatch -from code_review_bot.sources.phabricator import PhabricatorBuild +from code_review_bot.vcs import BaseRepository, BaseWorker logger = structlog.get_logger(__name__) -TREEHERDER_URL = "https://treeherder.mozilla.org/#/jobs?repo={}&revision={}" DEFAULT_AUTHOR = "code review bot " # On build failure, check try status until available every 5 minutes and up to 24h TRY_STATUS_URL = "https://treestatus.prod.lando.prod.cloudops.mozgcp.net/trees/try" TRY_STATUS_DELAY = 5 * 60 TRY_STATUS_MAX_WAIT = 24 * 60 * 60 -# Number of allowed retries on an unexpected push fail -MAX_PUSH_RETRIES = 4 -# Wait successive exponential delays: 6sec, 36sec, 3.6min, 21.6min -PUSH_RETRY_EXPONENTIAL_DELAY = 6 - -logger = structlog.get_logger(__name__) - - -class RetryNeeded(Exception): - """ - Raised when retrying a mercurial build is needed - """ def hg_run(cmd): @@ -146,32 +131,23 @@ def robust_checkout( hg_run(cmd) -class Repository: +class MercurialRepository(BaseRepository): """ A Mercurial repository with its try server credentials """ + DEFAULT_REVISION = "default" + def __init__(self, config, cache_root): - assert isinstance(config, dict) - self.name = config["name"] - self.url = config["url"] - self.dir = os.path.join(cache_root, config["name"]) + super().__init__(config, cache_root) self.share_base_dir = os.path.join(cache_root, f"{config['name']}-shared") self.checkout_mode = config.get("checkout", "batch") self.batch_size = config.get("batch_size", 10000) - self.try_url = config["try_url"] - self.try_name = config.get("try_name", "try") - self.default_revision = config.get("default_revision", "default") - - # Apply patches to the latest revision when `True`. - self.use_latest_revision = config.get("use_latest_revision", False) # Crash when configuration requests try syntax if config.get("try_mode") == "syntax": raise Exception("Try syntax mode is deprecated") - self._repo = None - # Write ssh key from secret _, self.ssh_key_path = tempfile.mkstemp(suffix=".key") with open(self.ssh_key_path, "w") as f: @@ -190,9 +166,6 @@ def __init__(self, config, cache_root): # Remove key when finished atexit.register(self.end_of_life) - def __str__(self): - return self.name - def end_of_life(self): os.unlink(self.ssh_key_path) logger.info("Removed ssh key") @@ -276,57 +249,11 @@ def get_base_identifier(self, needed_stack: list[PhabricatorPatch]) -> str: # Base revision may reference a Git hash on new repositories return self.get_mercurial_base_hash(base_rev_hash) - def apply_build(self, build): - """ - Apply a stack of patches to mercurial repo - and commit them one by one - """ - assert isinstance(build, PhabricatorBuild) - assert len(build.stack) > 0, "No patches to apply" - assert all(map(lambda p: isinstance(p, PhabricatorPatch), build.stack)) - - # Find the first unknown base revision - needed_stack = [] - for patch in reversed(build.stack): - # Skip already merged patches - if patch.merged: - logger.info( - f"Skip applying patch {patch.id} as it's already been merged upstream" - ) - continue - - # Add the patch into the stack only if not already merged ! - needed_stack.insert(0, patch) - - # Stop as soon as a base revision is available - if self.has_revision(patch.base_revision): - logger.info(f"Stopping at revision {patch.base_revision}") - break - - if not needed_stack: - logger.info("All the patches are already applied") - return - - hg_base = self.get_base_identifier(needed_stack) - - # When base revision is missing, update to default revision - build.base_revision = hg_base - build.missing_base_revision = not self.has_revision(hg_base) - if build.missing_base_revision: - logger.warning( - "Missing base revision from Phabricator", - revision=hg_base, - fallback=self.default_revision, - ) - hg_base = self.default_revision - - # Store the actual base revision we used - build.actual_base_revision = hg_base - - # Update the repo to base revision + def checkout_base(self, base): + """Update the local checkout to the base revision""" try: - logger.info(f"Updating repo to revision {hg_base}") - self.repo.update(rev=hg_base, clean=True) + logger.info(f"Updating repo to revision {base}") + self.repo.update(rev=base, clean=True) # See if the repo is clean repo_status = self.repo.status( @@ -335,111 +262,79 @@ def apply_build(self, build): if len(repo_status) != 0: logger.warn( "Repo is dirty!", - revision=hg_base, + revision=base, repo=self.name, repo_status=repo_status, ) except hglib.error.CommandError: - raise Exception(f"Failed to update to revision {hg_base}") - - # In this case revision is `hg_base` - logger.info("Updated repo", revision=hg_base, repo=self.name) - - def get_author(commit): - """Helper to build a mercurial author from Phabricator data""" - author = commit.get("author") - if author is None: - return DEFAULT_AUTHOR - if author["name"] and author["email"]: - # Build clean version without quotes - return f"{author['name']} <{author['email']}>" - return author["raw"] - - for patch in needed_stack: - if patch.commits: - # Use the first commit only - commit = patch.commits[0] - message = "{}\n".format(commit["message"]) - user = get_author(commit) - else: - # We should always have some commits here - logger.warning("Missing commit on patch", id=patch.id) - message = "" - user = DEFAULT_AUTHOR - message += f"Differential Diff: {patch.phid}" - - logger.info("Applying patch", phid=patch.phid, message=message) - patches = io.BytesIO(patch.patch.encode("utf-8")) - try: - self.repo.import_( - patches=patches, + raise Exception(f"Failed to update to revision {base}") + + # In this case revision is `base` + logger.info("Updated repo", revision=base, repo=self.name) + + @staticmethod + def get_author(commit): + """Helper to build a mercurial author from Phabricator data""" + author = commit.get("author") + if author is None: + return DEFAULT_AUTHOR + if author["name"] and author["email"]: + # Build clean version without quotes + return f"{author['name']} <{author['email']}>" + return author["raw"] + + def apply_patch(self, patch, message, commit): + """Apply a single patch on the local checkout and commit it""" + user = self.get_author(commit) if commit else DEFAULT_AUTHOR + + patches = io.BytesIO(patch.patch.encode("utf-8")) + try: + self.repo.import_( + patches=patches, + message=message.encode("utf-8"), + user=user.encode("utf-8"), + similarity=95, + ) + except hglib.error.CommandError as e: + logger.warning( + ( + f"Mercurial command from hglib failed: {e}. " + "Retrying with --config ui.patch=patch." + ), + phid=patch.phid, + exc_info=True, + ) + patches.seek(0) + # Same method as repo.import_() but with the extra argument "--config ui.patch=patch". + # https://repo.mercurial-scm.org/python-hglib/file/484b56ac4aec/hglib/client.py#l959 + self.repo.rawcommand( + hglib.util.cmdbuilder( + b"import", message=message.encode("utf-8"), user=user.encode("utf-8"), similarity=95, + config="ui.patch=patch", + *patches, ) - except hglib.error.CommandError as e: - logger.warning( - ( - f"Mercurial command from hglib failed: {e}. " - "Retrying with --config ui.patch=patch." - ), - phid=patch.phid, - exc_info=True, - ) - patches.seek(0) - # Same method as repo.import_() but with the extra argument "--config ui.patch=patch". - # https://repo.mercurial-scm.org/python-hglib/file/484b56ac4aec/hglib/client.py#l959 - self.repo.rawcommand( - hglib.util.cmdbuilder( - b"import", - message=message.encode("utf-8"), - user=user.encode("utf-8"), - similarity=95, - config="ui.patch=patch", - *patches, - ) - ) - # When using an external patch util mercurial won't automatically handle add/remove/renames - self.repo.rawcommand( - hglib.util.cmdbuilder( - b"addremove", - similarity=95, - ) - ) - except Exception as e: - logger.info( - f"Failed to apply patch: {e}", - phid=patch.phid, - exc_info=True, + ) + # When using an external patch util mercurial won't automatically handle add/remove/renames + self.repo.rawcommand( + hglib.util.cmdbuilder( + b"addremove", + similarity=95, ) - raise - - def add_try_commit(self, build): - """ - Build and commit the file configuring try - with try_task_config.json and the code-review workflow parameters in JSON - """ - path = os.path.join(self.dir, "try_task_config.json") - config = { - "version": 2, - "parameters": { - "target_tasks_method": "codereview", - "optimize_target_tasks": True, - "phabricator_diff": build.target_phid, - }, - } - diff_phid = build.stack[-1].phid - - if build.revision_url: - message = f"try_task_config for {build.revision_url}" - else: - message = "try_task_config for code-review" - message += f"\nDifferential Diff: {diff_phid}" + ) + except Exception as e: + logger.info( + f"Failed to apply patch: {e}", + phid=patch.phid, + exc_info=True, + ) + raise - # Write content as json and commit it - with open(path, "w") as f: - json.dump(config, f, sort_keys=True, indent=4) + def commit_try_task_config(self, path, message): + """Commit the try_task_config.json file as the bot""" self.repo.add(path.encode("utf-8")) self.repo.commit(message=message, user=DEFAULT_AUTHOR) @@ -457,6 +352,10 @@ def push_to_try(self): ) return tip + def revision_id(self, tip): + """Extract the revision identifier from a push_to_try result""" + return tip.node.decode("utf-8") + def clean(self): """ Steps to clean the mercurial repo @@ -478,11 +377,16 @@ def clean(self): self.repo.pull() -class MercurialWorker: +class MercurialWorker(BaseWorker): """ Mercurial worker maintaining several local clones """ + VCS_ERROR = hglib.error.CommandError + VCS_NAME = "Mercurial" + FAILURE_MODE = "fail:mercurial" + REPOSITORY_CLASS = MercurialRepository + ELIGIBLE_RETRY_ERRORS = [ error.lower() for error in [ @@ -492,67 +396,6 @@ class MercurialWorker: ] ] - def __init__( - self, - skippable_files=[], - ): - self.skippable_files = skippable_files - - def run(self, repository, build): - """ - Apply the stack of patches from the build, handling retries - in case of try server errors - """ - while build.retries <= MAX_PUSH_RETRIES: - start = time.time() - - if build.retries: - logger.warning( - "Trying to apply build's diff after a remote push error " - f"[{build.retries}/{MAX_PUSH_RETRIES}]" - ) - - try: - return self.handle_build(repository, build) - except RetryNeeded: - build.retries += 1 - - if build.retries > MAX_PUSH_RETRIES: - error_log = "Max number of retries has been reached pushing the build to try repository" - logger.warn("Mercurial error on diff", error=error_log, build=build) - return ( - "fail:mercurial", - build, - {"message": error_log, "duration": time.time() - start}, - ) - - # Ensure try is opened - self.wait_try_available() - - # Wait an exponential time before retrying the build - delay = PUSH_RETRY_EXPONENTIAL_DELAY**build.retries - logger.info( - f"An error occurred pushing the build to try, retrying after {delay}s" - ) - time.sleep(delay) - - def is_commit_skippable(self, build): - def get_files_touched_in_diff(rawdiff): - patched = [] - for parsed_diff in rs_parsepatch.get_diffs(rawdiff): - # filename is sometimes of format 'test.txt Tue Feb 05 17:23:40 2019 +0100' - # fix after https://github.com/mozilla/rust-parsepatch/issues/61 - if "filename" in parsed_diff: - filename = parsed_diff["filename"].split(" ")[0] - patched.append(filename) - return patched - - return any( - patched_file in self.skippable_files - for rev in build.stack - for patched_file in get_files_touched_in_diff(rev.patch) - ) - def wait_try_available(self): """ Wait until try status is "open" @@ -582,84 +425,9 @@ def get_status(): ) time.sleep(TRY_STATUS_DELAY) - def is_eligible_for_retry(self, error): - """ - Given a Mercurial error message, if it's an error likely due to a - temporary connection problem, consider it as eligible for retry. - """ - error = error.lower() - return any( - eligible_message in error for eligible_message in self.ELIGIBLE_RETRY_ERRORS - ) - - def handle_build(self, repository, build): - """ - Try to load and apply a diff on local clone - If successful, push to try and send a treeherder link - In case of an unexpected push failure, retry up to MAX_PUSH_RETRIES - times by putting the build task back in the queue - - If the build fail, send a unit result with a warning message - """ - assert isinstance(repository, Repository) - start = time.time() - - try: - # Start by cleaning the repo - repository.clean() - - # First apply patches on local repo - repository.apply_build(build) - - # Check Eligibility: some commits don't need to be pushed to try. - if self.is_commit_skippable(build): - logger.info("This patch series is ineligible for automated try push") - return ( - "fail:ineligible", - build, - { - "message": "Modified files match skippable internal configuration files", - "duration": time.time() - start, - }, - ) - - # Configure the try task - repository.add_try_commit(build) - - # Then push that stack on try - tip = repository.push_to_try() - logger.info("Diff has been pushed !") - - # Publish Treeherder link - uri = TREEHERDER_URL.format(repository.try_name, tip.node.decode("utf-8")) - except hglib.error.CommandError as e: - # Format nicely the error log - error_log = e.err - if isinstance(error_log, bytes): - error_log = error_log.decode("utf-8") - - if self.is_eligible_for_retry(error_log): - raise RetryNeeded - - logger.warn( - "Mercurial error on diff", error=error_log, args=e.args, build=build - ) - return ( - "fail:mercurial", - build, - {"message": error_log, "duration": time.time() - start}, - ) - - except Exception as e: - logger.warn("Failed to process diff", error=e, build=build) - return ( - "fail:general", - build, - {"message": str(e), "duration": time.time() - start}, - ) - - return ( - "success", - build, - {"treeherder_url": uri, "revision": tip.node.decode("utf-8")}, - ) + def format_error(self, error): + """Extract a readable error log from a Mercurial exception""" + error_log = error.err + if isinstance(error_log, bytes): + error_log = error_log.decode("utf-8") + return error_log diff --git a/bot/code_review_bot/vcs.py b/bot/code_review_bot/vcs.py new file mode 100644 index 000000000..01341f927 --- /dev/null +++ b/bot/code_review_bot/vcs.py @@ -0,0 +1,355 @@ +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, You can obtain one at http://mozilla.org/MPL/2.0/. + +import json +import os +import time + +import rs_parsepatch +import structlog +from libmozdata.phabricator import PhabricatorPatch + +from code_review_bot.sources.phabricator import PhabricatorBuild + +logger = structlog.get_logger(__name__) + +TREEHERDER_URL = "https://treeherder.mozilla.org/#/jobs?repo={}&revision={}" + +# Number of allowed retries on an unexpected push fail +MAX_PUSH_RETRIES = 4 +# Wait successive exponential delays: 6sec, 36sec, 3.6min, 21.6min +PUSH_RETRY_EXPONENTIAL_DELAY = 6 + + +class RetryNeeded(Exception): + """ + Raised when retrying a build is needed + """ + + +class BaseRepository: + """ + A repository with its try server credentials, independent of the + version control system. + + Subclasses provide the VCS specific operations: cloning, revision + lookups, applying a patch as a commit and pushing to the try server. + """ + + # Revision to apply patches on when the base revision is unknown + DEFAULT_REVISION = None + + def __init__(self, config, cache_root): + assert isinstance(config, dict) + self.name = config["name"] + self.url = config["url"] + self.dir = os.path.join(cache_root, config["name"]) + self.try_url = config["try_url"] + self.try_name = config.get("try_name", "try") + self.default_revision = config.get("default_revision", self.DEFAULT_REVISION) + + # Apply patches to the latest revision when `True`. + self.use_latest_revision = config.get("use_latest_revision", False) + + self._repo = None + + def __str__(self): + return self.name + + def clone(self): + raise NotImplementedError + + def has_revision(self, revision): + raise NotImplementedError + + def get_base_identifier(self, needed_stack: list[PhabricatorPatch]) -> str: + raise NotImplementedError + + def checkout_base(self, base): + """Update the local checkout to the base revision""" + raise NotImplementedError + + def apply_patch(self, patch: PhabricatorPatch, message, commit): + """Apply a single patch on the local checkout and commit it, + using the author from the Phabricator commit data""" + raise NotImplementedError + + def commit_try_task_config(self, path, message): + """Commit the try_task_config.json file as the bot""" + raise NotImplementedError + + def push_to_try(self): + raise NotImplementedError + + def revision_id(self, tip): + """Extract the revision identifier from a push_to_try result""" + raise NotImplementedError + + def clean(self): + raise NotImplementedError + + def apply_build(self, build): + """ + Apply a stack of patches to the local repository + and commit them one by one + """ + assert isinstance(build, PhabricatorBuild) + assert len(build.stack) > 0, "No patches to apply" + assert all(map(lambda p: isinstance(p, PhabricatorPatch), build.stack)) + + # Find the first unknown base revision + needed_stack = [] + for patch in reversed(build.stack): + # Skip already merged patches + if patch.merged: + logger.info( + f"Skip applying patch {patch.id} as it's already been merged upstream" + ) + continue + + # Add the patch into the stack only if not already merged ! + needed_stack.insert(0, patch) + + # Stop as soon as a base revision is available + if self.has_revision(patch.base_revision): + logger.info(f"Stopping at revision {patch.base_revision}") + break + + if not needed_stack: + logger.info("All the patches are already applied") + return + + base = self.get_base_identifier(needed_stack) + + # When base revision is missing, update to default revision + build.base_revision = base + build.missing_base_revision = not self.has_revision(base) + if build.missing_base_revision: + logger.warning( + "Missing base revision from Phabricator", + revision=base, + fallback=self.default_revision, + ) + base = self.default_revision + + # Store the actual base revision we used + build.actual_base_revision = base + + self.checkout_base(base) + + for patch in needed_stack: + if patch.commits: + # Use the first commit only + commit = patch.commits[0] + message = "{}\n".format(commit["message"]) + else: + # We should always have some commits here + logger.warning("Missing commit on patch", id=patch.id) + commit = None + message = "" + message += f"Differential Diff: {patch.phid}" + + logger.info("Applying patch", phid=patch.phid, message=message) + self.apply_patch(patch, message, commit) + + def add_try_commit(self, build): + """ + Build and commit the file configuring try + with try_task_config.json and the code-review workflow parameters in JSON + """ + path = os.path.join(self.dir, "try_task_config.json") + config = { + "version": 2, + "parameters": { + "target_tasks_method": "codereview", + "optimize_target_tasks": True, + "phabricator_diff": build.target_phid, + }, + } + diff_phid = build.stack[-1].phid + + if build.revision_url: + message = f"try_task_config for {build.revision_url}" + else: + message = "try_task_config for code-review" + message += f"\nDifferential Diff: {diff_phid}" + + # Write content as json and commit it + with open(path, "w") as f: + json.dump(config, f, sort_keys=True, indent=4) + self.commit_try_task_config(path, message) + + +class BaseWorker: + """ + Worker maintaining several local clones and applying builds on them, + independent of the version control system. + """ + + # Exception raised by the VCS layer on a failing command + VCS_ERROR = () + # Human readable VCS name, used in logs + VCS_NAME = None + # Worker output mode on a VCS failure + FAILURE_MODE = None + # Repository class handled by this worker + REPOSITORY_CLASS = BaseRepository + # Lowercase error messages elligible for a push retry + ELIGIBLE_RETRY_ERRORS = [] + + def __init__( + self, + skippable_files=[], + ): + self.skippable_files = skippable_files + + def run(self, repository, build): + """ + Apply the stack of patches from the build, handling retries + in case of try server errors + """ + while build.retries <= MAX_PUSH_RETRIES: + start = time.time() + + if build.retries: + logger.warning( + "Trying to apply build's diff after a remote push error " + f"[{build.retries}/{MAX_PUSH_RETRIES}]" + ) + + try: + return self.handle_build(repository, build) + except RetryNeeded: + build.retries += 1 + + if build.retries > MAX_PUSH_RETRIES: + error_log = "Max number of retries has been reached pushing the build to try repository" + logger.warn( + f"{self.VCS_NAME} error on diff", error=error_log, build=build + ) + return ( + self.FAILURE_MODE, + build, + {"message": error_log, "duration": time.time() - start}, + ) + + # Ensure the remote is available + self.wait_try_available() + + # Wait an exponential time before retrying the build + delay = PUSH_RETRY_EXPONENTIAL_DELAY**build.retries + logger.info( + f"An error occurred pushing the build to try, retrying after {delay}s" + ) + time.sleep(delay) + + def is_commit_skippable(self, build): + def get_files_touched_in_diff(rawdiff): + patched = [] + for parsed_diff in rs_parsepatch.get_diffs(rawdiff): + # filename is sometimes of format 'test.txt Tue Feb 05 17:23:40 2019 +0100' + # fix after https://github.com/mozilla/rust-parsepatch/issues/61 + if "filename" in parsed_diff: + filename = parsed_diff["filename"].split(" ")[0] + patched.append(filename) + return patched + + return any( + patched_file in self.skippable_files + for rev in build.stack + for patched_file in get_files_touched_in_diff(rev.patch) + ) + + def wait_try_available(self): + """ + Hook called before retrying a failed push, doing nothing by default + """ + + def is_eligible_for_retry(self, error): + """ + Given a VCS error message, if it's an error likely due to a + temporary connection problem, consider it as eligible for retry. + """ + error = error.lower() + return any( + eligible_message in error for eligible_message in self.ELIGIBLE_RETRY_ERRORS + ) + + def format_error(self, error): + """Extract a readable error log from a VCS exception""" + raise NotImplementedError + + def handle_build(self, repository, build): + """ + Try to load and apply a diff on local clone + If successful, push to try and send a treeherder link + In case of an unexpected push failure, retry up to MAX_PUSH_RETRIES + times by putting the build task back in the queue + + If the build fail, send a unit result with a warning message + """ + assert isinstance(repository, self.REPOSITORY_CLASS) + start = time.time() + + try: + # Start by cleaning the repo + repository.clean() + + # First apply patches on local repo + repository.apply_build(build) + + # Check Eligibility: some commits don't need to be pushed to try. + if self.is_commit_skippable(build): + logger.info("This patch series is ineligible for automated try push") + return ( + "fail:ineligible", + build, + { + "message": "Modified files match skippable internal configuration files", + "duration": time.time() - start, + }, + ) + + # Configure the try task + repository.add_try_commit(build) + + # Then push that stack on try + tip = repository.push_to_try() + logger.info("Diff has been pushed !") + + # Publish Treeherder link + uri = TREEHERDER_URL.format( + repository.try_name, repository.revision_id(tip) + ) + except self.VCS_ERROR as e: + error_log = self.format_error(e) + + if self.is_eligible_for_retry(error_log): + raise RetryNeeded + + logger.warn( + f"{self.VCS_NAME} error on diff", + error=error_log, + args=e.args, + build=build, + ) + return ( + self.FAILURE_MODE, + build, + {"message": error_log, "duration": time.time() - start}, + ) + + except Exception as e: + logger.warn("Failed to process diff", error=e, build=build) + return ( + "fail:general", + build, + {"message": str(e), "duration": time.time() - start}, + ) + + return ( + "success", + build, + {"treeherder_url": uri, "revision": repository.revision_id(tip)}, + ) diff --git a/bot/code_review_bot/workflow.py b/bot/code_review_bot/workflow.py index 2655f6fd0..c25a1d958 100644 --- a/bot/code_review_bot/workflow.py +++ b/bot/code_review_bot/workflow.py @@ -19,7 +19,11 @@ from code_review_bot.backend import BackendAPI from code_review_bot.config import settings from code_review_bot.git import git_clone -from code_review_bot.mercurial import MercurialWorker, Repository, robust_checkout +from code_review_bot.mercurial import ( + MercurialRepository, + MercurialWorker, + robust_checkout, +) from code_review_bot.report.debug import DebugReporter from code_review_bot.revisions import GithubRevision, PhabricatorRevision, Revision from code_review_bot.sources.phabricator import ( @@ -296,7 +300,7 @@ def start_analysis(self, revision): ) # Initialize mercurial repository - repository = Repository( + repository = MercurialRepository( config={ "name": revision.base_repository_conf.name, "try_name": revision.base_repository_conf.try_name, diff --git a/bot/tests/conftest.py b/bot/tests/conftest.py index 7be58736a..da6d97580 100644 --- a/bot/tests/conftest.py +++ b/bot/tests/conftest.py @@ -25,7 +25,7 @@ from code_review_bot import Level, stats from code_review_bot.backend import BackendAPI from code_review_bot.config import GetAppUserAgent, settings -from code_review_bot.mercurial import Repository +from code_review_bot.mercurial import MercurialRepository from code_review_bot.sources.phabricator import ( PhabricatorActions, PhabricatorBuild, @@ -1162,7 +1162,7 @@ def mock_mc(tmpdir): "try_url": "http://mozilla-central/try", "batch_size": 100, } - repo = Repository(config, tmpdir.realpath()) + repo = MercurialRepository(config, tmpdir.realpath()) repo._repo = build_repository(tmpdir, "mozilla-central") repo.clone = MagicMock(side_effect=lambda: True) return repo @@ -1181,7 +1181,7 @@ def mock_nss(tmpdir): "try_url": "http://nss/try", "batch_size": 100, } - repo = Repository(config, tmpdir.realpath()) + repo = MercurialRepository(config, tmpdir.realpath()) repo._repo = build_repository(tmpdir, "nss") repo.clone = MagicMock(side_effect=lambda: True) return repo diff --git a/bot/tests/test_mercurial.py b/bot/tests/test_mercurial.py index fd637824e..443da766d 100644 --- a/bot/tests/test_mercurial.py +++ b/bot/tests/test_mercurial.py @@ -681,11 +681,11 @@ def test_unexpected_push_failure(PhabricatorMock, mock_mc): with PhabricatorMock as phab: phab.load_patches_stack(build) - from code_review_bot import mercurial + from code_review_bot import mercurial, vcs - mercurial.MAX_PUSH_RETRIES = 1 + vcs.MAX_PUSH_RETRIES = 1 mercurial.TRY_STATUS_URL = "http://test.status/try" - mercurial.PUSH_RETRY_EXPONENTIAL_DELAY = 0 + vcs.PUSH_RETRY_EXPONENTIAL_DELAY = 0 mercurial.TRY_STATUS_DELAY = 0 mercurial.TRY_STATUS_MAX_WAIT = 0 @@ -693,7 +693,7 @@ def test_unexpected_push_failure(PhabricatorMock, mock_mc): "http://test.status/try", status=200, json={"result": {"status": "open"}} ) - repository_mock = MagicMock(spec=mercurial.Repository) + repository_mock = MagicMock(spec=mercurial.MercurialRepository) repository_mock.push_to_try.side_effect = [ hglib.error.CommandError( args=("push", "try_url"), @@ -703,6 +703,7 @@ def test_unexpected_push_failure(PhabricatorMock, mock_mc): ), mock_mc.repo.tip(), ] + repository_mock.revision_id.side_effect = lambda tip: tip.node.decode("utf-8") repository_mock.try_name = "try" repository_mock.retries = 0 @@ -743,11 +744,11 @@ def test_push_failure_max_retries(PhabricatorMock, mock_mc, monkeypatch): with PhabricatorMock as phab: phab.load_patches_stack(build) - from code_review_bot import mercurial + from code_review_bot import mercurial, vcs - mercurial.MAX_PUSH_RETRIES = 2 + vcs.MAX_PUSH_RETRIES = 2 mercurial.TRY_STATUS_URL = "http://test.status/try" - mercurial.PUSH_RETRY_EXPONENTIAL_DELAY = 2 + vcs.PUSH_RETRY_EXPONENTIAL_DELAY = 2 mercurial.TRY_STATUS_DELAY = 0 mercurial.TRY_STATUS_MAX_WAIT = 0 @@ -755,7 +756,7 @@ def test_push_failure_max_retries(PhabricatorMock, mock_mc, monkeypatch): "http://test.status/try", status=200, json={"result": {"status": "open"}} ) - repository_mock = MagicMock(spec=mercurial.Repository) + repository_mock = MagicMock(spec=mercurial.MercurialRepository) repository_mock.push_to_try.side_effect = hglib.error.CommandError( args=("push", "try_url"), ret=1, @@ -799,11 +800,11 @@ def test_push_closed_try(PhabricatorMock, mock_mc, monkeypatch): with PhabricatorMock as phab: phab.load_patches_stack(build) - from code_review_bot import mercurial + from code_review_bot import mercurial, vcs - mercurial.MAX_PUSH_RETRIES = 2 + vcs.MAX_PUSH_RETRIES = 2 mercurial.TRY_STATUS_URL = "http://test.status/try" - mercurial.PUSH_RETRY_EXPONENTIAL_DELAY = 2 + vcs.PUSH_RETRY_EXPONENTIAL_DELAY = 2 mercurial.TRY_STATUS_DELAY = 42 mercurial.TRY_STATUS_MAX_WAIT = 1 @@ -819,7 +820,7 @@ def test_push_closed_try(PhabricatorMock, mock_mc, monkeypatch): "http://test.status/try", status=200, json={"result": {"status": "open"}} ) - repository_mock = MagicMock(spec=mercurial.Repository) + repository_mock = MagicMock(spec=mercurial.MercurialRepository) repository_mock.push_to_try.side_effect = [ hglib.error.CommandError( args=("push", "try_url"), @@ -829,6 +830,7 @@ def test_push_closed_try(PhabricatorMock, mock_mc, monkeypatch): ), mock_mc.repo.tip(), ] + repository_mock.revision_id.side_effect = lambda tip: tip.node.decode("utf-8") repository_mock.try_name = "try" repository_mock.retries = 0 From 33883bb0b3895c503dfd678705be186382376167 Mon Sep 17 00:00:00 2001 From: nhadji Date: Wed, 29 Jul 2026 14:31:25 +0200 Subject: [PATCH 2/2] Bug 2048352 - Document the abstract methods of the VCS base classes --- bot/code_review_bot/vcs.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/bot/code_review_bot/vcs.py b/bot/code_review_bot/vcs.py index 01341f927..3dd5d9d8e 100644 --- a/bot/code_review_bot/vcs.py +++ b/bot/code_review_bot/vcs.py @@ -58,12 +58,17 @@ def __str__(self): return self.name def clone(self): + """Clone the repository in the local cache directory, or refresh an + existing clone, so that a stack of patches can be applied on it""" raise NotImplementedError def has_revision(self, revision): + """Check if a revision is directly available in the local repository""" raise NotImplementedError def get_base_identifier(self, needed_stack: list[PhabricatorPatch]) -> str: + """Return the revision identifier the stack of patches must be + applied against""" raise NotImplementedError def checkout_base(self, base): @@ -80,6 +85,8 @@ def commit_try_task_config(self, path, message): raise NotImplementedError def push_to_try(self): + """Push the locally applied stack to the remote try repository and + return the pushed tip, in the format accepted by revision_id()""" raise NotImplementedError def revision_id(self, tip): @@ -87,6 +94,8 @@ def revision_id(self, tip): raise NotImplementedError def clean(self): + """Restore the local repository to a pristine state, dropping the + commits applied by a previous build and pulling remote updates""" raise NotImplementedError def apply_build(self, build):