This fixes a problem where the placeholder path was not in the first rpath entry.

* Rework of buildcache creation and install prefix checking using the functions introduced in
https://github.com/spack/spack/pull/9199

Instead of replacing rpaths with placeholder and then checking strings, make use of the functions
relocate.is_recocatable and relocate.is_file_relocatable to decide if a package needs the allow-root option.

This fixes a problem where the placeholder path was not in the first rpath entry. This was seen in c++ libraries and binaries because the compiler was outside the spack install base path and always appears first in the rpath.

Instead of checking the first rpath entry, all rpaths have the placeholder path and the old install path (if it exists) replaced with the new install path.

* flake8
This commit is contained in:
Patrick Gartung 2019-03-01 07:47:26 -06:00 committed by GitHub
parent 433cc4a972
commit 1d4289afdd
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 73 additions and 102 deletions

View File

@ -7,7 +7,6 @@
import re import re
import tarfile import tarfile
import shutil import shutil
import platform
import tempfile import tempfile
import hashlib import hashlib
from contextlib import closing from contextlib import closing
@ -17,7 +16,7 @@
from six.moves.urllib.error import URLError from six.moves.urllib.error import URLError
import llnl.util.tty as tty import llnl.util.tty as tty
from llnl.util.filesystem import mkdirp, install_tree, get_filetype from llnl.util.filesystem import mkdirp, install_tree
import spack.cmd import spack.cmd
import spack.fetch_strategy as fs import spack.fetch_strategy as fs
@ -38,6 +37,7 @@ class NoOverwriteException(Exception):
""" """
Raised when a file exists and must be overwritten. Raised when a file exists and must be overwritten.
""" """
def __init__(self, file_path): def __init__(self, file_path):
err_msg = "\n%s\nexists\n" % file_path err_msg = "\n%s\nexists\n" % file_path
err_msg += "Use -f option to overwrite." err_msg += "Use -f option to overwrite."
@ -62,6 +62,7 @@ class PickKeyException(spack.error.SpackError):
""" """
Raised when multiple keys can be used to sign. Raised when multiple keys can be used to sign.
""" """
def __init__(self, keys): def __init__(self, keys):
err_msg = "Multi keys available for signing\n%s\n" % keys err_msg = "Multi keys available for signing\n%s\n" % keys
err_msg += "Use spack buildcache create -k <key hash> to pick a key." err_msg += "Use spack buildcache create -k <key hash> to pick a key."
@ -133,13 +134,13 @@ def write_buildinfo_file(prefix, workdir, rel=False):
binary_to_relocate = [] binary_to_relocate = []
link_to_relocate = [] link_to_relocate = []
blacklist = (".spack", "man") blacklist = (".spack", "man")
os_id = platform.system()
# Do this at during tarball creation to save time when tarball unpacked. # Do this at during tarball creation to save time when tarball unpacked.
# Used by make_package_relative to determine binaries to change. # Used by make_package_relative to determine binaries to change.
for root, dirs, files in os.walk(prefix, topdown=True): for root, dirs, files in os.walk(prefix, topdown=True):
dirs[:] = [d for d in dirs if d not in blacklist] dirs[:] = [d for d in dirs if d not in blacklist]
for filename in files: for filename in files:
path_name = os.path.join(root, filename) path_name = os.path.join(root, filename)
m_type, m_subtype = relocate.mime_type(path_name)
if os.path.islink(path_name): if os.path.islink(path_name):
link = os.readlink(path_name) link = os.readlink(path_name)
if os.path.isabs(link): if os.path.isabs(link):
@ -157,12 +158,12 @@ def write_buildinfo_file(prefix, workdir, rel=False):
# This cuts down on the number of files added to the list # This cuts down on the number of files added to the list
# of files potentially needing relocation # of files potentially needing relocation
elif relocate.strings_contains_installroot( elif relocate.strings_contains_installroot(
path_name, spack.store.layout.root): path_name,
filetype = get_filetype(path_name) spack.store.layout.root):
if relocate.needs_binary_relocation(filetype, os_id): if relocate.needs_binary_relocation(m_type, m_subtype):
rel_path_name = os.path.relpath(path_name, prefix) rel_path_name = os.path.relpath(path_name, prefix)
binary_to_relocate.append(rel_path_name) binary_to_relocate.append(rel_path_name)
elif relocate.needs_text_relocation(filetype): elif relocate.needs_text_relocation(m_type, m_subtype):
rel_path_name = os.path.relpath(path_name, prefix) rel_path_name = os.path.relpath(path_name, prefix)
text_to_relocate.append(rel_path_name) text_to_relocate.append(rel_path_name)
@ -479,8 +480,7 @@ def relocate_package(workdir, allow_root):
for filename in buildinfo['relocate_binaries']: for filename in buildinfo['relocate_binaries']:
path_name = os.path.join(workdir, filename) path_name = os.path.join(workdir, filename)
path_names.add(path_name) path_names.add(path_name)
relocate.relocate_binary(path_names, old_path, new_path, relocate.relocate_binary(path_names, old_path, new_path, allow_root)
allow_root)
path_names = set() path_names = set()
for filename in buildinfo.get('relocate_links', []): for filename in buildinfo.get('relocate_links', []):
path_name = os.path.join(workdir, filename) path_name = os.path.join(workdir, filename)

View File

@ -36,12 +36,16 @@ def get_patchelf():
# as we may need patchelf, find out where it is # as we may need patchelf, find out where it is
if platform.system() == 'Darwin': if platform.system() == 'Darwin':
return None return None
patchelf = spack.util.executable.which('patchelf')
if patchelf is None:
patchelf_spec = spack.cmd.parse_specs("patchelf", concretize=True)[0] patchelf_spec = spack.cmd.parse_specs("patchelf", concretize=True)[0]
patchelf = spack.repo.get(patchelf_spec) patchelf = spack.repo.get(patchelf_spec)
if not patchelf.installed: if not patchelf.installed:
patchelf.do_install() patchelf.do_install()
patchelf_executable = os.path.join(patchelf.prefix.bin, "patchelf") patchelf_executable = os.path.join(patchelf.prefix.bin, "patchelf")
return patchelf_executable return patchelf_executable
else:
return patchelf.path
def get_existing_elf_rpaths(path_name): def get_existing_elf_rpaths(path_name):
@ -266,31 +270,22 @@ def modify_elf_object(path_name, new_rpaths):
tty.die('relocation not supported for this platform') tty.die('relocation not supported for this platform')
def needs_binary_relocation(filetype, os_id=None): def needs_binary_relocation(m_type, m_subtype):
""" """
Check whether the given filetype is a binary that may need relocation. Check whether the given filetype is a binary that may need relocation.
""" """
retval = False if m_type == 'application':
if "relocatable" in filetype: if (m_subtype == 'x-executable' or m_subtype == 'x-sharedlib' or
m_subtype == 'x-mach-binary'):
return True
return False return False
if "link to" in filetype:
return False
if os_id == 'Darwin':
return ("Mach-O" in filetype)
elif os_id == 'Linux':
return ("ELF" in filetype)
else:
tty.die("Relocation not implemented for %s" % os_id)
return retval
def needs_text_relocation(filetype): def needs_text_relocation(m_type, m_subtype):
""" """
Check whether the given filetype is text that may need relocation. Check whether the given filetype is text that may need relocation.
""" """
if "link to" in filetype: return (m_type == "text")
return False
return ("text" in filetype)
def relocate_binary(path_names, old_dir, new_dir, allow_root): def relocate_binary(path_names, old_dir, new_dir, allow_root):
@ -302,51 +297,44 @@ def relocate_binary(path_names, old_dir, new_dir, allow_root):
if platform.system() == 'Darwin': if platform.system() == 'Darwin':
for path_name in path_names: for path_name in path_names:
(rpaths, deps, idpath) = macho_get_paths(path_name) (rpaths, deps, idpath) = macho_get_paths(path_name)
# new style buildaches with placeholder in binaries # one pass to replace placeholder
if (deps[0].startswith(placeholder) or (n_rpaths,
rpaths[0].startswith(placeholder) or n_deps,
(idpath and idpath.startswith(placeholder))): n_idpath) = macho_replace_paths(placeholder,
(new_rpaths,
new_deps,
new_idpath) = macho_replace_paths(placeholder,
new_dir, new_dir,
rpaths, rpaths,
deps, deps,
idpath) idpath)
# old style buildcaches with original install root in binaries # another pass to replace old_dir
else:
(new_rpaths, (new_rpaths,
new_deps, new_deps,
new_idpath) = macho_replace_paths(old_dir, new_idpath) = macho_replace_paths(old_dir,
new_dir, new_dir,
rpaths, n_rpaths,
deps, n_deps,
idpath) n_idpath)
modify_macho_object(path_name, modify_macho_object(path_name,
rpaths, deps, idpath, rpaths, deps, idpath,
new_rpaths, new_deps, new_idpath) new_rpaths, new_deps, new_idpath)
if (not allow_root and if (not allow_root and
old_dir != new_dir and old_dir != new_dir and
strings_contains_installroot(path_name, old_dir)): not file_is_relocatable(path_name)):
raise InstallRootStringException(path_name, old_dir) raise InstallRootStringException(path_name, old_dir)
elif platform.system() == 'Linux': elif platform.system() == 'Linux':
for path_name in path_names: for path_name in path_names:
orig_rpaths = get_existing_elf_rpaths(path_name) orig_rpaths = get_existing_elf_rpaths(path_name)
if orig_rpaths: if orig_rpaths:
if orig_rpaths[0].startswith(placeholder): # one pass to replace placeholder
# new style buildaches with placeholder in binaries n_rpaths = substitute_rpath(orig_rpaths,
new_rpaths = substitute_rpath(orig_rpaths,
placeholder, new_dir) placeholder, new_dir)
else: # one pass to replace old_dir
# old style buildcaches with original install new_rpaths = substitute_rpath(n_rpaths,
# root in binaries
new_rpaths = substitute_rpath(orig_rpaths,
old_dir, new_dir) old_dir, new_dir)
modify_elf_object(path_name, new_rpaths) modify_elf_object(path_name, new_rpaths)
if (not allow_root and if (not allow_root and
old_dir != new_dir and old_dir != new_dir and
strings_contains_installroot(path_name, old_dir)): not file_is_relocatable(path_name)):
raise InstallRootStringException(path_name, old_dir) raise InstallRootStringException(path_name, old_dir)
else: else:
tty.die("Relocation not implemented for %s" % platform.system()) tty.die("Relocation not implemented for %s" % platform.system())
@ -379,8 +367,8 @@ def make_binary_relative(cur_path_names, orig_path_names, old_dir, allow_root):
rpaths, deps, idpath, rpaths, deps, idpath,
new_rpaths, new_deps, new_idpath) new_rpaths, new_deps, new_idpath)
if (not allow_root and if (not allow_root and
strings_contains_installroot(cur_path)): not file_is_relocatable(cur_path, old_dir)):
raise InstallRootStringException(cur_path) raise InstallRootStringException(cur_path, old_dir)
elif platform.system() == 'Linux': elif platform.system() == 'Linux':
for cur_path, orig_path in zip(cur_path_names, orig_path_names): for cur_path, orig_path in zip(cur_path_names, orig_path_names):
orig_rpaths = get_existing_elf_rpaths(cur_path) orig_rpaths = get_existing_elf_rpaths(cur_path)
@ -389,7 +377,7 @@ def make_binary_relative(cur_path_names, orig_path_names, old_dir, allow_root):
orig_rpaths) orig_rpaths)
modify_elf_object(cur_path, new_rpaths) modify_elf_object(cur_path, new_rpaths)
if (not allow_root and if (not allow_root and
strings_contains_installroot(cur_path, old_dir)): not file_is_relocatable(cur_path, old_dir)):
raise InstallRootStringException(cur_path, old_dir) raise InstallRootStringException(cur_path, old_dir)
else: else:
tty.die("Prelocation not implemented for %s" % platform.system()) tty.die("Prelocation not implemented for %s" % platform.system())
@ -401,27 +389,14 @@ def make_binary_placeholder(cur_path_names, allow_root):
""" """
if platform.system() == 'Darwin': if platform.system() == 'Darwin':
for cur_path in cur_path_names: for cur_path in cur_path_names:
rpaths, deps, idpath = macho_get_paths(cur_path)
(new_rpaths,
new_deps,
new_idpath) = macho_make_paths_placeholder(rpaths, deps, idpath)
modify_macho_object(cur_path,
rpaths, deps, idpath,
new_rpaths, new_deps, new_idpath)
if (not allow_root and if (not allow_root and
strings_contains_installroot(cur_path, not file_is_relocatable(cur_path)):
spack.store.layout.root)):
raise InstallRootStringException( raise InstallRootStringException(
cur_path, spack.store.layout.root) cur_path, spack.store.layout.root)
elif platform.system() == 'Linux': elif platform.system() == 'Linux':
for cur_path in cur_path_names: for cur_path in cur_path_names:
orig_rpaths = get_existing_elf_rpaths(cur_path)
if orig_rpaths:
new_rpaths = get_placeholder_rpaths(cur_path, orig_rpaths)
modify_elf_object(cur_path, new_rpaths)
if (not allow_root and if (not allow_root and
strings_contains_installroot( not file_is_relocatable(cur_path)):
cur_path, spack.store.layout.root)):
raise InstallRootStringException( raise InstallRootStringException(
cur_path, spack.store.layout.root) cur_path, spack.store.layout.root)
else: else:
@ -498,6 +473,8 @@ def is_relocatable(spec):
raise ValueError('spec is not installed [{0}]'.format(str(spec))) raise ValueError('spec is not installed [{0}]'.format(str(spec)))
if spec.external or spec.virtual: if spec.external or spec.virtual:
tty.warn('external or virtual package %s is not relocatable' %
spec.name)
return False return False
# Explore the installation prefix of the spec # Explore the installation prefix of the spec
@ -538,7 +515,7 @@ def file_is_relocatable(file):
raise ValueError('{0} is not an absolute path'.format(file)) raise ValueError('{0} is not an absolute path'.format(file))
strings = Executable('strings') strings = Executable('strings')
patchelf = Executable('patchelf') patchelf = Executable(get_patchelf())
# Remove the RPATHS from the strings in the executable # Remove the RPATHS from the strings in the executable
set_of_strings = set(strings(file, output=str).split()) set_of_strings = set(strings(file, output=str).split())
@ -600,6 +577,6 @@ def mime_type(file):
Tuple containing the MIME type and subtype Tuple containing the MIME type and subtype
""" """
file_cmd = Executable('file') file_cmd = Executable('file')
output = file_cmd('-b', '--mime-type', file, output=str, error=str) output = file_cmd('-b', '-h', '--mime-type', file, output=str, error=str)
tty.debug('[MIME_TYPE] {0} -> {1}'.format(file, output.strip())) tty.debug('[MIME_TYPE] {0} -> {1}'.format(file, output.strip()))
return tuple(output.strip().split('/')) return tuple(output.strip().split('/'))

View File

@ -157,7 +157,7 @@ def test_buildcache(mock_archive, tmpdir):
else: else:
# create build cache without signing # create build cache without signing
args = parser.parse_args( args = parser.parse_args(
['create', '-d', mirror_path, '-u', str(spec)]) ['create', '-d', mirror_path, '-f', '-u', str(spec)])
buildcache.buildcache(parser, args) buildcache.buildcache(parser, args)
# Uninstall the package # Uninstall the package
@ -265,22 +265,16 @@ def test_relocate_links(tmpdir):
def test_needs_relocation(): def test_needs_relocation():
binary_type = (
'ELF 64-bit LSB executable, x86-64, version 1 (SYSV),'
' dynamically linked (uses shared libs),'
' for GNU/Linux x.y.z, stripped')
assert needs_binary_relocation(binary_type, os_id='Linux') assert needs_binary_relocation('application', 'x-sharedlib')
assert not needs_binary_relocation('relocatable', assert needs_binary_relocation('application', 'x-executable')
os_id='Linux') assert not needs_binary_relocation('application', 'x-octet-stream')
assert not needs_binary_relocation('symbolic link to `foo\'', assert not needs_binary_relocation('text', 'x-')
os_id='Linux')
assert needs_text_relocation('ASCII text') assert needs_text_relocation('text', 'x-')
assert not needs_text_relocation('symbolic link to `foo.text\'') assert not needs_text_relocation('symbolic link to', 'x-')
macho_type = 'Mach-O 64-bit executable x86_64' assert needs_binary_relocation('application', 'x-mach-binary')
assert needs_binary_relocation(macho_type, os_id='Darwin')
def test_macho_paths(): def test_macho_paths():