llvm_tools: update_chromeos_llvm_hash failure modes

Support 'disable_patches' and 'remove_patches'
failure mode options.

BUG=b:250648178
TEST=./patch_utils_unittest.py
     ./update_chromeos_llvm_hash_unittest.py
     ./patch_manager_unittest.py
     ./update_chromeos_llvm_hash [...] --failure_mode remove_patches
     ./update_chromeos_llvm_hash [...] --failure_mode disable_patches

Change-Id: I6269b2220cf05413c7776087030297773ab9a154
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/3935651
Reviewed-by: Adrian Dole <adriandole@google.com>
Auto-Submit: Adrian Dole <adriandole@google.com>
Tested-by: Adrian Dole <adriandole@google.com>
Reviewed-by: Denis Nikitin <denik@chromium.org>
Reviewed-by: Jordan Abrahams-Whitehead <ajordanr@google.com>
Commit-Queue: Adrian Dole <adriandole@google.com>

5 files changed, 258 insertions, 212 deletions

diff --git a/llvm_tools/patch_manager.py b/llvm_tools/patch_manager.py
index 11e82227..4d4e8385 100755
--- a/llvm_tools/patch_manager.py
+++ b/llvm_tools/patch_manager.py
@@ -7,11 +7,10 @@
 
 import argparse
 import enum
-import json
 import os
 from pathlib import Path
 import sys
-from typing import Any, Dict, IO, Iterable, List, Optional, Tuple
+from typing import Iterable, List, Optional, Tuple
 
 from failure_modes import FailureModes
 import get_llvm_hash
@@ -97,13 +96,6 @@ def GetHEADSVNVersion(src_path):
     return version
 
 
-def _WriteJsonChanges(patches: List[Dict[str, Any]], file_io: IO[str]):
-    """Write JSON changes to file, does not acquire new file lock."""
-    json.dump(patches, file_io, indent=4, separators=(",", ": "))
-    # Need to add a newline as json.dump omits it.
-    file_io.write("\n")
-
-
 def GetCommitHashesForBisection(src_path, good_svn_version, bad_svn_version):
     """Gets the good and bad commit hashes required by `git bisect start`."""
 
@@ -114,105 +106,6 @@ def GetCommitHashesForBisection(src_path, good_svn_version, bad_svn_version):
     return good_commit_hash, bad_commit_hash
 
 
-def RemoveOldPatches(
-    svn_version: int, llvm_src_dir: Path, patches_json_fp: Path
-):
-    """Remove patches that don't and will never apply for the future.
-
-    Patches are determined to be "old" via the "is_old" method for
-    each patch entry.
-
-    Args:
-      svn_version: LLVM SVN version.
-      llvm_src_dir: LLVM source directory.
-      patches_json_fp: Location to edit patches on.
-    """
-    with patches_json_fp.open(encoding="utf-8") as f:
-        patches_list = json.load(f)
-    patch_entries = (
-        patch_utils.PatchEntry.from_dict(llvm_src_dir, elem)
-        for elem in patches_list
-    )
-    oldness = [(entry, entry.is_old(svn_version)) for entry in patch_entries]
-    filtered_entries = [entry.to_dict() for entry, old in oldness if not old]
-    with patch_utils.atomic_write(patches_json_fp, encoding="utf-8") as f:
-        _WriteJsonChanges(filtered_entries, f)
-    removed_entries = [entry for entry, old in oldness if old]
-    plural_patches = "patch" if len(removed_entries) == 1 else "patches"
-    print(f"Removed {len(removed_entries)} old {plural_patches}:")
-    for r in removed_entries:
-        print(f"- {r.rel_patch_path}: {r.title()}")
-
-
-def UpdateVersionRanges(
-    svn_version: int, llvm_src_dir: Path, patches_json_fp: Path
-):
-    """Reduce the version ranges of failing patches.
-
-    Patches which fail to apply will have their 'version_range.until'
-    field reduced to the passed in svn_version.
-
-    Modifies the contents of patches_json_fp.
-
-    Ars:
-      svn_version: LLVM revision number.
-      llvm_src_dir: llvm-project directory path.
-      patches_json_fp: Filepath to the PATCHES.json file.
-    """
-    with patches_json_fp.open(encoding="utf-8") as f:
-        patch_entries = patch_utils.json_to_patch_entries(
-            patches_json_fp.parent,
-            f,
-        )
-    modified_entries = UpdateVersionRangesWithEntries(
-        svn_version, llvm_src_dir, patch_entries
-    )
-    with patch_utils.atomic_write(patches_json_fp, encoding="utf-8") as f:
-        _WriteJsonChanges([p.to_dict() for p in patch_entries], f)
-    for entry in modified_entries:
-        print(
-            f"Stopped applying {entry.rel_patch_path} ({entry.title()}) "
-            f"for r{svn_version}"
-        )
-
-
-def UpdateVersionRangesWithEntries(
-    svn_version: int,
-    llvm_src_dir: Path,
-    patch_entries: Iterable[patch_utils.PatchEntry],
-) -> List[patch_utils.PatchEntry]:
-    """Test-able helper for UpdateVersionRanges.
-
-    Args:
-      svn_version: LLVM revision number.
-      llvm_src_dir: llvm-project directory path.
-      patch_entries: PatchEntry objects to modify.
-
-    Returns:
-      A list of PatchEntry objects which were modified.
-
-    Post:
-      Modifies patch_entries in place.
-    """
-    modified_entries: List[patch_utils.PatchEntry] = []
-    with patch_utils.git_clean_context(llvm_src_dir):
-        for pe in patch_entries:
-            test_result = pe.test_apply(llvm_src_dir)
-            if not test_result:
-                if pe.version_range is None:
-                    pe.version_range = {}
-                pe.version_range["until"] = svn_version
-                modified_entries.append(pe)
-            else:
-                # We have to actually apply the patch so that future patches
-                # will stack properly.
-                if not pe.apply(llvm_src_dir).succeeded:
-                    raise RuntimeError(
-                        "Could not apply patch that dry ran successfully"
-                    )
-    return modified_entries
-
-
 def CheckPatchApplies(
     svn_version: int,
     llvm_src_dir: Path,
@@ -374,10 +267,14 @@ def main(sys_argv: List[str]):
         PrintPatchResults(result)
 
     def _remove(args):
-        RemoveOldPatches(args.svn_version, llvm_src_dir, patches_json_fp)
+        patch_utils.remove_old_patches(
+            args.svn_version, llvm_src_dir, patches_json_fp
+        )
 
     def _disable(args):
-        UpdateVersionRanges(args.svn_version, llvm_src_dir, patches_json_fp)
+        patch_utils.update_version_ranges(
+            args.svn_version, llvm_src_dir, patches_json_fp
+        )
 
     def _test_single(args):
         if not args.test_patch:
diff --git a/llvm_tools/patch_manager_unittest.py b/llvm_tools/patch_manager_unittest.py
index 19c2d8af..42697d91 100755
--- a/llvm_tools/patch_manager_unittest.py
+++ b/llvm_tools/patch_manager_unittest.py
@@ -62,50 +62,6 @@ class PatchManagerTest(unittest.TestCase):
         mock_isfile.assert_called_once()
 
     @mock.patch("builtins.print")
-    def testRemoveOldPatches(self, _):
-        """Can remove old patches from PATCHES.json."""
-        one_patch_dict = {
-            "metadata": {
-                "title": "[some label] hello world",
-            },
-            "platforms": [
-                "chromiumos",
-            ],
-            "rel_patch_path": "x/y/z",
-            "version_range": {
-                "from": 4,
-                "until": 5,
-            },
-        }
-        patches = [
-            one_patch_dict,
-            {**one_patch_dict, "version_range": {"until": None}},
-            {**one_patch_dict, "version_range": {"from": 100}},
-            {**one_patch_dict, "version_range": {"until": 8}},
-        ]
-        cases = [
-            (0, lambda x: self.assertEqual(len(x), 4)),
-            (6, lambda x: self.assertEqual(len(x), 3)),
-            (8, lambda x: self.assertEqual(len(x), 2)),
-            (1000, lambda x: self.assertEqual(len(x), 2)),
-        ]
-
-        def _t(dirname: str, svn_version: int, assertion_f: Callable):
-            json_filepath = Path(dirname) / "PATCHES.json"
-            with json_filepath.open("w", encoding="utf-8") as f:
-                json.dump(patches, f)
-            patch_manager.RemoveOldPatches(svn_version, Path(), json_filepath)
-            with json_filepath.open("r", encoding="utf-8") as f:
-                result = json.load(f)
-            assertion_f(result)
-
-        with tempfile.TemporaryDirectory(
-            prefix="patch_manager_unittest"
-        ) as dirname:
-            for r, a in cases:
-                _t(dirname, r, a)
-
-    @mock.patch("builtins.print")
     @mock.patch.object(patch_utils, "git_clean_context")
     def testCheckPatchApplies(self, _, mock_git_clean_context):
         """Tests whether we can apply a single patch for a given svn_version."""
@@ -253,53 +209,6 @@ class PatchManagerTest(unittest.TestCase):
                 patch_manager.GitBisectionCode.SKIP,
             )
 
-    @mock.patch("patch_utils.git_clean_context", mock.MagicMock)
-    def testUpdateVersionRanges(self):
-        """Test the UpdateVersionRanges function."""
-        with tempfile.TemporaryDirectory(
-            prefix="patch_manager_unittest"
-        ) as dirname:
-            dirpath = Path(dirname)
-            patches = [
-                patch_utils.PatchEntry(
-                    workdir=dirpath,
-                    rel_patch_path="x.patch",
-                    metadata=None,
-                    platforms=None,
-                    version_range={
-                        "from": 0,
-                        "until": 2,
-                    },
-                ),
-                patch_utils.PatchEntry(
-                    workdir=dirpath,
-                    rel_patch_path="y.patch",
-                    metadata=None,
-                    platforms=None,
-                    version_range={
-                        "from": 0,
-                        "until": 2,
-                    },
-                ),
-            ]
-            patches[0].apply = mock.MagicMock(
-                return_value=patch_utils.PatchResult(
-                    succeeded=False, failed_hunks={"a/b/c": []}
-                )
-            )
-            patches[1].apply = mock.MagicMock(
-                return_value=patch_utils.PatchResult(succeeded=True)
-            )
-            results = patch_manager.UpdateVersionRangesWithEntries(
-                1, dirpath, patches
-            )
-            # We should only have updated the version_range of the first patch,
-            # as that one failed to apply.
-            self.assertEqual(len(results), 1)
-            self.assertEqual(results[0].version_range, {"from": 0, "until": 1})
-            self.assertEqual(patches[0].version_range, {"from": 0, "until": 1})
-            self.assertEqual(patches[1].version_range, {"from": 0, "until": 2})
-
 
 if __name__ == "__main__":
     unittest.main()
diff --git a/llvm_tools/patch_utils.py b/llvm_tools/patch_utils.py
index ca912f2b..b86e1925 100644
--- a/llvm_tools/patch_utils.py
+++ b/llvm_tools/patch_utils.py
@@ -12,7 +12,7 @@ from pathlib import Path
 import re
 import subprocess
 import sys
-from typing import Any, Dict, IO, List, Optional, Tuple, Union
+from typing import Any, Dict, IO, Iterable, List, Optional, Tuple, Union
 
 
 CHECKED_FILE_RE = re.compile(r"^checking file\s+(.*)$")
@@ -456,3 +456,137 @@ def git_clean_context(git_root_dir: Path):
         yield
     finally:
         clean_src_tree(git_root_dir)
+
+
+def _write_json_changes(patches: List[Dict[str, Any]], file_io: IO[str]):
+    """Write JSON changes to file, does not acquire new file lock."""
+    json.dump(patches, file_io, indent=4, separators=(",", ": "))
+    # Need to add a newline as json.dump omits it.
+    file_io.write("\n")
+
+
+def update_version_ranges(
+    svn_version: int, llvm_src_dir: Path, patches_json_fp: Path
+) -> PatchInfo:
+    """Reduce the version ranges of failing patches.
+
+    Patches which fail to apply will have their 'version_range.until'
+    field reduced to the passed in svn_version.
+
+    Modifies the contents of patches_json_fp.
+
+    Args:
+      svn_version: LLVM revision number.
+      llvm_src_dir: llvm-project directory path.
+      patches_json_fp: Filepath to the PATCHES.json file.
+
+    Returns:
+      PatchInfo for applied and disabled patches.
+    """
+    with patches_json_fp.open(encoding="utf-8") as f:
+        patch_entries = json_to_patch_entries(
+            patches_json_fp.parent,
+            f,
+        )
+    modified_entries, applied_patches = update_version_ranges_with_entries(
+        svn_version, llvm_src_dir, patch_entries
+    )
+    with atomic_write(patches_json_fp, encoding="utf-8") as f:
+        _write_json_changes([p.to_dict() for p in patch_entries], f)
+    for entry in modified_entries:
+        print(
+            f"Stopped applying {entry.rel_patch_path} ({entry.title()}) "
+            f"for r{svn_version}"
+        )
+    return PatchInfo(
+        non_applicable_patches=[],
+        applied_patches=[p.rel_patch_path for p in applied_patches],
+        failed_patches=[],
+        disabled_patches=[p.rel_patch_path for p in modified_entries],
+        removed_patches=[],
+        modified_metadata=patches_json_fp if modified_entries else None,
+    )
+
+
+def update_version_ranges_with_entries(
+    svn_version: int,
+    llvm_src_dir: Path,
+    patch_entries: Iterable[PatchEntry],
+) -> Tuple[List[PatchEntry], List[PatchEntry]]:
+    """Test-able helper for UpdateVersionRanges.
+
+    Args:
+      svn_version: LLVM revision number.
+      llvm_src_dir: llvm-project directory path.
+      patch_entries: PatchEntry objects to modify.
+
+    Returns:
+      Tuple of (modified entries, applied patches)
+
+    Post:
+      Modifies patch_entries in place.
+    """
+    modified_entries: List[PatchEntry] = []
+    applied_patches: List[PatchEntry] = []
+    with git_clean_context(llvm_src_dir):
+        for pe in patch_entries:
+            test_result = pe.test_apply(llvm_src_dir)
+            if not test_result:
+                if pe.version_range is None:
+                    pe.version_range = {}
+                pe.version_range["until"] = svn_version
+                modified_entries.append(pe)
+            else:
+                # We have to actually apply the patch so that future patches
+                # will stack properly.
+                if not pe.apply(llvm_src_dir).succeeded:
+                    raise RuntimeError(
+                        "Could not apply patch that dry ran successfully"
+                    )
+                applied_patches.append(pe)
+
+    return modified_entries, applied_patches
+
+
+def remove_old_patches(
+    svn_version: int, llvm_src_dir: Path, patches_json_fp: Path
+) -> PatchInfo:
+    """Remove patches that don't and will never apply for the future.
+
+    Patches are determined to be "old" via the "is_old" method for
+    each patch entry.
+
+    Args:
+      svn_version: LLVM SVN version.
+      llvm_src_dir: LLVM source directory.
+      patches_json_fp: Location to edit patches on.
+
+    Returns:
+      PatchInfo for modified patches.
+    """
+    with patches_json_fp.open(encoding="utf-8") as f:
+        patches_list = json.load(f)
+    patch_entries = (
+        PatchEntry.from_dict(llvm_src_dir, elem) for elem in patches_list
+    )
+    oldness = [(entry, entry.is_old(svn_version)) for entry in patch_entries]
+    filtered_entries = [entry.to_dict() for entry, old in oldness if not old]
+    with atomic_write(patches_json_fp, encoding="utf-8") as f:
+        _write_json_changes(filtered_entries, f)
+    removed_entries = [entry for entry, old in oldness if old]
+    plural_patches = "patch" if len(removed_entries) == 1 else "patches"
+    print(f"Removed {len(removed_entries)} old {plural_patches}:")
+    for r in removed_entries:
+        print(f"- {r.rel_patch_path}: {r.title()}")
+
+    patches_dir_path = llvm_src_dir / patches_json_fp.parent
+    return PatchInfo(
+        non_applicable_patches=[],
+        applied_patches=[],
+        failed_patches=[],
+        disabled_patches=[],
+        removed_patches=[
+            patches_dir_path / p.rel_patch_path for p in removed_entries
+        ],
+        modified_metadata=patches_json_fp if removed_entries else None,
+    )
diff --git a/llvm_tools/patch_utils_unittest.py b/llvm_tools/patch_utils_unittest.py
index 8fe45c2c..b8c21390 100755
--- a/llvm_tools/patch_utils_unittest.py
+++ b/llvm_tools/patch_utils_unittest.py
@@ -6,9 +6,11 @@
 """Unit tests for the patch_utils.py file."""
 
 import io
+import json
 from pathlib import Path
 import subprocess
 import tempfile
+from typing import Callable
 import unittest
 from unittest import mock
 
@@ -99,7 +101,7 @@ class TestPatchUtils(unittest.TestCase):
 
     def test_can_parse_from_json(self):
         """Test that patches be loaded from json."""
-        json = """
+        patches_json = """
 [
   {
     "metadata": {},
@@ -121,7 +123,7 @@ class TestPatchUtils(unittest.TestCase):
   }
 ]
     """
-        result = pu.json_to_patch_entries(Path(), io.StringIO(json))
+        result = pu.json_to_patch_entries(Path(), io.StringIO(patches_json))
         self.assertEqual(len(result), 4)
 
     def test_parsed_hunks(self):
@@ -215,6 +217,97 @@ Hunk #1 SUCCEEDED at 96 with fuzz 1.
                 test_file.write_text("abc")
             self.assertTrue(pu.is_git_dirty(dirpath))
 
+    @mock.patch("patch_utils.git_clean_context", mock.MagicMock)
+    def test_update_version_ranges(self):
+        """Test the UpdateVersionRanges function."""
+        with tempfile.TemporaryDirectory(
+            prefix="patch_manager_unittest"
+        ) as dirname:
+            dirpath = Path(dirname)
+            patches = [
+                pu.PatchEntry(
+                    workdir=dirpath,
+                    rel_patch_path="x.patch",
+                    metadata=None,
+                    platforms=None,
+                    version_range={
+                        "from": 0,
+                        "until": 2,
+                    },
+                ),
+                pu.PatchEntry(
+                    workdir=dirpath,
+                    rel_patch_path="y.patch",
+                    metadata=None,
+                    platforms=None,
+                    version_range={
+                        "from": 0,
+                        "until": 2,
+                    },
+                ),
+            ]
+            patches[0].apply = mock.MagicMock(
+                return_value=pu.PatchResult(
+                    succeeded=False, failed_hunks={"a/b/c": []}
+                )
+            )
+            patches[1].apply = mock.MagicMock(
+                return_value=pu.PatchResult(succeeded=True)
+            )
+            results, _ = pu.update_version_ranges_with_entries(
+                1, dirpath, patches
+            )
+            # We should only have updated the version_range of the first patch,
+            # as that one failed to apply.
+            self.assertEqual(len(results), 1)
+            self.assertEqual(results[0].version_range, {"from": 0, "until": 1})
+            self.assertEqual(patches[0].version_range, {"from": 0, "until": 1})
+            self.assertEqual(patches[1].version_range, {"from": 0, "until": 2})
+
+    @mock.patch("builtins.print")
+    def test_remove_old_patches(self, _):
+        """Can remove old patches from PATCHES.json."""
+        one_patch_dict = {
+            "metadata": {
+                "title": "[some label] hello world",
+            },
+            "platforms": [
+                "chromiumos",
+            ],
+            "rel_patch_path": "x/y/z",
+            "version_range": {
+                "from": 4,
+                "until": 5,
+            },
+        }
+        patches = [
+            one_patch_dict,
+            {**one_patch_dict, "version_range": {"until": None}},
+            {**one_patch_dict, "version_range": {"from": 100}},
+            {**one_patch_dict, "version_range": {"until": 8}},
+        ]
+        cases = [
+            (0, lambda x: self.assertEqual(len(x), 4)),
+            (6, lambda x: self.assertEqual(len(x), 3)),
+            (8, lambda x: self.assertEqual(len(x), 2)),
+            (1000, lambda x: self.assertEqual(len(x), 2)),
+        ]
+
+        def _t(dirname: str, svn_version: int, assertion_f: Callable):
+            json_filepath = Path(dirname) / "PATCHES.json"
+            with json_filepath.open("w", encoding="utf-8") as f:
+                json.dump(patches, f)
+            pu.remove_old_patches(svn_version, Path(), json_filepath)
+            with json_filepath.open("r", encoding="utf-8") as f:
+                result = json.load(f)
+            assertion_f(result)
+
+        with tempfile.TemporaryDirectory(
+            prefix="patch_utils_unittest"
+        ) as dirname:
+            for r, a in cases:
+                _t(dirname, r, a)
+
     @staticmethod
     def _default_json_dict():
         return {
diff --git a/llvm_tools/update_chromeos_llvm_hash.py b/llvm_tools/update_chromeos_llvm_hash.py
index c52c7328..75c6ce6c 100755
--- a/llvm_tools/update_chromeos_llvm_hash.py
+++ b/llvm_tools/update_chromeos_llvm_hash.py
@@ -411,7 +411,7 @@ def RemovePatchesFromFilesDir(patches):
     """Removes the patches from $FILESDIR of a package.
 
     Args:
-      patches: A list of absolute pathes of patches to remove
+      patches: A list of absolute paths of patches to remove
 
     Raises:
       ValueError: Failed to remove a patch in $FILESDIR.
@@ -728,13 +728,26 @@ def UpdatePackagesPatchMetadataFile(
 
                 src_path = Path(dirname)
                 with patch_utils.git_clean_context(src_path):
-                    patches_info = patch_utils.apply_all_from_json(
-                        svn_version=svn_version,
-                        llvm_src_dir=src_path,
-                        patches_json_fp=patches_json_fp,
-                        continue_on_failure=mode
-                        == failure_modes.FailureModes.CONTINUE,
-                    )
+                    if (
+                        mode == failure_modes.FailureModes.FAIL
+                        or mode == failure_modes.FailureModes.CONTINUE
+                    ):
+                        patches_info = patch_utils.apply_all_from_json(
+                            svn_version=svn_version,
+                            llvm_src_dir=src_path,
+                            patches_json_fp=patches_json_fp,
+                            continue_on_failure=mode
+                            == failure_modes.FailureModes.CONTINUE,
+                        )
+                    elif mode == failure_modes.FailureModes.REMOVE_PATCHES:
+                        patches_info = patch_utils.remove_old_patches(
+                            svn_version, src_path, patches_json_fp
+                        )
+                    elif mode == failure_modes.FailureModes.DISABLE_PATCHES:
+                        patches_info = patch_utils.update_version_ranges(
+                            svn_version, src_path, patches_json_fp
+                        )
+
                 package_info[cur_package] = patches_info._asdict()
 
     return package_info