patches: make re-applied patches idempotent (#26784)

We use POSIX `patch` to apply patches to files when building, but
`patch` by default prompts the user when it looks like a patch
has already been applied. This means that:

1. If a patch lands in upstream and we don't disable it
   in a package, the build will start failing.
2. `spack develop` builds (which keep the stage around) will
   fail the second time you try to use them.

To avoid that, we can run `patch` with `-N` (also called
`--forward`, but the long option is not in POSIX). `-N` causes
`patch` to just ignore patches that have already been applied.
This *almost* makes `patch` idempotent, except that it returns 1
when it detects already applied patches with `-N`, so we have to
look at the output of the command to see if it's safe to ignore
the error.

- [x] Remove non-POSIX `-s` option from `patch` call
- [x] Add `-N` option to `patch`
- [x] Ignore error status when `patch` returns 1 due to `-N`
- [x] Add tests for applying a patch twice and applying a bad patch
- [x] Tweak `spack.util.executable` so that it saves the error that
      *would have been* raised with `fail_on_error=True`. This lets
      us easily re-raise it.

Co-authored-by: Greg Becker <becker33@llnl.gov>

4 files changed, 137 insertions, 25 deletions

diff --git a/lib/spack/spack/patch.py b/lib/spack/spack/patch.py
index a273514636..cdd9ad0387 100644
--- a/lib/spack/spack/patch.py
+++ b/lib/spack/spack/patch.py
@@ -7,6 +7,7 @@ import hashlib
 import inspect
 import os
 import os.path
+import sys
 
 import llnl.util.filesystem
 import llnl.util.lang
@@ -25,6 +26,12 @@ from spack.util.executable import which
 def apply_patch(stage, patch_path, level=1, working_dir='.'):
     """Apply the patch at patch_path to code in the stage.
 
+    Spack runs ``patch`` with ``-N`` so that it does not reject already-applied
+    patches.  This is useful for develop specs, so that the build does not fail
+    due to repeated application of patches, and for easing requirements on patch
+    specifications in packages -- packages won't stop working when patches we
+    previously had to apply land in upstream.
+
     Args:
         stage (spack.stage.Stage): stage with code that will be patched
         patch_path (str): filesystem location for the patch to apply
@@ -34,10 +41,31 @@ def apply_patch(stage, patch_path, level=1, working_dir='.'):
     """
     patch = which("patch", required=True)
     with llnl.util.filesystem.working_dir(stage.source_path):
-        patch('-s',
-              '-p', str(level),
-              '-i', patch_path,
-              '-d', working_dir)
+        output = patch(
+            '-N',               # don't reject already-applied patches
+            '-p', str(level),   # patch level (directory depth)
+            '-i', patch_path,   # input source is the patch file
+            '-d', working_dir,  # patch chdir's to here before patching
+            output=str,
+            fail_on_error=False,
+        )
+
+        if patch.returncode != 0:
+            # `patch` returns 1 both:
+            #   a) when an error applying a patch, and
+            #   b) when -N is supplied and the patch has already been applied
+            #
+            # It returns > 1 if there's something more serious wrong.
+            #
+            # So, the best we can do is to look for return code 1, look for output
+            # indicating that the patch was already applied, and ignore the error
+            # if we see it. Most implementations (BSD and GNU) seem to have the
+            # same messages, so we expect these checks to be reliable.
+            if patch.returncode > 1 or not any(
+                    s in output for s in ("Skipping patch", "ignored")
+            ):
+                sys.stderr.write(output)
+                raise patch.error
 
 
 class Patch(object):
diff --git a/lib/spack/spack/test/patch.py b/lib/spack/spack/test/patch.py
index 8383100440..5285be8ba9 100644
--- a/lib/spack/spack/test/patch.py
+++ b/lib/spack/spack/test/patch.py
@@ -15,8 +15,9 @@ import spack.patch
 import spack.paths
 import spack.repo
 import spack.util.compression
+import spack.util.crypto
 from spack.spec import Spec
-from spack.stage import Stage
+from spack.stage import DIYStage, Stage
 from spack.util.executable import Executable
 
 # various sha256 sums (using variables for legibility)
@@ -33,6 +34,43 @@ url2_sha256 = '1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd'
 url2_archive_sha256 = 'abcdabcdabcdabcdabcdabcdabcdabcdabcdabcdabcdabcdabcdabcdabcdabcd'
 
 
+# some simple files for patch tests
+file_to_patch = """\
+first line
+second line
+"""
+
+patch_file = """\
+diff a/foo.txt b/foo-expected.txt
+--- a/foo.txt
++++ b/foo-expected.txt
+@@ -1,2 +1,3 @@
++zeroth line
+ first line
+-second line
++third line
+"""
+
+expected_patch_result = """\
+zeroth line
+first line
+third line
+"""
+
+file_patch_cant_apply_to = """\
+this file
+is completely different
+from anything in the files
+or patch above
+"""
+
+
+def write_file(filename, contents):
+    """Helper function for setting up tests."""
+    with open(filename, 'w') as f:
+        f.write(contents)
+
+
 @pytest.fixture()
 def mock_patch_stage(tmpdir_factory, monkeypatch):
     # Don't disrupt the spack install directory with tests.
@@ -67,19 +105,9 @@ def test_url_patch(mock_patch_stage, filename, sha256, archive_sha256):
 
         mkdirp(stage.source_path)
         with working_dir(stage.source_path):
-            # write a file to be patched
-            with open('foo.txt', 'w') as f:
-                f.write("""\
-first line
-second line
-""")
-            # write the expected result of patching.
-            with open('foo-expected.txt', 'w') as f:
-                f.write("""\
-zeroth line
-first line
-third line
-""")
+            write_file("foo.txt", file_to_patch)
+            write_file("foo-expected.txt", expected_patch_result)
+
         # apply the patch and compare files
         patch.fetch()
         patch.apply(stage)
@@ -89,6 +117,47 @@ third line
             assert filecmp.cmp('foo.txt', 'foo-expected.txt')
 
 
+def test_apply_patch_twice(mock_patch_stage, tmpdir):
+    """Ensure that patch doesn't fail if applied twice."""
+
+    stage = DIYStage(str(tmpdir))
+    with tmpdir.as_cwd():
+        write_file("foo.txt", file_to_patch)
+        write_file("foo-expected.txt", expected_patch_result)
+        write_file("foo.patch", patch_file)
+
+    FakePackage = collections.namedtuple(
+        'FakePackage', ['name', 'namespace', 'fullname'])
+    fake_pkg = FakePackage('fake-package', 'test', 'fake-package')
+
+    def make_patch(filename):
+        path = os.path.realpath(str(tmpdir.join(filename)))
+        url = 'file://' + path
+        sha256 = spack.util.crypto.checksum("sha256", path)
+        return spack.patch.UrlPatch(fake_pkg, url, sha256=sha256)
+
+    # apply the first time
+    patch = make_patch('foo.patch')
+    patch.fetch()
+
+    patch.apply(stage)
+    with working_dir(stage.source_path):
+        assert filecmp.cmp('foo.txt', 'foo-expected.txt')
+
+    # ensure apply() is idempotent
+    patch.apply(stage)
+    with working_dir(stage.source_path):
+        assert filecmp.cmp('foo.txt', 'foo-expected.txt')
+
+    # now write a file that can't be patched
+    with working_dir(stage.source_path):
+        write_file("foo.txt", file_patch_cant_apply_to)
+
+    # this application should fail with a real error
+    with pytest.raises(spack.util.executable.ProcessError):
+        patch.apply(stage)
+
+
 def test_patch_in_spec(mock_packages, config):
     """Test whether patches in a package appear in the spec."""
     spec = Spec('patch')
diff --git a/lib/spack/spack/util/crypto.py b/lib/spack/spack/util/crypto.py
index 549216a4c6..8e055126fe 100644
--- a/lib/spack/spack/util/crypto.py
+++ b/lib/spack/spack/util/crypto.py
@@ -92,6 +92,11 @@ def checksum(hashlib_algo, filename, **kwargs):
     """Returns a hex digest of the filename generated using an
        algorithm from hashlib.
     """
+    if isinstance(hashlib_algo, str):
+        if hashlib_algo not in hashes:
+            raise ValueError("Invalid hash algorithm: ", hashlib_algo)
+        hashlib_algo = hash_fun_for_algo(hashlib_algo)
+
     block_size = kwargs.get('block_size', 2**20)
     hasher = hashlib_algo()
     with open(filename, 'rb') as file:
diff --git a/lib/spack/spack/util/executable.py b/lib/spack/spack/util/executable.py
index e615ccdcfd..ded429c2e6 100644
--- a/lib/spack/spack/util/executable.py
+++ b/lib/spack/spack/util/executable.py
@@ -27,6 +27,7 @@ class Executable(object):
         from spack.util.environment import EnvironmentModifications  # no cycle
         self.default_envmod = EnvironmentModifications()
         self.returncode = None
+        self.error = None  # saved ProcessError when fail_on_error
 
         if not self.exe:
             raise ProcessError("Cannot construct executable for '%s'" % name)
@@ -90,7 +91,8 @@ class Executable(object):
                 the environment (neither requires nor precludes env)
             fail_on_error (bool): Raise an exception if the subprocess returns
                 an error. Default is True. The return code is available as
-                ``exe.returncode``
+                ``exe.returncode``, and a saved ``ProcessError`` that would
+                have been raised is in ``exe.error``.
             ignore_errors (int or list): A list of error codes to ignore.
                 If these codes are returned, this process will not raise
                 an exception even if ``fail_on_error`` is set to ``True``
@@ -213,7 +215,7 @@ class Executable(object):
                         sys.stderr.write(errstr)
 
             rc = self.returncode = proc.returncode
-            if fail_on_error and rc != 0 and (rc not in ignore_errors):
+            if rc != 0:
                 long_msg = cmd_line_string
                 if result:
                     # If the output is not captured in the result, it will have
@@ -222,8 +224,11 @@ class Executable(object):
                     # stdout/stderr (e.g. if 'output' is not specified)
                     long_msg += '\n' + result
 
-                raise ProcessError('Command exited with status %d:' %
-                                   proc.returncode, long_msg)
+                self.error = ProcessError(
+                    'Command exited with status %d:' % proc.returncode, long_msg
+                )
+                if fail_on_error and (rc not in ignore_errors):
+                    raise self.error
 
             return result
 
@@ -232,10 +237,15 @@ class Executable(object):
                 '%s: %s' % (self.exe[0], e.strerror), 'Command: ' + cmd_line_string)
 
         except subprocess.CalledProcessError as e:
+            self.error = ProcessError(
+                str(e),
+                '\nExit status %d when invoking command: %s' % (
+                    proc.returncode,
+                    cmd_line_string,
+                ),
+            )
             if fail_on_error:
-                raise ProcessError(
-                    str(e), '\nExit status %d when invoking command: %s' %
-                    (proc.returncode, cmd_line_string))
+                raise self.error
 
         finally:
             if close_ostream: