From 1484a94b1e7239d0f8c5350640b428bb520cf301 Mon Sep 17 00:00:00 2001 From: Patrick Gartung Date: Thu, 9 Nov 2017 12:53:34 -0600 Subject: spack buildcache: symlink and relative RPATH fixes (#6140) * Skip rewriting files that are links (this also avoids issues with parsing the output of the 'file' command for symlinks) * Fail rather than warn for several gpg signing issues (e.g. if there is no key available to sign with) * Installation with 'buildcache install' only retrieves link and run dependencies (this matches 'buildcache create' which only creates tarballs of link/run dependencies) * Don't rewrite RPATHs for a binary if the binary cached package was created with relative RPATHs * Refactor relocate_binary to operate on multiple binaries (given as a collection of paths). This was likewise done for relocate_text and make_binary_relative --- lib/spack/spack/binary_distribution.py | 58 +++++++++------- lib/spack/spack/cmd/buildcache.py | 18 ++--- lib/spack/spack/relocate.py | 123 ++++++++++++++++++--------------- lib/spack/spack/test/packaging.py | 60 +++++++++++----- 4 files changed, 153 insertions(+), 106 deletions(-) (limited to 'lib') diff --git a/lib/spack/spack/binary_distribution.py b/lib/spack/spack/binary_distribution.py index 8b812db687..3198e7b4ee 100644 --- a/lib/spack/spack/binary_distribution.py +++ b/lib/spack/spack/binary_distribution.py @@ -95,7 +95,7 @@ def read_buildinfo_file(prefix): return buildinfo -def write_buildinfo_file(prefix): +def write_buildinfo_file(prefix, rel=False): """ Create a cache file containing information required for the relocation @@ -119,6 +119,7 @@ def write_buildinfo_file(prefix): # Create buildinfo data and write it to disk buildinfo = {} + buildinfo['relative_rpaths'] = rel buildinfo['buildpath'] = spack.store.layout.root buildinfo['relocate_textfiles'] = text_to_relocate buildinfo['relocate_binaries'] = binary_to_relocate @@ -132,7 +133,7 @@ def tarball_directory_name(spec): Return name of the tarball directory according to the convention -//-/ """ - return "%s/%s/%s-%s" % (spack.architecture.sys_type(), + return "%s/%s/%s-%s" % (spec.architecture, str(spec.compiler).replace("@", "-"), spec.name, spec.version) @@ -142,7 +143,7 @@ def tarball_name(spec, ext): Return the name of the tarfile according to the convention --- """ - return "%s-%s-%s-%s-%s%s" % (spack.architecture.sys_type(), + return "%s-%s-%s-%s-%s%s" % (spec.architecture, str(spec.compiler).replace("@", "-"), spec.name, spec.version, @@ -246,7 +247,7 @@ def build_tarball(spec, outdir, force=False, rel=False, yes_to_all=False, install_tree(spec.prefix, workdir, symlinks=True) # create info for later relocation and create tar - write_buildinfo_file(workdir) + write_buildinfo_file(workdir, rel=rel) # optinally make the paths in the binaries relative to each other # in the spack install tree before creating tarball @@ -333,10 +334,13 @@ def make_package_relative(workdir, prefix): """ buildinfo = read_buildinfo_file(workdir) old_path = buildinfo['buildpath'] + orig_path_names = list() + cur_path_names = list() for filename in buildinfo['relocate_binaries']: - orig_path_name = os.path.join(prefix, filename) - cur_path_name = os.path.join(workdir, filename) - relocate.make_binary_relative(cur_path_name, orig_path_name, old_path) + orig_path_names.append(os.path.join(prefix, filename)) + cur_path_names.append(os.path.join(workdir, filename)) + relocate.make_binary_relative(cur_path_names, orig_path_names, + old_path) def relocate_package(prefix): @@ -346,18 +350,25 @@ def relocate_package(prefix): buildinfo = read_buildinfo_file(prefix) new_path = spack.store.layout.root old_path = buildinfo['buildpath'] - if new_path == old_path: + rel = buildinfo.get('relative_rpaths', False) + if new_path == old_path and not rel: return tty.msg("Relocating package from", "%s to %s." % (old_path, new_path)) - for filename in buildinfo['relocate_binaries']: - path_name = os.path.join(prefix, filename) - relocate.relocate_binary(path_name, old_path, new_path) - + path_names = set() for filename in buildinfo['relocate_textfiles']: path_name = os.path.join(prefix, filename) - relocate.relocate_text(path_name, old_path, new_path) + path_names.add(path_name) + relocate.relocate_text(path_names, old_path, new_path) + # If the binary files in the package were not edited to use + # relative RPATHs, then the RPATHs need to be relocated + if not rel: + path_names = set() + for filename in buildinfo['relocate_binaries']: + path_name = os.path.join(prefix, filename) + path_names.add(path_name) + relocate.relocate_binary(path_names, old_path, new_path) def extract_tarball(spec, filename, yes_to_all=False, force=False): @@ -391,17 +402,16 @@ def extract_tarball(spec, filename, yes_to_all=False, force=False): # get the sha256 checksum of the tarball checksum = checksum_tarball(tarfile_path) - if not yes_to_all: - # get the sha256 checksum recorded at creation - spec_dict = {} - with open(specfile_path, 'r') as inputfile: - content = inputfile.read() - spec_dict = yaml.load(content) - bchecksum = spec_dict['binary_cache_checksum'] - - # if the checksums don't match don't install - if bchecksum['hash'] != checksum: - raise NoChecksumException() + # get the sha256 checksum recorded at creation + spec_dict = {} + with open(specfile_path, 'r') as inputfile: + content = inputfile.read() + spec_dict = yaml.load(content) + bchecksum = spec_dict['binary_cache_checksum'] + + # if the checksums don't match don't install + if bchecksum['hash'] != checksum: + raise NoChecksumException() # delay creating installpath until verification is complete mkdirp(installpath) diff --git a/lib/spack/spack/cmd/buildcache.py b/lib/spack/spack/cmd/buildcache.py index 64ac8e4d42..653c59074a 100644 --- a/lib/spack/spack/cmd/buildcache.py +++ b/lib/spack/spack/cmd/buildcache.py @@ -227,16 +227,16 @@ def createtarball(args): except NoOverwriteException as e: tty.warn("%s exists, use -f to force overwrite." % e) except NoGpgException: - tty.warn("gpg2 is not available," - " use -y to create unsigned build caches") + tty.die("gpg2 is not available," + " use -y to create unsigned build caches") except NoKeyException: - tty.warn("no default key available for signing," - " use -y to create unsigned build caches" - " or spack gpg init to create a default key") + tty.die("no default key available for signing," + " use -y to create unsigned build caches" + " or spack gpg init to create a default key") except PickKeyException: - tty.warn("multi keys available for signing," - " use -y to create unsigned build caches" - " or -k to pick a key") + tty.die("multi keys available for signing," + " use -y to create unsigned build caches" + " or -k to pick a key") def installtarball(args): @@ -267,7 +267,7 @@ def install_tarball(spec, args): force = False if args.force: force = True - for d in s.dependencies(): + for d in s.dependencies(deptype=('link', 'run')): tty.msg("Installing buildcache for dependency spec %s" % d) install_tarball(d, args) package = spack.repo.get(spec) diff --git a/lib/spack/spack/relocate.py b/lib/spack/spack/relocate.py index b742db2d6b..9341b55c86 100644 --- a/lib/spack/spack/relocate.py +++ b/lib/spack/spack/relocate.py @@ -91,7 +91,7 @@ def macho_get_paths(path_name): otool = Executable('otool') output = otool("-l", path_name, output=str, err=str) last_cmd = None - idpath = '' + idpath = None rpaths = [] deps = [] for line in output.split('\n'): @@ -119,24 +119,24 @@ def macho_make_paths_relative(path_name, old_dir, rpaths, deps, idpath): in rpaths and deps; idpaths are replaced with @rpath/basebane(path_name); replacement are returned. """ - id = None - nrpaths = [] - ndeps = [] + new_idpath = None if idpath: - id = '@rpath/%s' % os.path.basename(idpath) + new_idpath = '@rpath/%s' % os.path.basename(idpath) + new_rpaths = list() + new_deps = list() for rpath in rpaths: if re.match(old_dir, rpath): rel = os.path.relpath(rpath, start=os.path.dirname(path_name)) - nrpaths.append('@loader_path/%s' % rel) + new_rpaths.append('@loader_path/%s' % rel) else: - nrpaths.append(rpath) + new_rpaths.append(rpath) for dep in deps: if re.match(old_dir, dep): rel = os.path.relpath(dep, start=os.path.dirname(path_name)) - ndeps.append('@loader_path/%s' % rel) + new_deps.append('@loader_path/%s' % rel) else: - ndeps.append(dep) - return nrpaths, ndeps, id + new_deps.append(dep) + return (new_rpaths, new_deps, new_idpath) def macho_replace_paths(old_dir, new_dir, rpaths, deps, idpath): @@ -144,22 +144,22 @@ def macho_replace_paths(old_dir, new_dir, rpaths, deps, idpath): Replace old_dir with new_dir in rpaths, deps and idpath and return replacements """ - id = None - nrpaths = [] - ndeps = [] + new_idpath = None if idpath: - id = idpath.replace(old_dir, new_dir) + new_idpath = idpath.replace(old_dir, new_dir) + new_rpaths = list() + new_deps = list() for rpath in rpaths: - nrpath = rpath.replace(old_dir, new_dir) - nrpaths.append(nrpath) + new_rpath = rpath.replace(old_dir, new_dir) + new_rpaths.append(new_rpath) for dep in deps: - ndep = dep.replace(old_dir, new_dir) - ndeps.append(ndep) - return nrpaths, ndeps, id + new_dep = dep.replace(old_dir, new_dir) + new_deps.append(new_dep) + return new_rpaths, new_deps, new_idpath -def modify_macho_object(cur_path_name, orig_path_name, old_dir, new_dir, - relative): +def modify_macho_object(cur_path, rpaths, deps, idpath, + new_rpaths, new_deps, new_idpath): """ Modify MachO binary path_name by replacing old_dir with new_dir or the relative path to spack install root. @@ -171,28 +171,18 @@ def modify_macho_object(cur_path_name, orig_path_name, old_dir, new_dir, install_name_tool -rpath old new binary """ # avoid error message for libgcc_s - if 'libgcc_' in cur_path_name: + if 'libgcc_' in cur_path: return - rpaths, deps, idpath = macho_get_paths(cur_path_name) - id = None - nrpaths = [] - ndeps = [] - if relative: - nrpaths, ndeps, id = macho_make_paths_relative(orig_path_name, - old_dir, rpaths, - deps, idpath) - else: - nrpaths, ndeps, id = macho_replace_paths(old_dir, new_dir, rpaths, - deps, idpath) install_name_tool = Executable('install_name_tool') - if id: - install_name_tool('-id', id, cur_path_name, output=str, err=str) + if new_idpath: + install_name_tool('-id', new_idpath, str(cur_path), + output=str, err=str) - for orig, new in zip(deps, ndeps): - install_name_tool('-change', orig, new, cur_path_name) + for orig, new in zip(deps, new_deps): + install_name_tool('-change', orig, new, str(cur_path)) - for orig, new in zip(rpaths, nrpaths): - install_name_tool('-rpath', orig, new, cur_path_name) + for orig, new in zip(rpaths, new_rpaths): + install_name_tool('-rpath', orig, new, str(cur_path)) return @@ -227,6 +217,8 @@ def needs_binary_relocation(filetype): retval = False if "relocatable" in filetype: return False + if "link" in filetype: + return False if platform.system() == 'Darwin': return ('Mach-O' in filetype) elif platform.system() == 'Linux': @@ -240,44 +232,65 @@ def needs_text_relocation(filetype): """ Check whether the given filetype is text that may need relocation. """ + if "link" in filetype: + return False return ("text" in filetype) -def relocate_binary(path_name, old_dir, new_dir): +def relocate_binary(path_names, old_dir, new_dir): """ - Change old_dir to new_dir in RPATHs of elf or mach-o file path_name + Change old_dir to new_dir in RPATHs of elf or mach-o files """ if platform.system() == 'Darwin': - modify_macho_object(path_name, old_dir, new_dir, relative=False) + for path_name in path_names: + rpaths, deps, idpath = macho_get_paths(path_name) + new_rpaths, new_deps, new_idpath = macho_replace_paths(old_dir, + new_dir, + rpaths, + deps, + idpath) + modify_macho_object(path_name, + rpaths, deps, idpath, + new_rpaths, new_deps, new_idpath) elif platform.system() == 'Linux': - orig_rpaths = get_existing_elf_rpaths(path_name) - new_rpaths = substitute_rpath(orig_rpaths, old_dir, new_dir) - modify_elf_object(path_name, orig_rpaths, new_rpaths) + for path_name in path_names: + orig_rpaths = get_existing_elf_rpaths(path_name) + new_rpaths = substitute_rpath(orig_rpaths, old_dir, new_dir) + modify_elf_object(path_name, orig_rpaths, new_rpaths) else: tty.die("Relocation not implemented for %s" % platform.system()) -def make_binary_relative(cur_path_name, orig_path_name, old_dir): +def make_binary_relative(cur_path_names, orig_path_names, old_dir): """ - Make RPATHs relative to old_dir in given elf or mach-o file path_name + Make RPATHs relative to old_dir in given elf or mach-o files """ if platform.system() == 'Darwin': - new_dir = '' - modify_macho_object(cur_path_name, orig_path_name, old_dir, new_dir, - relative=True) + for cur_path, orig_path in zip(cur_path_names, orig_path_names): + rpaths, deps, idpath = macho_get_paths(cur_path) + (new_rpaths, + new_deps, + new_idpath) = macho_make_paths_relative(orig_path, old_dir, + rpaths, deps, idpath) + modify_macho_object(cur_path, + rpaths, deps, idpath, + new_rpaths, new_deps, new_idpath) elif platform.system() == 'Linux': - orig_rpaths = get_existing_elf_rpaths(cur_path_name) - new_rpaths = get_relative_rpaths(orig_path_name, old_dir, orig_rpaths) - modify_elf_object(cur_path_name, orig_rpaths, new_rpaths) + for cur_path, orig_path in zip(cur_path_names, orig_path_names): + orig_rpaths = get_existing_elf_rpaths(cur_path) + new_rpaths = get_relative_rpaths(orig_path, old_dir, + orig_rpaths) + modify_elf_object(cur_path, orig_rpaths, new_rpaths) else: tty.die("Prelocation not implemented for %s" % platform.system()) -def relocate_text(path_name, old_dir, new_dir): +def relocate_text(path_names, old_dir, new_dir): """ Replace old path with new path in text file path_name """ - filter_file("r'%s'" % old_dir, "r'%s'" % new_dir, path_name) + filter_file("r'%s'" % old_dir, "r'%s'" % new_dir, + *path_names, backup=False) def substitute_rpath(orig_rpath, topdir, new_root_path): diff --git a/lib/spack/spack/test/packaging.py b/lib/spack/spack/test/packaging.py index 65cbdeaecc..f9c77c530f 100644 --- a/lib/spack/spack/test/packaging.py +++ b/lib/spack/spack/test/packaging.py @@ -41,7 +41,8 @@ import spack.cmd.buildcache as buildcache from spack.spec import Spec from spack.fetch_strategy import URLFetchStrategy, FetchStrategyComposite from spack.util.executable import ProcessError -from spack.relocate import needs_binary_relocation, get_patchelf +from spack.relocate import needs_binary_relocation, needs_text_relocation +from spack.relocate import get_patchelf 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 @@ -218,6 +219,8 @@ echo $PATH""" def test_relocate(): assert (needs_binary_relocation('relocatable') is False) + assert (needs_binary_relocation('link') is False) + assert (needs_text_relocation('link') is False) out = macho_make_paths_relative('/Users/Shares/spack/pkgC/lib/libC.dylib', '/Users/Shared/spack', @@ -225,8 +228,8 @@ def test_relocate(): '/Users/Shared/spack/pkgB/lib', '/usr/local/lib'), ('/Users/Shared/spack/pkgA/libA.dylib', - '/Users/Shared/spack/pkgB/libB.dylib', - '/usr/local/lib/libloco.dylib'), + '/Users/Shared/spack/pkgB/libB.dylib', + '/usr/local/lib/libloco.dylib'), '/Users/Shared/spack/pkgC/lib/libC.dylib') assert out == (['@loader_path/../../../../Shared/spack/pkgA/lib', '@loader_path/../../../../Shared/spack/pkgB/lib', @@ -242,8 +245,8 @@ def test_relocate(): '/Users/Shared/spack/pkgB/lib', '/usr/local/lib'), ('/Users/Shared/spack/pkgA/libA.dylib', - '/Users/Shared/spack/pkgB/libB.dylib', - '/usr/local/lib/libloco.dylib'), None) + '/Users/Shared/spack/pkgB/libB.dylib', + '/usr/local/lib/libloco.dylib'), None) assert out == (['@loader_path/../../pkgA/lib', '@loader_path/../../pkgB/lib', @@ -258,8 +261,8 @@ def test_relocate(): '/Users/Shared/spack/pkgB/lib', '/usr/local/lib'), ('/Users/Shared/spack/pkgA/libA.dylib', - '/Users/Shared/spack/pkgB/libB.dylib', - '/usr/local/lib/libloco.dylib'), + '/Users/Shared/spack/pkgB/libB.dylib', + '/usr/local/lib/libloco.dylib'), '/Users/Shared/spack/pkgC/lib/libC.dylib') assert out == (['/Applications/spack/pkgA/lib', '/Applications/spack/pkgB/lib', @@ -275,8 +278,8 @@ def test_relocate(): '/Users/Shared/spack/pkgB/lib', '/usr/local/lib'), ('/Users/Shared/spack/pkgA/libA.dylib', - '/Users/Shared/spack/pkgB/libB.dylib', - '/usr/local/lib/libloco.dylib'), + '/Users/Shared/spack/pkgB/libB.dylib', + '/usr/local/lib/libloco.dylib'), None) assert out == (['/Applications/spack/pkgA/lib', '/Applications/spack/pkgB/lib', @@ -301,25 +304,46 @@ def test_relocate(): def test_relocate_macho(tmpdir): with tmpdir.as_cwd(): get_patchelf() - assert (needs_binary_relocation('Mach-O') is True) + assert (needs_binary_relocation('Mach-O')) - macho_get_paths('/bin/bash') + rpaths, deps, idpath = macho_get_paths('/bin/bash') + nrpaths, ndeps, nid = macho_make_paths_relative('/bin/bash', '/usr', + rpaths, deps, idpath) shutil.copyfile('/bin/bash', 'bash') + modify_macho_object('bash', + rpaths, deps, idpath, + nrpaths, ndeps, nid) - modify_macho_object('bash', '/bin/bash', '/usr', '/opt', False) - modify_macho_object('bash', '/bin/bash', '/usr', '/opt', True) + rpaths, deps, idpath = macho_get_paths('/bin/bash') + nrpaths, ndeps, nid = macho_replace_paths('/usr', '/opt', + rpaths, deps, idpath) + shutil.copyfile('/bin/bash', 'bash') + modify_macho_object('bash', + rpaths, deps, idpath, + nrpaths, ndeps, nid) + + path = '/usr/lib/libncurses.5.4.dylib' + rpaths, deps, idpath = macho_get_paths(path) + nrpaths, ndeps, nid = macho_make_paths_relative(path, '/usr', + rpaths, deps, idpath) + shutil.copyfile( + '/usr/lib/libncurses.5.4.dylib', 'libncurses.5.4.dylib') + modify_macho_object('libncurses.5.4.dylib', + rpaths, deps, idpath, + nrpaths, ndeps, nid) + rpaths, deps, idpath = macho_get_paths(path) + nrpaths, ndeps, nid = macho_replace_paths('/usr', '/opt', + rpaths, deps, idpath) shutil.copyfile( '/usr/lib/libncurses.5.4.dylib', 'libncurses.5.4.dylib') modify_macho_object( 'libncurses.5.4.dylib', - '/usr/lib/libncurses.5.4.dylib', '/usr', '/opt', False) - modify_macho_object( - 'libncurses.5.4.dylib', - '/usr/lib/libncurses.5.4.dylib', '/usr', '/opt', True) + rpaths, deps, idpath, + nrpaths, ndeps, nid) @pytest.mark.skipif(sys.platform != 'linux2', reason="only works with Elf objects") def test_relocate_elf(): - assert (needs_binary_relocation('ELF') is True) + assert (needs_binary_relocation('ELF')) -- cgit v1.2.3-70-g09d2