spack.patch: support reversing patches (#43040)

The `patch()` directive can now be invoked with `reverse=True` to apply a patch in reverse.
This is useful for reverting commits that caused errors in projects, even if only the forward
patch is available, e.g. via a GitHub commit patch URL.

`patch(..., reverse=True)` runs `patch -R` behind the scenes. This is a POSIX option so we
can expect it to be available on the `patch` command.

---------

Co-authored-by: Massimiliano Culpo <massimiliano.culpo@gmail.com>

lib/spack/spack/directives.py

@@ -660,6 +660,7 @@ def patch(
     level: int = 1,
     when: WhenType = None,
     working_dir: str = ".",
+    reverse: bool = False,
     sha256: Optional[str] = None,
     archive_sha256: Optional[str] = None,
 ) -> Patcher:
@@ -673,10 +674,10 @@ def patch(
         level: patch level (as in the patch shell command)
         when: optional anonymous spec that specifies when to apply the patch
         working_dir: dir to change to before applying
+        reverse: reverse the patch
         sha256: sha256 sum of the patch, used to verify the patch (only required for URL patches)
         archive_sha256: sha256 sum of the *archive*, if the patch is compressed (only required for
             compressed URL patches)
-
     """
 
     def _execute_patch(pkg_or_dep: Union["spack.package_base.PackageBase", Dependency]):
@@ -711,13 +712,14 @@ def _execute_patch(pkg_or_dep: Union["spack.package_base.PackageBase", Dependenc
                 url_or_filename,
                 level,
                 working_dir=working_dir,
+                reverse=reverse,
                 ordering_key=ordering_key,
                 sha256=sha256,
                 archive_sha256=archive_sha256,
             )
         else:
             patch = spack.patch.FilePatch(
-                pkg, url_or_filename, level, working_dir, ordering_key=ordering_key
+                pkg, url_or_filename, level, working_dir, reverse, ordering_key=ordering_key
             )
 
         cur_patches.append(patch)

lib/spack/spack/patch.py

@@ -26,7 +26,11 @@
 
 
 def apply_patch(
-    stage: "spack.stage.Stage", patch_path: str, level: int = 1, working_dir: str = "."
+    stage: "spack.stage.Stage",
+    patch_path: str,
+    level: int = 1,
+    working_dir: str = ".",
+    reverse: bool = False,
 ) -> None:
     """Apply the patch at patch_path to code in the stage.
 
@@ -35,6 +39,7 @@ def apply_patch(
         patch_path: filesystem location for the patch to apply
         level: patch level
         working_dir: relative path *within* the stage to change to
+        reverse: reverse the patch
     """
     git_utils_path = os.environ.get("PATH", "")
     if sys.platform == "win32":
@@ -45,6 +50,10 @@ def apply_patch(
             git_root = git_root / "usr" / "bin"
             git_utils_path = os.pathsep.join([str(git_root), git_utils_path])
 
+    args = ["-s", "-p", str(level), "-i", patch_path, "-d", working_dir]
+    if reverse:
+        args.append("-R")
+
     # TODO: Decouple Spack's patch support on Windows from Git
     # for Windows, and instead have Spack directly fetch, install, and
     # utilize that patch.
@@ -53,7 +62,7 @@ def apply_patch(
     # flag is passed.
     patch = which("patch", required=True, path=git_utils_path)
     with llnl.util.filesystem.working_dir(stage.source_path):
-        patch("-s", "-p", str(level), "-i", patch_path, "-d", working_dir)
+        patch(*args)
 
 
 class Patch:
@@ -67,7 +76,12 @@ class Patch:
     sha256: str
 
     def __init__(
-        self, pkg: "spack.package_base.PackageBase", path_or_url: str, level: int, working_dir: str
+        self,
+        pkg: "spack.package_base.PackageBase",
+        path_or_url: str,
+        level: int,
+        working_dir: str,
+        reverse: bool = False,
     ) -> None:
         """Initialize a new Patch instance.
 
@@ -76,6 +90,7 @@ def __init__(
             path_or_url: the relative path or URL to a patch file
             level: patch level
             working_dir: relative path *within* the stage to change to
+            reverse: reverse the patch
         """
         # validate level (must be an integer >= 0)
         if not isinstance(level, int) or not level >= 0:
@@ -87,6 +102,7 @@ def __init__(
         self.path: Optional[str] = None  # must be set before apply()
         self.level = level
         self.working_dir = working_dir
+        self.reverse = reverse
 
     def apply(self, stage: "spack.stage.Stage") -> None:
         """Apply a patch to source in a stage.
@@ -97,7 +113,7 @@ def apply(self, stage: "spack.stage.Stage") -> None:
         if not self.path or not os.path.isfile(self.path):
             raise NoSuchPatchError(f"No such patch: {self.path}")
 
-        apply_patch(stage, self.path, self.level, self.working_dir)
+        apply_patch(stage, self.path, self.level, self.working_dir, self.reverse)
 
     # TODO: Use TypedDict once Spack supports Python 3.8+ only
     def to_dict(self) -> Dict[str, Any]:
@@ -111,6 +127,7 @@ def to_dict(self) -> Dict[str, Any]:
             "sha256": self.sha256,
             "level": self.level,
             "working_dir": self.working_dir,
+            "reverse": self.reverse,
         }
 
     def __eq__(self, other: object) -> bool:
@@ -146,6 +163,7 @@ def __init__(
         relative_path: str,
         level: int,
         working_dir: str,
+        reverse: bool = False,
         ordering_key: Optional[Tuple[str, int]] = None,
     ) -> None:
         """Initialize a new FilePatch instance.
@@ -155,6 +173,7 @@ def __init__(
             relative_path: path to patch, relative to the repository directory for a package.
             level: level to pass to patch command
             working_dir: path within the source directory where patch should be applied
+            reverse: reverse the patch
             ordering_key: key used to ensure patches are applied in a consistent order
         """
         self.relative_path = relative_path
@@ -182,7 +201,7 @@ def __init__(
             msg += "package %s.%s does not exist." % (pkg.namespace, pkg.name)
             raise ValueError(msg)
 
-        super().__init__(pkg, abs_path, level, working_dir)
+        super().__init__(pkg, abs_path, level, working_dir, reverse)
         self.path = abs_path
         self.ordering_key = ordering_key
 
@@ -228,6 +247,7 @@ def __init__(
         level: int = 1,
         *,
         working_dir: str = ".",
+        reverse: bool = False,
         sha256: str,  # This is required for UrlPatch
         ordering_key: Optional[Tuple[str, int]] = None,
         archive_sha256: Optional[str] = None,
@@ -239,12 +259,13 @@ def __init__(
             url: URL where the patch can be fetched
             level: level to pass to patch command
             working_dir: path within the source directory where patch should be applied
+            reverse: reverse the patch
             ordering_key: key used to ensure patches are applied in a consistent order
             sha256: sha256 sum of the patch, used to verify the patch
             archive_sha256: sha256 sum of the *archive*, if the patch is compressed
                 (only required for compressed URL patches)
         """
-        super().__init__(pkg, url, level, working_dir)
+        super().__init__(pkg, url, level, working_dir, reverse)
 
         self.url = url
         self._stage: Optional["spack.stage.Stage"] = None
@@ -350,13 +371,20 @@ def from_dict(
             dictionary["url"],
             dictionary["level"],
             working_dir=dictionary["working_dir"],
+            # Added in v0.22, fallback required for backwards compatibility
+            reverse=dictionary.get("reverse", False),
             sha256=dictionary["sha256"],
             archive_sha256=dictionary.get("archive_sha256"),
         )
 
     elif "relative_path" in dictionary:
         patch = FilePatch(
-            pkg_cls, dictionary["relative_path"], dictionary["level"], dictionary["working_dir"]
+            pkg_cls,
+            dictionary["relative_path"],
+            dictionary["level"],
+            dictionary["working_dir"],
+            # Added in v0.22, fallback required for backwards compatibility
+            dictionary.get("reverse", False),
         )
 
         # If the patch in the repo changes, we cannot get it back, so we

lib/spack/spack/test/patch.py

@@ -6,6 +6,7 @@
 import collections
 import filecmp
 import os
+import shutil
 import sys
 
 import pytest
@@ -89,7 +90,6 @@ def test_url_patch(mock_patch_stage, filename, sha256, archive_sha256, config):
     # Make a patch object
     url = url_util.path_to_file_url(filename)
     s = Spec("patch").concretized()
-    patch = spack.patch.UrlPatch(s.package, url, sha256=sha256, archive_sha256=archive_sha256)
 
     # make a stage
     with Stage(url) as stage:  # TODO: url isn't used; maybe refactor Stage
@@ -105,6 +105,8 @@ def test_url_patch(mock_patch_stage, filename, sha256, archive_sha256, config):
 second line
 """
                 )
+            # save it for later comparison
+            shutil.copyfile("foo.txt", "foo-original.txt")
             # write the expected result of patching.
             with open("foo-expected.txt", "w") as f:
                 f.write(
@@ -115,6 +117,7 @@ def test_url_patch(mock_patch_stage, filename, sha256, archive_sha256, config):
 """
                 )
         # apply the patch and compare files
+        patch = spack.patch.UrlPatch(s.package, url, sha256=sha256, archive_sha256=archive_sha256)
         with patch.stage:
             patch.stage.create()
             patch.stage.fetch()
@@ -124,6 +127,19 @@ def test_url_patch(mock_patch_stage, filename, sha256, archive_sha256, config):
         with working_dir(stage.source_path):
             assert filecmp.cmp("foo.txt", "foo-expected.txt")
 
+        # apply the patch in reverse and compare files
+        patch = spack.patch.UrlPatch(
+            s.package, url, sha256=sha256, archive_sha256=archive_sha256, reverse=True
+        )
+        with patch.stage:
+            patch.stage.create()
+            patch.stage.fetch()
+            patch.stage.expand_archive()
+            patch.apply(stage)
+
+        with working_dir(stage.source_path):
+            assert filecmp.cmp("foo.txt", "foo-original.txt")
+
 
 def test_patch_in_spec(mock_packages, config):
     """Test whether patches in a package appear in the spec."""
@@ -425,6 +441,19 @@ def test_patch_no_file():
         patch.apply("")
 
 
+def test_patch_no_sha256():
+    # Give it the attributes we need to construct the error message
+    FakePackage = collections.namedtuple("FakePackage", ["name", "namespace", "fullname"])
+    fp = FakePackage("fake-package", "test", "fake-package")
+    url = url_util.path_to_file_url("foo.tgz")
+    match = "Compressed patches require 'archive_sha256' and patch 'sha256' attributes: file://"
+    with pytest.raises(spack.patch.PatchDirectiveError, match=match):
+        spack.patch.UrlPatch(fp, url, sha256="", archive_sha256="")
+    match = "URL patches require a sha256 checksum"
+    with pytest.raises(spack.patch.PatchDirectiveError, match=match):
+        spack.patch.UrlPatch(fp, url, sha256="", archive_sha256="abc")
+
+
 @pytest.mark.parametrize("level", [-1, 0.0, "1"])
 def test_invalid_level(level):
     # Give it the attributes we need to construct the error message
@@ -432,3 +461,41 @@ def test_invalid_level(level):
     fp = FakePackage("fake-package", "test")
     with pytest.raises(ValueError, match="Patch level needs to be a non-negative integer."):
         spack.patch.Patch(fp, "nonexistent_file", level, "")
+
+
+def test_equality():
+    FakePackage = collections.namedtuple("FakePackage", ["name", "namespace", "fullname"])
+    fp = FakePackage("fake-package", "test", "fake-package")
+    patch1 = spack.patch.UrlPatch(fp, "nonexistent_url1", sha256="abc")
+    patch2 = spack.patch.UrlPatch(fp, "nonexistent_url2", sha256="def")
+    assert patch1 == patch1
+    assert patch1 != patch2
+    assert patch1 != "not a patch"
+
+
+def test_sha256_setter(mock_patch_stage, config):
+    path = os.path.join(data_path, "foo.patch")
+    s = Spec("patch").concretized()
+    patch = spack.patch.FilePatch(s.package, path, level=1, working_dir=".")
+    patch.sha256 = "abc"
+
+
+def test_invalid_from_dict(mock_packages, config):
+    dictionary = {}
+    with pytest.raises(ValueError, match="Invalid patch dictionary:"):
+        spack.patch.from_dict(dictionary)
+
+    dictionary = {"owner": "patch"}
+    with pytest.raises(ValueError, match="Invalid patch dictionary:"):
+        spack.patch.from_dict(dictionary)
+
+    dictionary = {
+        "owner": "patch",
+        "relative_path": "foo.patch",
+        "level": 1,
+        "working_dir": ".",
+        "reverse": False,
+        "sha256": bar_sha256,
+    }
+    with pytest.raises(spack.fetch_strategy.ChecksumError, match="sha256 checksum failed for"):
+        spack.patch.from_dict(dictionary)