From c63c4a048c796c900f90728c6ea11d5972cabd95 Mon Sep 17 00:00:00 2001 From: Greg Becker Date: Fri, 11 Jan 2019 14:52:01 -0800 Subject: 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(-) (limited to 'lib') 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.fetch_strategy import URLFetchStrategy, FetchStrategyComposite 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 @@ echo $PATH""" 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 @@ echo $PATH""" 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 @@ echo $PATH""" 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 @@ echo $PATH""" 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 @@ echo $PATH""" 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 @@ echo $PATH""" 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),' -- cgit v1.2.3-60-g2f50