From c63c4a048c796c900f90728c6ea11d5972cabd95 Mon Sep 17 00:00:00 2001
From: Greg Becker <becker33@llnl.gov>
Date: Fri, 11 Jan 2019 14:52:01 -0800
Subject: [PATCH] Binary caching bugfix: symlink relocation (#10073)
Binary caches of packages with absolute symlinks had broken symlinks.
As a stopgap measure, #9747 addressed this by replacing symlinks with
copies of files when creating binary cached packages.
This reverts #9747 and instead, either relative-izes the symlink or
rewrites the target. If the binary cache is created using '--rel' (as
in "spack buildcache create --rel...") then absolute symlinks will be
replaced with relative symlinks (in addition to making RPATHs relative
as before); otherwise they are rewritten (when the binary cache is
unpacked and installed).
---
lib/spack/spack/binary_distribution.py | 44 +++++++++++++++++++----
lib/spack/spack/relocate.py | 47 ++++++++++++++++++++++++
lib/spack/spack/test/packaging.py | 49 ++++++++++++++++++++++++--
3 files changed, 131 insertions(+), 9 deletions(-)
diff --git a/lib/spack/spack/binary_distribution.py b/lib/spack/spack/binary_distribution.py
index 4a5522340b..7bceb1189e 100644
--- a/lib/spack/spack/binary_distribution.py
+++ b/lib/spack/spack/binary_distribution.py
@@ -116,6 +116,7 @@ def write_buildinfo_file(prefix, workdir, rel=False):
"""
text_to_relocate = []
binary_to_relocate = []
+ link_to_relocate = []
blacklist = (".spack", "man")
os_id = platform.system()
# Do this at during tarball creation to save time when tarball unpacked.
@@ -124,10 +125,23 @@ def write_buildinfo_file(prefix, workdir, rel=False):
dirs[:] = [d for d in dirs if d not in blacklist]
for filename in files:
path_name = os.path.join(root, filename)
+ if os.path.islink(path_name):
+ link = os.readlink(path_name)
+ if os.path.isabs(link):
+ # Relocate absolute links into the spack tree
+ if link.startswith(spack.store.layout.root):
+ rel_path_name = os.path.relpath(path_name, prefix)
+ link_to_relocate.append(rel_path_name)
+ else:
+ msg = 'Absolute link %s to %s ' % (path_name, link)
+ msg += 'outside of stage %s ' % prefix
+ msg += 'cannot be relocated.'
+ tty.warn(msg)
+
# Check if the file contains a string with the installroot.
# This cuts down on the number of files added to the list
# of files potentially needing relocation
- if relocate.strings_contains_installroot(
+ elif relocate.strings_contains_installroot(
path_name, spack.store.layout.root):
filetype = get_filetype(path_name)
if relocate.needs_binary_relocation(filetype, os_id):
@@ -145,6 +159,7 @@ def write_buildinfo_file(prefix, workdir, rel=False):
prefix, spack.store.layout.root)
buildinfo['relocate_textfiles'] = text_to_relocate
buildinfo['relocate_binaries'] = binary_to_relocate
+ buildinfo['relocate_links'] = link_to_relocate
filename = buildinfo_file_name(workdir)
with open(filename, 'w') as outfile:
outfile.write(yaml.dump(buildinfo, default_flow_style=True))
@@ -269,9 +284,7 @@ def build_tarball(spec, outdir, force=False, rel=False, unsigned=False,
raise NoOverwriteException(str(specfile_path))
# make a copy of the install directory to work with
workdir = os.path.join(tempfile.mkdtemp(), os.path.basename(spec.prefix))
- # set symlinks=False here to avoid broken symlinks when archiving and
- # moving the package
- install_tree(spec.prefix, workdir, symlinks=False)
+ install_tree(spec.prefix, workdir, symlinks=True)
# create info for later relocation and create tar
write_buildinfo_file(spec.prefix, workdir, rel=rel)
@@ -287,7 +300,7 @@ def build_tarball(spec, outdir, force=False, rel=False, unsigned=False,
tty.die(str(e))
else:
try:
- make_package_placeholder(workdir, allow_root)
+ make_package_placeholder(workdir, spec.prefix, allow_root)
except Exception as e:
shutil.rmtree(workdir)
shutil.rmtree(tarfile_dir)
@@ -366,7 +379,8 @@ def download_tarball(spec):
def make_package_relative(workdir, prefix, allow_root):
"""
- Change paths in binaries to relative paths
+ Change paths in binaries to relative paths. Change absolute symlinks
+ to relative symlinks.
"""
buildinfo = read_buildinfo_file(workdir)
old_path = buildinfo['buildpath']
@@ -377,9 +391,15 @@ def make_package_relative(workdir, prefix, allow_root):
cur_path_names.append(os.path.join(workdir, filename))
relocate.make_binary_relative(cur_path_names, orig_path_names,
old_path, allow_root)
+ orig_path_names = list()
+ cur_path_names = list()
+ for filename in buildinfo.get('relocate_links', []):
+ orig_path_names.append(os.path.join(prefix, filename))
+ cur_path_names.append(os.path.join(workdir, filename))
+ relocate.make_link_relative(cur_path_names, orig_path_names)
-def make_package_placeholder(workdir, allow_root):
+def make_package_placeholder(workdir, prefix, allow_root):
"""
Change paths in binaries to placeholder paths
"""
@@ -389,6 +409,11 @@ def make_package_placeholder(workdir, allow_root):
cur_path_names.append(os.path.join(workdir, filename))
relocate.make_binary_placeholder(cur_path_names, allow_root)
+ cur_path_names = list()
+ for filename in buildinfo.get('relocate_links', []):
+ cur_path_names.append(os.path.join(workdir, filename))
+ relocate.make_link_placeholder(cur_path_names, workdir, prefix)
+
def relocate_package(workdir, allow_root):
"""
@@ -419,6 +444,11 @@ def relocate_package(workdir, allow_root):
path_names.add(path_name)
relocate.relocate_binary(path_names, old_path, new_path,
allow_root)
+ path_names = set()
+ for filename in buildinfo.get('relocate_links', []):
+ path_name = os.path.join(workdir, filename)
+ path_names.add(path_name)
+ relocate.relocate_links(path_names, old_path, new_path)
def extract_tarball(spec, filename, allow_root=False, unsigned=False,
diff --git a/lib/spack/spack/relocate.py b/lib/spack/spack/relocate.py
index a559ae6495..2d3bc6f837 100644
--- a/lib/spack/spack/relocate.py
+++ b/lib/spack/spack/relocate.py
@@ -351,6 +351,18 @@ def relocate_binary(path_names, old_dir, new_dir, allow_root):
tty.die("Relocation not implemented for %s" % platform.system())
+def make_link_relative(cur_path_names, orig_path_names):
+ """
+ Change absolute links to be relative.
+ """
+ for cur_path, orig_path in zip(cur_path_names, orig_path_names):
+ old_src = os.readlink(orig_path)
+ new_src = os.path.relpath(old_src, orig_path)
+
+ os.unlink(cur_path)
+ os.symlink(new_src, cur_path)
+
+
def make_binary_relative(cur_path_names, orig_path_names, old_dir, allow_root):
"""
Replace old RPATHs with paths relative to old_dir in binary files
@@ -415,6 +427,41 @@ def make_binary_placeholder(cur_path_names, allow_root):
tty.die("Placeholder not implemented for %s" % platform.system())
+def make_link_placeholder(cur_path_names, cur_dir, old_dir):
+ """
+ Replace old install path with placeholder in absolute links.
+
+ Links in ``cur_path_names`` must link to absolute paths.
+ """
+ for cur_path in cur_path_names:
+ placeholder = set_placeholder(spack.store.layout.root)
+ placeholder_prefix = old_dir.replace(spack.store.layout.root,
+ placeholder)
+ cur_src = os.readlink(cur_path)
+ rel_src = os.path.relpath(cur_src, cur_dir)
+ new_src = os.path.join(placeholder_prefix, rel_src)
+
+ os.unlink(cur_path)
+ os.symlink(new_src, cur_path)
+
+
+def relocate_links(path_names, old_dir, new_dir):
+ """
+ Replace old path with new path in link sources.
+
+ Links in ``path_names`` must link to absolute paths or placeholders.
+ """
+ placeholder = set_placeholder(old_dir)
+ for path_name in path_names:
+ old_src = os.readlink(path_name)
+ # replace either placeholder or old_dir
+ new_src = old_src.replace(placeholder, new_dir, 1)
+ new_src = new_src.replace(old_dir, new_dir, 1)
+
+ os.unlink(path_name)
+ os.symlink(new_src, path_name)
+
+
def relocate_text(path_names, old_dir, new_dir):
"""
Replace old path with new path in text file path_name
diff --git a/lib/spack/spack/test/packaging.py b/lib/spack/spack/test/packaging.py
index 096d257a3e..bb086f20c7 100644
--- a/lib/spack/spack/test/packaging.py
+++ b/lib/spack/spack/test/packaging.py
@@ -25,7 +25,7 @@
from spack.util.executable import ProcessError
from spack.relocate import needs_binary_relocation, needs_text_relocation
from spack.relocate import strings_contains_installroot
-from spack.relocate import get_patchelf, relocate_text
+from spack.relocate import get_patchelf, relocate_text, relocate_links
from spack.relocate import substitute_rpath, get_relative_rpaths
from spack.relocate import macho_replace_paths, macho_make_paths_relative
from spack.relocate import modify_macho_object, macho_get_paths
@@ -85,9 +85,12 @@ def test_buildcache(mock_archive, tmpdir):
with open(filename, "w") as script:
script.write(spec.prefix)
+ # Create an absolute symlink
+ linkname = os.path.join(spec.prefix, "link_to_dummy.txt")
+ os.symlink(filename, linkname)
+
# Create the build cache and
# put it directly into the mirror
-
mirror_path = os.path.join(str(tmpdir), 'test-mirror')
spack.mirror.create(
mirror_path, specs=[], no_checksum=True
@@ -100,6 +103,7 @@ def test_buildcache(mock_archive, tmpdir):
stage = spack.stage.Stage(
mirrors['spack-mirror-test'], name="build_cache", keep=True)
stage.create()
+
# setup argument parser
parser = argparse.ArgumentParser()
buildcache.setup_parser(parser)
@@ -120,6 +124,13 @@ def test_buildcache(mock_archive, tmpdir):
args = parser.parse_args(['install', '-f', str(pkghash)])
buildcache.buildcache(parser, args)
+ files = os.listdir(spec.prefix)
+ assert 'link_to_dummy.txt' in files
+ assert 'dummy.txt' in files
+ assert os.path.realpath(
+ os.path.join(spec.prefix, 'link_to_dummy.txt')
+ ) == os.path.realpath(os.path.join(spec.prefix, 'dummy.txt'))
+
# create build cache with relative path and signing
args = parser.parse_args(
['create', '-d', mirror_path, '-f', '-r', str(spec)])
@@ -136,6 +147,13 @@ def test_buildcache(mock_archive, tmpdir):
args = parser.parse_args(['install', '-f', str(pkghash)])
buildcache.buildcache(parser, args)
+ files = os.listdir(spec.prefix)
+ assert 'link_to_dummy.txt' in files
+ assert 'dummy.txt' in files
+ assert os.path.realpath(
+ os.path.join(spec.prefix, 'link_to_dummy.txt')
+ ) == os.path.realpath(os.path.join(spec.prefix, 'dummy.txt'))
+
else:
# create build cache without signing
args = parser.parse_args(
@@ -149,6 +167,13 @@ def test_buildcache(mock_archive, tmpdir):
args = parser.parse_args(['install', '-u', str(spec)])
buildcache.install_tarball(spec, args)
+ files = os.listdir(spec.prefix)
+ assert 'link_to_dummy.txt' in files
+ assert 'dummy.txt' in files
+ assert os.path.realpath(
+ os.path.join(spec.prefix, 'link_to_dummy.txt')
+ ) == os.path.realpath(os.path.join(spec.prefix, 'dummy.txt'))
+
# test overwrite install without verification
args = parser.parse_args(['install', '-f', '-u', str(pkghash)])
buildcache.buildcache(parser, args)
@@ -169,9 +194,17 @@ def test_buildcache(mock_archive, tmpdir):
args = parser.parse_args(['install', '-f', '-u', str(pkghash)])
buildcache.buildcache(parser, args)
+ files = os.listdir(spec.prefix)
+ assert 'link_to_dummy.txt' in files
+ assert 'dummy.txt' in files
+ assert os.path.realpath(
+ os.path.join(spec.prefix, 'link_to_dummy.txt')
+ ) == os.path.realpath(os.path.join(spec.prefix, 'dummy.txt'))
+
# Validate the relocation information
buildinfo = bindist.read_buildinfo_file(spec.prefix)
assert(buildinfo['relocate_textfiles'] == ['dummy.txt'])
+ assert(buildinfo['relocate_links'] == ['link_to_dummy.txt'])
args = parser.parse_args(['list'])
buildcache.buildcache(parser, args)
@@ -219,6 +252,18 @@ def test_relocate_text(tmpdir):
assert(strings_contains_installroot(filename, old_dir) is False)
+def test_relocate_links(tmpdir):
+ with tmpdir.as_cwd():
+ old_dir = '/home/spack/opt/spack'
+ filename = 'link.ln'
+ old_src = os.path.join(old_dir, filename)
+ os.symlink(old_src, filename)
+ filenames = [filename]
+ new_dir = '/opt/rh/devtoolset/'
+ relocate_links(filenames, old_dir, new_dir)
+ assert os.path.realpath(filename) == os.path.join(new_dir, filename)
+
+
def test_needs_relocation():
binary_type = (
'ELF 64-bit LSB executable, x86-64, version 1 (SYSV),'
--
GitLab