From 56629f6781ba55fa3a7c0ad09f5ae9eacb312c29 Mon Sep 17 00:00:00 2001 From: Yannik Tausch Date: Sat, 7 Sep 2024 12:38:24 +0200 Subject: [PATCH 1/6] refactor rerender in auto_tick --- conda_forge_tick/auto_tick.py | 162 +++++++++++++++------------- conda_forge_tick/contexts.py | 29 +++++ conda_forge_tick/git_utils.py | 21 +++- conda_forge_tick/migrators_types.py | 1 + tests/test_contexts.py | 19 ++++ tests/test_git_utils.py | 56 ++++++++++ 6 files changed, 212 insertions(+), 76 deletions(-) diff --git a/conda_forge_tick/auto_tick.py b/conda_forge_tick/auto_tick.py index 151a816f2..a7ec13f6c 100644 --- a/conda_forge_tick/auto_tick.py +++ b/conda_forge_tick/auto_tick.py @@ -4,9 +4,11 @@ import logging import os import random +import textwrap import time import traceback import typing +from dataclasses import dataclass from textwrap import dedent from typing import Literal, MutableMapping, cast from urllib.error import URLError @@ -54,7 +56,7 @@ from conda_forge_tick.migration_runner import run_migration from conda_forge_tick.migrators import MigrationYaml, Migrator, Version from conda_forge_tick.migrators.version import VersionMigrationError -from conda_forge_tick.os_utils import eval_cmd, pushd +from conda_forge_tick.os_utils import eval_cmd from conda_forge_tick.rerender_feedstock import rerender_feedstock from conda_forge_tick.solver_checks import is_recipe_solvable from conda_forge_tick.utils import ( @@ -223,6 +225,69 @@ def _commit_migration( raise +@dataclass(frozen=True) +class _RerenderInfo: + """ + Additional information about a rerender operation. + """ + + nontrivial_changes: bool + """ + True if any files which are not in the following list were changed during the rerender, False otherwise: + 1. anything in the recipe directory + 2. the README file + """ + rerender_comment: str | None = None + """ + If requested, a comment to be added to the PR to indicate an issue with the rerender. + None if no comment should be added. + """ + + +def _run_rerender( + git_cli: GitCli, context: ClonedFeedstockContext, suppress_errors: bool = False +) -> _RerenderInfo: + logger.info("Rerendering the feedstock") + + try: + rerender_msg = rerender_feedstock(str(context.local_clone_dir), timeout=900) + except Exception as e: + logger.error("RERENDER ERROR", exc_info=e) + + if not suppress_errors: + raise + + rerender_comment = textwrap.dedent( + """ + Hi! This feedstock was not able to be rerendered after the version update changes. I + have pushed the version update changes anyways and am trying to rerender again with this + comment. Hopefully you all can fix this! + @conda-forge-admin rerender + """ + ) + + return _RerenderInfo( + nontrivial_changes=False, rerender_comment=rerender_comment + ) + + if rerender_msg is None: + return _RerenderInfo(nontrivial_changes=False) + + git_cli.commit(context.local_clone_dir, rerender_msg, all_=True, allow_empty=True) + + # HEAD~ is the state before the last commit + changed_files = git_cli.diffed_files(context.local_clone_dir, "HEAD~") + + recipe_dir = context.local_clone_dir / "recipe" + + nontrivial_changes = any( + not file.is_relative_to(recipe_dir) and not file.name.startswith("README") + for file in changed_files + ) + + return _RerenderInfo(nontrivial_changes) + + def run_with_tmpdir( context: FeedstockContext, migrator: Migrator, @@ -336,6 +401,18 @@ def run( raise_commit_errors=migrator.allow_empty_commits and not rerender, ) + if rerender: + # for version migrations, check solvable or automerge, we always raise rerender errors + suppress_errors = ( + not is_version_migration + and not context.check_solvable + and not context.automerge + ) + + rerender_info = _run_rerender(git_backend.cli, context, suppress_errors) + else: + rerender_info = _RerenderInfo(nontrivial_changes=False) + # This is needed because we want to migrate to the new backend step-by-step repo: github3.repos.Repository | None = github3_client().repository( context.git_repo_owner, context.git_repo_name @@ -345,73 +422,13 @@ def run( feedstock_dir = str(context.local_clone_dir.resolve()) - # rerender, maybe - diffed_files: typing.List[str] = [] - with pushd(feedstock_dir): - if rerender: - head_ref = eval_cmd(["git", "rev-parse", "HEAD"]).strip() - logger.info("Rerendering the feedstock") - - try: - rerender_msg = rerender_feedstock(feedstock_dir, timeout=900) - if rerender_msg is not None: - eval_cmd(["git", "commit", "--allow-empty", "-am", rerender_msg]) - - make_rerender_comment = False - except Exception as e: - # I am trying this bit of code to force these errors - # to be surfaced in the logs at the right time. - print(f"RERENDER ERROR: {e}", flush=True) - if not isinstance(migrator, Version): - raise - else: - # for check solvable or automerge, we always raise rerender errors - if get_keys_default( - context.attrs, - ["conda-forge.yml", "bot", "check_solvable"], - {}, - False, - ) or get_keys_default( - context.attrs, - ["conda-forge.yml", "bot", "automerge"], - {}, - False, - ): - raise - else: - make_rerender_comment = True - - # If we tried to run the MigrationYaml and rerender did nothing (we only - # bumped the build number and dropped a yaml file in migrations) bail - # for instance platform specific migrations - gdiff = eval_cmd( - ["git", "diff", "--name-only", f"{head_ref.strip()}...HEAD"] - ) - - diffed_files = [ - _ - for _ in gdiff.split() - if not ( - _.startswith("recipe") - or _.startswith("migrators") - or _.startswith("README") - ) - ] - else: - make_rerender_comment = False - - feedstock_automerge = get_keys_default( - context.attrs, - ["conda-forge.yml", "bot", "automerge"], - {}, - False, - ) if isinstance(migrator, Version): - has_automerge = feedstock_automerge in [True, "version"] + has_automerge = context.automerge in [True, "version"] else: - has_automerge = getattr( - migrator, "automerge", False - ) and feedstock_automerge in [True, "migration"] + has_automerge = getattr(migrator, "automerge", False) and context.automerge in [ + True, + "migration", + ] migrator_check_solvable = getattr(migrator, "check_solvable", True) feedstock_check_solvable = get_keys_default( @@ -432,7 +449,7 @@ def run( logger.info( f"""automerge and check_solvable status/settings: automerge: - feedstock_automerge: {feedstock_automerge} + feedstock_automerge: {context.automerge} migratror_automerge: {getattr(migrator, 'automerge', False)} has_automerge: {has_automerge} (only considers feedstock if version migration) check_solvable: @@ -505,7 +522,7 @@ def run( pr_json: typing.Union[MutableMapping, None, bool] if ( isinstance(migrator, MigrationYaml) - and not diffed_files + and not rerender_info.nontrivial_changes and context.attrs["name"] != "conda-forge-pinning" ): # spoof this so it looks like the package is done @@ -538,15 +555,10 @@ def run( pr_json = False ljpr = False - if pr_json and pr_json["state"] != "closed" and make_rerender_comment: + if pr_json and pr_json["state"] != "closed" and rerender_info.rerender_comment: comment_on_pr( pr_json, - """\ -Hi! This feedstock was not able to be rerendered after the version update changes. I -have pushed the version update changes anyways and am trying to rerender again with this -comment. Hopefully you all can fix this! - -@conda-forge-admin rerender""", + rerender_info.rerender_comment, repo, ) diff --git a/conda_forge_tick/contexts.py b/conda_forge_tick/contexts.py index 9175ab076..a0ba8c873 100644 --- a/conda_forge_tick/contexts.py +++ b/conda_forge_tick/contexts.py @@ -11,6 +11,7 @@ from conda_forge_tick.lazy_json_backends import load from conda_forge_tick.migrators_types import AttrsTypedDict +from conda_forge_tick.utils import get_keys_default if os.path.exists("all_feedstocks.json"): with open("all_feedstocks.json") as f: @@ -54,6 +55,34 @@ def git_repo_owner(self) -> str: def git_repo_name(self) -> str: return f"{self.feedstock_name}-feedstock" + @property + def automerge(self) -> bool | str: + """ + Get the automerge setting of the feedstock. + Note: A better solution to implement this is to use the NodeAttributes Pydantic + model for the attrs field. This can be done in the future. + """ + return get_keys_default( + self.attrs, + ["conda-forge.yml", "bot", "automerge"], + {}, + False, + ) + + @property + def check_solvable(self) -> bool: + """ + Get the check_solvable setting of the feedstock. + Note: A better solution to implement this is to use the NodeAttributes Pydantic + model for the attrs field. This can be done in the future. + """ + return get_keys_default( + self.attrs, + ["conda-forge.yml", "bot", "check_solvable"], + {}, + False, + ) + @contextmanager def reserve_clone_directory(self) -> Iterator[ClonedFeedstockContext]: """ diff --git a/conda_forge_tick/git_utils.py b/conda_forge_tick/git_utils.py index 01c1f9ec5..f94cb168d 100644 --- a/conda_forge_tick/git_utils.py +++ b/conda_forge_tick/git_utils.py @@ -11,7 +11,7 @@ from datetime import datetime from functools import cached_property from pathlib import Path -from typing import Dict, Optional, Union +from typing import Dict, Iterator, Optional, Union import backoff import github @@ -327,6 +327,25 @@ def checkout_new_branch( ["checkout", "--quiet", "-b", branch] + start_point_option, git_dir ) + def diffed_files( + self, git_dir: Path, commit_a: str, commit_b: str = "HEAD" + ) -> Iterator[Path]: + """ + Get the files that are different between two commits. + :param git_dir: The directory of the git repository. This should be the root of the repository. + If it is a subdirectory, only the files in that subdirectory will be returned. + :param commit_a: The first commit. + :param commit_b: The second commit. + :return: An iterator over the files that are different between the two commits. + """ + + # --relative ensures that we do not assemble invalid paths below if git_dir is a subdirectory + ret = self._run_git_command( + ["diff", "--name-only", "--relative", commit_a, commit_b], git_dir + ) + + return (git_dir / line for line in ret.stdout.splitlines()) + @lock_git_operation() def clone_fork_and_branch( self, diff --git a/conda_forge_tick/migrators_types.py b/conda_forge_tick/migrators_types.py index 8f83d6ee3..a2574c0eb 100644 --- a/conda_forge_tick/migrators_types.py +++ b/conda_forge_tick/migrators_types.py @@ -136,6 +136,7 @@ class AttrsTypedDict_(TypedDict, total=False): new_version: Union[str, bool] archived: bool PRed: List[PRedElementTypedDict] + version_pr_info: typing.Any # Legacy types in here bad: Union[bool, str] # TODO: ADD in diff --git a/tests/test_contexts.py b/tests/test_contexts.py index 82956ea57..e0ea82c82 100644 --- a/tests/test_contexts.py +++ b/tests/test_contexts.py @@ -56,6 +56,25 @@ def test_feedstock_context_git_repo_name(): assert context.git_repo_name == "TEST-FEEDSTOCK-NAME-feedstock" +@pytest.mark.parametrize("automerge", [True, False]) +def test_feedstock_context_automerge(automerge: bool): + context = FeedstockContext( + "TEST-FEEDSTOCK-NAME", demo_attrs_automerge if automerge else demo_attrs + ) + + assert context.automerge == automerge + + +@pytest.mark.parametrize("check_solvable", [True, False]) +def test_feedstock_context_check_solvable(check_solvable: bool): + context = FeedstockContext( + "TEST-FEEDSTOCK-NAME", + demo_attrs_check_solvable if check_solvable else demo_attrs, + ) + + assert context.check_solvable == check_solvable + + @pytest.mark.parametrize("default_branch", [None, "", "feature"]) @pytest.mark.parametrize( "attrs", [demo_attrs, demo_attrs_automerge, demo_attrs_check_solvable] diff --git a/tests/test_git_utils.py b/tests/test_git_utils.py index 767ebde22..983d54a82 100644 --- a/tests/test_git_utils.py +++ b/tests/test_git_utils.py @@ -418,6 +418,62 @@ def test_git_cli_fetch_all(): cli.fetch_all(dir_path) +def test_git_cli_diffed_files(): + cli = GitCli() + + with tempfile.TemporaryDirectory() as tmpdir: + dir_path = Path(tmpdir) + + init_temp_git_repo(dir_path) + + cli.commit(dir_path, "Initial commit", allow_empty=True) + dir_path.joinpath("test.txt").touch() + cli.add(dir_path, dir_path / "test.txt") + cli.commit(dir_path, "Add test.txt") + + diffed_files = list(cli.diffed_files(dir_path, "HEAD~1")) + + assert (dir_path / "test.txt") in diffed_files + assert len(diffed_files) == 1 + + +def test_git_cli_diffed_files_no_diff(): + cli = GitCli() + + with tempfile.TemporaryDirectory() as tmpdir: + dir_path = Path(tmpdir) + + init_temp_git_repo(dir_path) + + cli.commit(dir_path, "Initial commit", allow_empty=True) + + diffed_files = list(cli.diffed_files(dir_path, "HEAD")) + + assert len(diffed_files) == 0 + + +@mock.patch("conda_forge_tick.git_utils.GitCli._run_git_command") +def test_git_cli_diffed_files_mock(run_git_command_mock: MagicMock): + cli = GitCli() + + git_dir = Path("TEST_DIR") + commit = "COMMIT" + + run_git_command_mock.return_value = subprocess.CompletedProcess( + args=[], returncode=0, stdout="test.txt\n" + ) + + diffed_files = list(cli.diffed_files(git_dir, commit)) + + run_git_command_mock.assert_called_once_with( + ["diff", "--name-only", "--relative", commit, "HEAD"], + git_dir, + capture_text=True, + ) + + assert diffed_files == [git_dir / "test.txt"] + + def test_git_cli_does_branch_exist(): cli = GitCli() From 67e81d188c7658cb02c59b6f0b0a716ccff2d9ba Mon Sep 17 00:00:00 2001 From: Yannik Tausch Date: Sat, 7 Sep 2024 13:09:38 +0200 Subject: [PATCH 2/6] fix: diffed_files must capture text --- conda_forge_tick/git_utils.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/conda_forge_tick/git_utils.py b/conda_forge_tick/git_utils.py index f94cb168d..ec4d71809 100644 --- a/conda_forge_tick/git_utils.py +++ b/conda_forge_tick/git_utils.py @@ -341,7 +341,9 @@ def diffed_files( # --relative ensures that we do not assemble invalid paths below if git_dir is a subdirectory ret = self._run_git_command( - ["diff", "--name-only", "--relative", commit_a, commit_b], git_dir + ["diff", "--name-only", "--relative", commit_a, commit_b], + git_dir, + capture_text=True, ) return (git_dir / line for line in ret.stdout.splitlines()) From 72841f3687ca1ebc2ae4bb333febc5cfee43c403 Mon Sep 17 00:00:00 2001 From: Yannik Tausch Date: Mon, 9 Sep 2024 11:18:52 +0200 Subject: [PATCH 3/6] fix suppress_errors Co-authored-by: Matthew R. Becker --- conda_forge_tick/auto_tick.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/conda_forge_tick/auto_tick.py b/conda_forge_tick/auto_tick.py index a7ec13f6c..14eb9e4b9 100644 --- a/conda_forge_tick/auto_tick.py +++ b/conda_forge_tick/auto_tick.py @@ -402,9 +402,9 @@ def run( ) if rerender: - # for version migrations, check solvable or automerge, we always raise rerender errors + # for version migrations, without check solvable or automerge, we can suppress rerender errors suppress_errors = ( - not is_version_migration + is_version_migration and not context.check_solvable and not context.automerge ) From 72cd31f1fbce89602f5c982f9e9332a933401e11 Mon Sep 17 00:00:00 2001 From: Yannik Tausch Date: Mon, 16 Sep 2024 09:24:57 +0200 Subject: [PATCH 4/6] re-add newline in PR body Co-authored-by: Matthew R. Becker --- conda_forge_tick/auto_tick.py | 1 + 1 file changed, 1 insertion(+) diff --git a/conda_forge_tick/auto_tick.py b/conda_forge_tick/auto_tick.py index 14eb9e4b9..5e84c9307 100644 --- a/conda_forge_tick/auto_tick.py +++ b/conda_forge_tick/auto_tick.py @@ -262,6 +262,7 @@ def _run_rerender( Hi! This feedstock was not able to be rerendered after the version update changes. I have pushed the version update changes anyways and am trying to rerender again with this comment. Hopefully you all can fix this! + @conda-forge-admin rerender """ ) From 08dc6bbd8a83b19b35f0eb2c1d8167f5a36356e5 Mon Sep 17 00:00:00 2001 From: Yannik Tausch Date: Mon, 16 Sep 2024 09:26:06 +0200 Subject: [PATCH 5/6] use nontrivial_changes keyword everywhere --- conda_forge_tick/auto_tick.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/conda_forge_tick/auto_tick.py b/conda_forge_tick/auto_tick.py index 5e84c9307..4ae36eefd 100644 --- a/conda_forge_tick/auto_tick.py +++ b/conda_forge_tick/auto_tick.py @@ -262,7 +262,7 @@ def _run_rerender( Hi! This feedstock was not able to be rerendered after the version update changes. I have pushed the version update changes anyways and am trying to rerender again with this comment. Hopefully you all can fix this! - + @conda-forge-admin rerender """ ) @@ -286,7 +286,7 @@ def _run_rerender( for file in changed_files ) - return _RerenderInfo(nontrivial_changes) + return _RerenderInfo(nontrivial_changes=nontrivial_changes) def run_with_tmpdir( From ae8944466e2f2c7963d3a21af958aeed46b1834e Mon Sep 17 00:00:00 2001 From: Yannik Tausch Date: Mon, 16 Sep 2024 09:27:32 +0200 Subject: [PATCH 6/6] pass supress_errors as keyword argument --- conda_forge_tick/auto_tick.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/conda_forge_tick/auto_tick.py b/conda_forge_tick/auto_tick.py index 4ae36eefd..df8862d0c 100644 --- a/conda_forge_tick/auto_tick.py +++ b/conda_forge_tick/auto_tick.py @@ -410,7 +410,9 @@ def run( and not context.automerge ) - rerender_info = _run_rerender(git_backend.cli, context, suppress_errors) + rerender_info = _run_rerender( + git_backend.cli, context, suppress_errors=suppress_errors + ) else: rerender_info = _RerenderInfo(nontrivial_changes=False)