From 72ca7d6ee5bb61153487a3b26ec800d1ed5dc8e6 Mon Sep 17 00:00:00 2001 From: Harmen Stoppels Date: Wed, 15 Dec 2021 17:56:03 +0100 Subject: Revert "patches: make re-applied patches idempotent (#26784)" (#27625) This reverts commit c5ca0db27fce5d772dc8a4fcffec3b62bb0bf1f3. --- lib/spack/spack/patch.py | 36 ++------------ lib/spack/spack/test/patch.py | 97 ++++++-------------------------------- lib/spack/spack/util/crypto.py | 5 -- lib/spack/spack/util/executable.py | 24 +++------- 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: -- cgit v1.2.3-70-g09d2