sbang: fixes for sbang relocation

This fixes sbang relocation when using old binary packages, and updates
code in `relocate.py`.

There are really two places where we would want to handle an `sbang`
relocation:

1. Installing an old package that uses `sbang` with shebang lines like
   `#!/bin/bash $spack_prefix/sbang`
2. Installing a *new* package that uses `sbang` with shebang lines like
   `#!/bin/sh $install_tree/sbang`

The second case is actually handled automatically by our text relocation;
we don't need any special relocation logic for new shebangs, as our
relocation logic already changes references to the build-time
`install_tree` to point to the `install_tree` at intall-time.

Case 1 was not properly handled -- we would not take an old binary
package and point its shebangs at the new `sbang` location. This PR fixes
that and updates the code in `relocation.py` with some notes.

There is one more case we don't currently handle: if a binary package is
created from an installation in a short prefix that does *not* need
`sbang` and is installed to a long prefix that *does* need `sbang`, we
won't do anything. We should just patch the file as we would for a normal
install. In some upcoming PR we should probably change *all* `sbang`
relocation logic to be idempotent and to apply to any sort of shebang'd
file. Then we'd only have to worry about which files to `sbang`-ify at
install time and wouldn't need to care about these special cases.

lib/spack/spack/hooks/sbang.py

@@ -27,6 +27,18 @@ def sbang_install_path():
     return os.path.join(spack.store.layout.root, "bin", "sbang")
 
 
+def sbang_shebang_line():
+    """Full shebang line that should be prepended to files to use sbang.
+
+    The line returned does not have a final newline (caller should add it
+    if needed).
+
+    This should be the only place in Spack that knows about what
+    interpreter we use for ``sbang``.
+    """
+    return '#!/bin/sh %s' % sbang_install_path()
+
+
 def shebang_too_long(path):
     """Detects whether a file has a shebang line that is too long."""
     if not os.path.isfile(path):
@@ -51,7 +63,7 @@ def filter_shebang(path):
             original = original.decode('UTF-8')
 
     # This line will be prepended to file
-    new_sbang_line = '#!/bin/sh %s\n' % sbang_install_path()
+    new_sbang_line = '%s\n' % sbang_shebang_line()
 
     # Skip files that are already using sbang.
     if original.startswith(new_sbang_line):

lib/spack/spack/relocate.py

@@ -136,7 +136,7 @@ def _make_relative(reference_file, path_root, paths):
         reference_file (str): file from which the reference directory
             is computed
         path_root (str): root of the relative paths
-        paths: paths to be examined
+        paths: (list) paths to be examined
 
     Returns:
         List of relative paths
@@ -790,10 +790,10 @@ def relocate_text(
         files, orig_layout_root, new_layout_root, orig_install_prefix,
         new_install_prefix, orig_spack, new_spack, new_prefixes
 ):
-    """Relocate text file from the original installation prefix to the
+    """Relocate text file from the original ``install_tree`` to the new one.
-    new prefix.
 
-    Relocation also affects the the path in Spack's sbang script.
+    This also handles relocating Spack's sbang scripts to point at the
+    new install tree.
 
     Args:
         files (list): text files to be relocated
@@ -805,18 +805,28 @@ def relocate_text(
         new_spack (str): path to the new Spack
         new_prefixes (dict): dictionary that maps the original prefixes to
             where they should be relocated
+
     """
     # TODO: reduce the number of arguments (8 seems too much)
-    orig_sbang = '#!/bin/bash {0}/bin/sbang'.format(orig_spack)
-    new_sbang = '#!/bin/bash {0}/bin/sbang'.format(new_spack)
 
+    # This is vestigial code for the *old* location of sbang. Previously,
+    # sbang was a bash script, and it lived in the spack prefix. It is
+    # now a POSIX script that lives in the install prefix. Old packages
+    # will have the old sbang location in their shebangs.
+    import spack.hooks.sbang as sbang
+    orig_sbang = '#!/bin/bash {0}/bin/sbang'.format(orig_spack)
+    new_sbang = sbang.sbang_shebang_line()
+
+    # Do relocations on text that refers to the install tree
     for filename in files:
         _replace_prefix_text(filename, orig_install_prefix, new_install_prefix)
         for orig_dep_prefix, new_dep_prefix in new_prefixes.items():
             _replace_prefix_text(filename, orig_dep_prefix, new_dep_prefix)
         _replace_prefix_text(filename, orig_layout_root, new_layout_root)
-        # relocate the sbang location only if the spack directory changed
+
-        if orig_spack != new_spack:
+        # Point old packages at the new sbang location. Packages that
+        # already use the new sbang location will already have been
+        # handled by the prior call to _replace_prefix_text
         _replace_prefix_text(filename, orig_sbang, new_sbang)
 
 

lib/spack/spack/test/relocate.py

@@ -12,6 +12,7 @@
 import pytest
 import spack.architecture
 import spack.concretize
+import spack.hooks.sbang as sbang
 import spack.paths
 import spack.relocate
 import spack.spec
@@ -401,3 +402,48 @@ def test_relocate_text_bin_raise_if_new_prefix_is_longer():
         spack.relocate.relocate_text_bin(
             ['item'], short_prefix, long_prefix, None, None, None
         )
+
+
+@pytest.mark.parametrize("sbang_line", [
+    "#!/bin/bash /path/to/orig/spack/bin/sbang",
+    "#!/bin/sh /orig/layout/root/bin/sbang"
+])
+def test_relocate_text_old_sbang(tmpdir, sbang_line):
+    """Ensure that old and new sbang styles are relocated."""
+
+    old_install_prefix = "/orig/layout/root/orig/install/prefix"
+    new_install_prefix = os.path.join(
+        spack.store.layout.root, "new", "install", "prefix"
+    )
+
+    # input file with an sbang line
+    original = """\
+{0}
+#!/usr/bin/env python
+
+/orig/layout/root/orig/install/prefix
+""".format(sbang_line)
+
+    # expected relocation
+    expected = """\
+{0}
+#!/usr/bin/env python
+
+{1}
+""".format(sbang.sbang_shebang_line(), new_install_prefix)
+
+    path = tmpdir.ensure("path", "to", "file")
+    with path.open("w") as f:
+        f.write(original)
+
+    spack.relocate.relocate_text(
+        [str(path)],
+        "/orig/layout/root",   spack.store.layout.root,
+        old_install_prefix,    new_install_prefix,
+        "/path/to/orig/spack", spack.paths.spack_root,
+        {
+            old_install_prefix: new_install_prefix
+        }
+    )
+
+    assert expected == open(str(path)).read()