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>
This commit is contained in:
Todd Gamblin
GitHub
parent
a118e799ae
commit
c5ca0db27f
@ -7,6 +7,7 @@
|
||||
import inspect
|
||||
import os
|
||||
import os.path
|
||||
import sys
|
||||
|
||||
import llnl.util.filesystem
|
||||
import llnl.util.lang
|
||||
@ -25,6 +26,12 @@
|
||||
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):
|
||||
|
||||
@ -15,8 +15,9 @@
|
||||
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_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 @@ 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')
|
||||
|
||||
@ -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:
|
||||
|
||||
@ -27,6 +27,7 @@ def __init__(self, name):
|
||||
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 @@ def __call__(self, *args, **kwargs):
|
||||
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 @@ def streamify(arg, mode):
|
||||
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 @@ def streamify(arg, mode):
|
||||
# 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 @@ def streamify(arg, mode):
|
||||
'%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:
|
||||
|
||||
Loading…
Reference in New Issue
Block a user