From 1644cb07e36e9acab7822ca5a8897183e88552d3 Mon Sep 17 00:00:00 2001 From: Mike Hommey Date: Mon, 24 Jun 2024 23:07:29 +0000 Subject: [PATCH] Bug 1500188 - Change the API to create a try commit. r=releng-reviewers,sheehan,ahal Currently, the caller potentially prepares files, add them to the VCS, and then calls the function to create the commit, does whatever it wants to do with it, then undoes it. With the ultimate goal of not doing local changes to the work tree to act on that try commit (for a push), this change in API makes it so that everything except what do do with the commit is handled by one function, that now takes paths and file contents for the files that were previously created by the caller. Differential Revision: https://phabricator.services.mozilla.com/D214214 --- .../mozversioncontrol/__init__.py | 125 ++++++++++++++---- python/mozversioncontrol/test/python.toml | 4 +- .../test/test_create_try_commit.py | 45 ------- .../test/test_push_to_try.py | 29 +++- .../test/test_remove_current_commit.py | 44 ------ .../mozversioncontrol/test/test_try_commit.py | 35 +++++ tools/tryselect/lando.py | 18 +-- tools/tryselect/push.py | 82 ++++++------ tools/tryselect/task_config.py | 20 --- 9 files changed, 206 insertions(+), 196 deletions(-) delete mode 100644 python/mozversioncontrol/test/test_create_try_commit.py delete mode 100644 python/mozversioncontrol/test/test_remove_current_commit.py create mode 100644 python/mozversioncontrol/test/test_try_commit.py diff --git a/python/mozversioncontrol/mozversioncontrol/__init__.py b/python/mozversioncontrol/mozversioncontrol/__init__.py index 083197ed17f4..9ba6fc17295f 100644 --- a/python/mozversioncontrol/mozversioncontrol/__init__.py +++ b/python/mozversioncontrol/mozversioncontrol/__init__.py @@ -8,9 +8,11 @@ import os import re import shutil import subprocess +from contextlib import contextmanager from datetime import datetime from pathlib import Path from typing import ( + Dict, Iterator, List, Optional, @@ -260,7 +262,12 @@ class Repository(object): """ @abc.abstractmethod - def push_to_try(self, message, allow_log_capture=False): + def push_to_try( + self, + message: str, + changed_files: Dict[str, str] = {}, + allow_log_capture: bool = False, + ): """Create a temporary commit, push it to try and clean it up afterwards. @@ -268,6 +275,9 @@ class Repository(object): extension is not installed. On git, MissingVCSExtension will be raised if git cinnabar is not present. + `changed_files` is a dict of file paths and their contents, see + `stage_changes`. + If `allow_log_capture` is set to `True`, then the push-to-try will be run using Popen instead of check_call so that the logs can be captured elsewhere. """ @@ -327,24 +337,43 @@ class Repository(object): ) @abc.abstractmethod - def get_branch_nodes(self) -> List[str]: + def get_branch_nodes(self, head: Optional[str] = None) -> List[str]: """Return a list of commit SHAs for nodes on the current branch.""" @abc.abstractmethod def get_commit_patches(self, nodes: str) -> List[bytes]: """Return the contents of the patch `node` in the VCS's standard format.""" + @contextmanager @abc.abstractmethod - def create_try_commit(self, commit_message: str): - """Create a temporary try commit. + def try_commit( + self, commit_message: str, changed_files: Optional[Dict[str, str]] = None + ): + """Create a temporary try commit as a context manager. Create a new commit using `commit_message` as the commit message. The commit may be empty, for example when only including try syntax. + + `changed_files` may contain a dict of file paths and their contents, + see `stage_changes`. """ - @abc.abstractmethod - def remove_current_commit(self): - """Remove the currently checked out commit from VCS history.""" + def stage_changes(self, changed_files: Dict[str, str]): + """Stage a set of file changes + + `changed_files` is a dict that contains the paths of files to change or + create as keys and their respective contents as values. + """ + paths = [] + for path, content in changed_files.items(): + full_path = Path(self.path) / path + full_path.parent.mkdir(parents=True, exist_ok=True) + with full_path.open("w") as fh: + fh.write(content) + paths.append(full_path) + + if paths: + self.add_remove_files(*paths) @abc.abstractmethod def get_last_modified_time_for_file(self, path: Path): @@ -589,7 +618,15 @@ class HgRepository(Repository): except subprocess.CalledProcessError: raise MissingVCSExtension(extension) - def push_to_try(self, message, allow_log_capture=False): + def push_to_try( + self, + message: str, + changed_files: Dict[str, str] = {}, + allow_log_capture: bool = False, + ): + if changed_files: + self.stage_changes(changed_files) + try: cmd = (str(self._tool), "push-to-try", "-m", message) if allow_log_capture: @@ -616,12 +653,14 @@ class HgRepository(Repository): finally: self._run("revert", "-a") - def get_branch_nodes(self, base_ref: Optional[str] = None) -> List[str]: + def get_branch_nodes( + self, head: Optional[str] = None, base_ref: Optional[str] = None + ) -> List[str]: """Return a list of commit SHAs for nodes on the current branch.""" if not base_ref: base_ref = self.base_ref - head_ref = self.head_ref + head_ref = head or self.head_ref return self._run( "log", @@ -661,17 +700,26 @@ class HgRepository(Repository): return patches - def create_try_commit(self, commit_message: str): - """Create a temporary try commit. + @contextmanager + def try_commit( + self, commit_message: str, changed_files: Optional[Dict[str, str]] = None + ): + """Create a temporary try commit as a context manager. Create a new commit using `commit_message` as the commit message. The commit may be empty, for example when only including try syntax. + + `changed_files` may contain a dict of file paths and their contents, + see `stage_changes`. """ + if changed_files: + self.stage_changes(changed_files) + # Allow empty commit messages in case we only use try-syntax. self._run("--config", "ui.allowemptycommit=1", "commit", "-m", commit_message) - def remove_current_commit(self): - """Remove the currently checked out commit from VCS history.""" + yield self.head_ref + try: self._run("prune", ".") except subprocess.CalledProcessError: @@ -869,12 +917,16 @@ class GitRepository(Repository): def update(self, ref): self._run("checkout", ref) - def push_to_try(self, message, allow_log_capture=False): + def push_to_try( + self, + message: str, + changed_files: Dict[str, str] = {}, + allow_log_capture: bool = False, + ): if not self.has_git_cinnabar: raise MissingVCSExtension("cinnabar") - self.create_try_commit(message) - try: + with self.try_commit(message, changed_files) as head: cmd = ( str(self._tool), "-c", @@ -884,7 +936,7 @@ class GitRepository(Repository): "cinnabar.data=never", "push", "hg::ssh://hg.mozilla.org/try", - "+HEAD:refs/heads/branches/default/tip", + f"+{head}:refs/heads/branches/default/tip", ) if allow_log_capture: self._push_to_try_with_log_capture( @@ -899,19 +951,17 @@ class GitRepository(Repository): ) else: subprocess.check_call(cmd, cwd=self.path) - finally: - self.remove_current_commit() def set_config(self, name, value): self._run("config", name, value) - def get_branch_nodes(self) -> List[str]: + def get_branch_nodes(self, head: Optional[str] = None) -> List[str]: """Return a list of commit SHAs for nodes on the current branch.""" remote_args = self.get_mozilla_remote_args() return self._run( "log", - "HEAD", + head or "HEAD", "--reverse", "--not", *remote_args, @@ -927,18 +977,32 @@ class GitRepository(Repository): for node in nodes ] - def create_try_commit(self, message: str): - """Create a temporary try commit. + @contextmanager + def try_commit( + self, commit_message: str, changed_files: Optional[Dict[str, str]] = None + ): + """Create a temporary try commit as a context manager. Create a new commit using `commit_message` as the commit message. The commit may be empty, for example when only including try syntax. + + `changed_files` may contain a dict of file paths and their contents, + see `stage_changes`. """ + if changed_files: + self.stage_changes(changed_files) + self._run( - "-c", "commit.gpgSign=false", "commit", "--allow-empty", "-m", message + "-c", + "commit.gpgSign=false", + "commit", + "--allow-empty", + "-m", + commit_message, ) - def remove_current_commit(self): - """Remove the currently checked out commit from VCS history.""" + yield "HEAD" + self._run("reset", "HEAD~") def get_last_modified_time_for_file(self, path: Path): @@ -1062,7 +1126,12 @@ class SrcRepository(Repository): def update(self, ref): pass - def push_to_try(self, message, allow_log_capture=False): + def push_to_try( + self, + message: str, + changed_files: Dict[str, str] = {}, + allow_log_capture: bool = False, + ): pass def set_config(self, name, value): diff --git a/python/mozversioncontrol/test/python.toml b/python/mozversioncontrol/test/python.toml index a194c2a22a61..3a1ca6145a78 100644 --- a/python/mozversioncontrol/test/python.toml +++ b/python/mozversioncontrol/test/python.toml @@ -7,8 +7,6 @@ subsuite = "mozversioncontrol" ["test_context_manager.py"] -["test_create_try_commit.py"] - ["test_get_branch_nodes.py"] ["test_get_commit_patches.py"] @@ -19,7 +17,7 @@ subsuite = "mozversioncontrol" ["test_push_to_try.py"] -["test_remove_current_commit.py"] +["test_try_commit.py"] ["test_update.py"] diff --git a/python/mozversioncontrol/test/test_create_try_commit.py b/python/mozversioncontrol/test/test_create_try_commit.py deleted file mode 100644 index 45e7facb0e1e..000000000000 --- a/python/mozversioncontrol/test/test_create_try_commit.py +++ /dev/null @@ -1,45 +0,0 @@ -# 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 mozunit - -from mozversioncontrol import get_repository_object - -STEPS = { - "hg": [ - """ - echo "{}" > try_task_config.json - hg add try_task_config.json - """, - ], - "git": [ - """ - echo "{}" > try_task_config.json - git add try_task_config.json - """, - ], -} - - -def test_create_try_commit(repo): - commit_message = "try commit message" - vcs = get_repository_object(repo.dir) - - # Create a non-empty commit. - repo.execute_next_step() - vcs.create_try_commit(commit_message) - non_empty_commit_sha = vcs.head_ref - - assert vcs.get_changed_files(rev=non_empty_commit_sha) == ["try_task_config.json"] - - # Create an empty commit. - vcs.create_try_commit(commit_message) - empty_commit_sha = vcs.head_ref - - # Commit should be created with no changed files. - assert vcs.get_changed_files(rev=empty_commit_sha) == [] - - -if __name__ == "__main__": - mozunit.main() diff --git a/python/mozversioncontrol/test/test_push_to_try.py b/python/mozversioncontrol/test/test_push_to_try.py index d0770c14386f..125d7159cf33 100644 --- a/python/mozversioncontrol/test/test_push_to_try.py +++ b/python/mozversioncontrol/test/test_push_to_try.py @@ -2,6 +2,7 @@ # 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 os import subprocess import mozunit @@ -17,22 +18,46 @@ def test_push_to_try(repo, monkeypatch): captured_commands = [] def fake_run(*args, **kwargs): - captured_commands.append(args[0]) + cmd = args[0] + captured_commands.append(cmd) + if os.path.basename(cmd[0]).startswith("hg") and cmd[1] == "--version": + return "version 6.7" monkeypatch.setattr(subprocess, "check_output", fake_run) monkeypatch.setattr(subprocess, "check_call", fake_run) - vcs.push_to_try(commit_message) + vcs.push_to_try( + commit_message, + { + "extra-file": "content", + "other/extra-file": "content2", + }, + ) tool = vcs._tool if repo.vcs == "hg": expected = [ + (str(tool), "--version"), + ( + str(tool), + "--config", + "extensions.automv=", + "addremove", + os.path.join(vcs.path, "extra-file"), + os.path.join(vcs.path, "other", "extra-file"), + ), (str(tool), "push-to-try", "-m", commit_message), (str(tool), "revert", "-a"), ] else: expected = [ (str(tool), "cinnabar", "--version"), + ( + str(tool), + "add", + os.path.join(vcs.path, "extra-file"), + os.path.join(vcs.path, "other", "extra-file"), + ), ( str(tool), "-c", diff --git a/python/mozversioncontrol/test/test_remove_current_commit.py b/python/mozversioncontrol/test/test_remove_current_commit.py deleted file mode 100644 index cec20c1ef4d4..000000000000 --- a/python/mozversioncontrol/test/test_remove_current_commit.py +++ /dev/null @@ -1,44 +0,0 @@ -# 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 mozunit -import pytest - -from mozversioncontrol import get_repository_object - -STEPS = { - "hg": [ - """ - echo "{}" > try_task_config.json - hg add try_task_config.json - hg commit -m "Try config commit" - """ - ], - "git": [ - """ - echo "{}" > try_task_config.json - git add try_task_config.json - git commit -m "Try config commit" - """ - ], -} - - -@pytest.mark.xfail(reason="Requires the Mercurial evolve extension.", strict=False) -def test_remove_current_commit(repo): - vcs = get_repository_object(repo.dir) - initial_head_ref = vcs.head_ref - - # Create a new commit. - repo.execute_next_step() - - vcs.remove_current_commit() - - assert ( - vcs.head_ref == initial_head_ref - ), "Removing current commit should revert to previous head." - - -if __name__ == "__main__": - mozunit.main() diff --git a/python/mozversioncontrol/test/test_try_commit.py b/python/mozversioncontrol/test/test_try_commit.py new file mode 100644 index 000000000000..1ac57b3319e7 --- /dev/null +++ b/python/mozversioncontrol/test/test_try_commit.py @@ -0,0 +1,35 @@ +# 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 mozunit +import pytest + +from mozversioncontrol import get_repository_object + + +@pytest.mark.xfail(reason="Requires the Mercurial evolve extension.", strict=False) +def test_try_commit(repo): + commit_message = "try commit message" + vcs = get_repository_object(repo.dir) + initial_head_ref = vcs.head_ref + + # Create a non-empty commit. + with vcs.try_commit(commit_message, {"try_task_config.json": "{}"}) as head: + assert vcs.get_changed_files(rev=head) == ["try_task_config.json"] + + assert ( + vcs.head_ref == initial_head_ref + ), "We should have reverted to previous head after try_commit" + + # Create an empty commit. + with vcs.try_commit(commit_message) as head: + assert vcs.get_changed_files(rev=head) == [] + + assert ( + vcs.head_ref == initial_head_ref + ), "We should have reverted to previous head after try_commit" + + +if __name__ == "__main__": + mozunit.main() diff --git a/tools/tryselect/lando.py b/tools/tryselect/lando.py index 7abd2ddfaee6..baca2f9fa6d4 100644 --- a/tools/tryselect/lando.py +++ b/tools/tryselect/lando.py @@ -35,10 +35,6 @@ from mozversioncontrol import ( HgRepository, ) -from .task_config import ( - try_config_commit, -) - TOKEN_FILE = ( Path(get_state_dir(specific_to_topsrcdir=False)) / "lando_auth0_user_token.json" ) @@ -74,7 +70,9 @@ def load_token_from_disk() -> Optional[dict]: return user_token -def get_stack_info(vcs: SupportedVcsRepository) -> Tuple[str, List[str]]: +def get_stack_info( + vcs: SupportedVcsRepository, head: Optional[str] +) -> Tuple[str, List[str]]: """Retrieve information about the current stack for submission via Lando. Returns a tuple of the current public base commit as a Mercurial SHA, @@ -92,7 +90,7 @@ def get_stack_info(vcs: SupportedVcsRepository) -> Tuple[str, List[str]]: if isinstance(vcs, HgRepository): branch_nodes_kwargs["base_ref"] = base_commit - nodes = vcs.get_branch_nodes(**branch_nodes_kwargs) + nodes = vcs.get_branch_nodes(head, **branch_nodes_kwargs) if not nodes: raise ValueError("Could not find any commit hashes for submission.") elif len(nodes) == 1: @@ -395,7 +393,9 @@ class LandoAPI: return response_json -def push_to_lando_try(vcs: SupportedVcsRepository, commit_message: str): +def push_to_lando_try( + vcs: SupportedVcsRepository, commit_message: str, changed_files: dict +): """Push a set of patches to Lando's try endpoint.""" # Map `Repository` subclasses to the `patch_format` value Lando expects. PATCH_FORMAT_STRING_MAPPING = { @@ -419,9 +419,9 @@ def push_to_lando_try(vcs: SupportedVcsRepository, commit_message: str): # Get the time when the push was initiated, not including Auth0 login time. push_start_time = time.perf_counter() - with try_config_commit(vcs, commit_message): + with vcs.try_commit(commit_message) as head: try: - base_commit, patches = get_stack_info(vcs) + base_commit, patches = get_stack_info(vcs, head) except ValueError as exc: error_msg = "abort: error gathering patches for submission." print(error_msg) diff --git a/tools/tryselect/push.py b/tools/tryselect/push.py index a4811682bc23..1793bf062b93 100644 --- a/tools/tryselect/push.py +++ b/tools/tryselect/push.py @@ -8,7 +8,6 @@ import os import sys import traceback -import six from mach.util import get_state_dir from mozbuild.base import MozbuildObject from mozversioncontrol import MissingVCSExtension, get_repository_object @@ -57,14 +56,6 @@ history_path = os.path.join( ) -def write_task_config(try_task_config): - config_path = os.path.join(vcs.path, "try_task_config.json") - with open(config_path, "w") as fh: - json.dump(try_task_config, fh, indent=4, separators=(",", ": "), sort_keys=True) - fh.write("\n") - return config_path - - def write_task_config_history(msg, try_task_config): if not os.path.isfile(history_path): if not os.path.isdir(os.path.dirname(history_path)): @@ -230,46 +221,47 @@ def push_to_try( method, ) - config_path = None - changed_files = [] + changed_files = {} + if try_task_config: + changed_files["try_task_config.json"] = ( + json.dumps( + try_task_config, indent=4, separators=(",", ": "), sort_keys=True + ) + + "\n" + ) if push and method not in ("again", "auto", "empty"): write_task_config_history(msg, try_task_config) - config_path = write_task_config(try_task_config) - changed_files.append(config_path) if (push or stage_changes) and files_to_change: - for path, content in files_to_change.items(): - path = os.path.join(vcs.path, path) - with open(path, "wb") as fh: - fh.write(six.ensure_binary(content)) - changed_files.append(path) + changed_files.update(files_to_change.items()) + + if not push: + print("Commit message:") + print(commit_message) + config = changed_files.pop("try_task_config.json", None) + if config: + print("Calculated try_task_config.json:") + print(config) + if stage_changes: + vcs.stage_changes(changed_files) + + return try: - if not push: - print("Commit message:") - print(commit_message) - if config_path: - print("Calculated try_task_config.json:") - with open(config_path) as fh: - print(fh.read()) - return - - vcs.add_remove_files(*changed_files) - - try: - if push_to_lando: - push_to_lando_try(vcs, commit_message) - else: - vcs.push_to_try(commit_message, allow_log_capture=allow_log_capture) - except MissingVCSExtension as e: - if e.ext == "push-to-try": - print(HG_PUSH_TO_TRY_NOT_FOUND) - elif e.ext == "cinnabar": - print(GIT_CINNABAR_NOT_FOUND) - else: - raise - sys.exit(1) - finally: - if config_path and os.path.isfile(config_path): - os.remove(config_path) + if push_to_lando: + push_to_lando_try(vcs, commit_message, changed_files) + else: + vcs.push_to_try( + commit_message, + changed_files=changed_files, + allow_log_capture=allow_log_capture, + ) + except MissingVCSExtension as e: + if e.ext == "push-to-try": + print(HG_PUSH_TO_TRY_NOT_FOUND) + elif e.ext == "cinnabar": + print(GIT_CINNABAR_NOT_FOUND) + else: + raise + sys.exit(1) diff --git a/tools/tryselect/task_config.py b/tools/tryselect/task_config.py index 7e941971ed05..9643a0d5a5aa 100644 --- a/tools/tryselect/task_config.py +++ b/tools/tryselect/task_config.py @@ -15,14 +15,12 @@ import subprocess import sys from abc import ABCMeta, abstractmethod, abstractproperty from argparse import SUPPRESS, Action -from contextlib import contextmanager from textwrap import dedent import mozpack.path as mozpath import requests import six from mozbuild.base import BuildEnvironmentNotFoundException, MozbuildObject -from mozversioncontrol import Repository from taskgraph.util import taskcluster from .tasks import resolve_tests_by_suite @@ -32,24 +30,6 @@ here = pathlib.Path(__file__).parent build = MozbuildObject.from_environment(cwd=str(here)) -@contextmanager -def try_config_commit(vcs: Repository, commit_message: str): - """Context manager that creates and removes a try config commit.""" - # Add the `try_task_config.json` file if it exists. - try_task_config_path = pathlib.Path(build.topsrcdir) / "try_task_config.json" - if try_task_config_path.exists(): - vcs.add_remove_files("try_task_config.json") - - try: - # Create a try config commit. - vcs.create_try_commit(commit_message) - - yield - finally: - # Revert the try config commit. - vcs.remove_current_commit() - - class ParameterConfig: __metaclass__ = ABCMeta