Revert "patches: make re-applied patches idempotent (#26784)" (#27625)

This reverts commit c5ca0db27fce5d772dc8a4fcffec3b62bb0bf1f3.

4 files changed, 25 insertions, 137 deletions

diff --git a/lib/spack/spack/patch.py b/lib/spack/spack/patch.py
index cdd9ad0387..a273514636 100644
--- a/lib/spack/spack/patch.py
+++ b/lib/spack/spack/patch.py
@@ -7,7 +7,6 @@ import hashlib
 import inspect
 import os
 import os.path
-import sys
 
 import llnl.util.filesystem
 import llnl.util.lang
@@ -26,12 +25,6 @@ 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
@@ -41,31 +34,10 @@ def apply_patch(stage, patch_path, level=1, working_dir='.'):
     """
     patch = which("patch", required=True)
     with llnl.util.filesystem.working_dir(stage.source_path):
-        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
+        patch('-s',
+              '-p', str(level),
+              '-i', patch_path,
+              '-d', working_dir)
 
 
 class Patch(object):
diff --git a/lib/spack/spack/test/patch.py b/lib/spack/spack/test/patch.py
index 5285be8ba9..8383100440 100644
--- a/lib/spack/spack/test/patch.py
+++ b/lib/spack/spack/test/patch.py
@@ -15,9 +15,8 @@ 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 DIYStage, Stage
+from spack.stage import Stage
 from spack.util.executable import Executable
 
 # various sha256 sums (using variables for legibility)
@@ -34,43 +33,6 @@ 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.
@@ -105,9 +67,19 @@ def test_url_patch(mock_patch_stage, filename, sha256, archive_sha256):
 
         mkdirp(stage.source_path)
         with working_dir(stage.source_path):
-            write_file("foo.txt", file_to_patch)
-            write_file("foo-expected.txt", expected_patch_result)
-
+            # 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
+""")
         # apply the patch and compare files
         patch.fetch()
         patch.apply(stage)
@@ -117,47 +89,6 @@ def test_url_patch(mock_patch_stage, filename, sha256, archive_sha256):
             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 8e055126fe..549216a4c6 100644
--- a/lib/spack/spack/util/crypto.py
+++ b/lib/spack/spack/util/crypto.py
@@ -92,11 +92,6 @@ 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 ded429c2e6..e615ccdcfd 100644
--- a/lib/spack/spack/util/executable.py
+++ b/lib/spack/spack/util/executable.py
@@ -27,7 +27,6 @@ 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)
@@ -91,8 +90,7 @@ 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``, and a saved ``ProcessError`` that would
-                have been raised is in ``exe.error``.
+                ``exe.returncode``
             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``
@@ -215,7 +213,7 @@ class Executable(object):
                         sys.stderr.write(errstr)
 
             rc = self.returncode = proc.returncode
-            if rc != 0:
+            if fail_on_error and rc != 0 and (rc not in ignore_errors):
                 long_msg = cmd_line_string
                 if result:
                     # If the output is not captured in the result, it will have
@@ -224,11 +222,8 @@ class Executable(object):
                     # stdout/stderr (e.g. if 'output' is not specified)
                     long_msg += '\n' + result
 
-                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
+                raise ProcessError('Command exited with status %d:' %
+                                   proc.returncode, long_msg)
 
             return result
 
@@ -237,15 +232,10 @@ 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 self.error
+                raise ProcessError(
+                    str(e), '\nExit status %d when invoking command: %s' %
+                    (proc.returncode, cmd_line_string))
 
         finally:
             if close_ostream: