Minimal cleanup of a few tests in `test/packaging.py` (#38880)

* Minimal cleanup of a few tests in packaging.py

* Use f-strings

1 files changed, 125 insertions, 162 deletions

diff --git a/lib/spack/spack/test/packaging.py b/lib/spack/spack/test/packaging.py
index 43193b578c..b9bc51ac42 100644
--- a/lib/spack/spack/test/packaging.py
+++ b/lib/spack/spack/test/packaging.py
@@ -8,15 +8,15 @@ This test checks the binary packaging infrastructure
 """
 import argparse
 import os
+import pathlib
 import platform
 import shutil
-import stat
 import sys
 from collections import OrderedDict
 
 import pytest
 
-from llnl.util.filesystem import mkdirp
+from llnl.util import filesystem as fs
 from llnl.util.symlink import symlink
 
 import spack.binary_distribution as bindist
@@ -43,144 +43,110 @@ from spack.spec import Spec
 pytestmark = pytest.mark.skipif(sys.platform == "win32", reason="does not run on windows")
 
 
-def fake_fetchify(url, pkg):
-    """Fake the URL for a package so it downloads from a file."""
-    fetcher = FetchStrategyComposite()
-    fetcher.append(URLFetchStrategy(url))
-    pkg.fetcher = fetcher
-
-
 @pytest.mark.usefixtures("install_mockery", "mock_gnupghome")
-def test_buildcache(mock_archive, tmpdir):
-    # tweak patchelf to only do a download
-    pspec = Spec("patchelf").concretized()
-    pkg = pspec.package
-    fake_fetchify(pkg.fetcher, pkg)
-    mkdirp(os.path.join(pkg.prefix, "bin"))
-    patchelfscr = os.path.join(pkg.prefix, "bin", "patchelf")
-    f = open(patchelfscr, "w")
-    body = """#!/bin/bash
-echo $PATH"""
-    f.write(body)
-    f.close()
-    st = os.stat(patchelfscr)
-    os.chmod(patchelfscr, st.st_mode | stat.S_IEXEC)
-
-    # Install the test package
-    spec = Spec("trivial-install-test-package")
-    spec.concretize()
-    assert spec.concrete
-    pkg = spec.package
-    fake_fetchify(mock_archive.url, pkg)
-    pkg.do_install()
+def test_buildcache(mock_archive, tmp_path, monkeypatch, mutable_config):
+    # Install a test package
+    spec = Spec("trivial-install-test-package").concretized()
+    fetcher = FetchStrategyComposite()
+    fetcher.append(URLFetchStrategy(mock_archive.url))
+    monkeypatch.setattr(spec.package, "fetcher", fetcher)
+    spec.package.do_install()
     pkghash = "/" + str(spec.dag_hash(7))
 
     # Put some non-relocatable file in there
-    filename = os.path.join(spec.prefix, "dummy.txt")
-    with open(filename, "w") as script:
-        script.write(spec.prefix)
+    dummy_txt = pathlib.Path(spec.prefix) / "dummy.txt"
+    dummy_txt.write_text(spec.prefix)
 
     # Create an absolute symlink
     linkname = os.path.join(spec.prefix, "link_to_dummy.txt")
-    symlink(filename, linkname)
+    symlink(dummy_txt, linkname)
 
-    # Create the build cache  and
-    # put it directly into the mirror
-    mirror_path = os.path.join(str(tmpdir), "test-mirror")
+    # Create the build cache and put it directly into the mirror
+    mirror_path = str(tmp_path / "test-mirror")
     spack.mirror.create(mirror_path, specs=[])
 
     # register mirror with spack config
     mirrors = {"spack-mirror-test": url_util.path_to_file_url(mirror_path)}
     spack.config.set("mirrors", mirrors)
 
-    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)
+    with spack.stage.Stage(mirrors["spack-mirror-test"], name="build_cache", keep=True):
+        parser = argparse.ArgumentParser()
+        buildcache.setup_parser(parser)
+
+        create_args = ["create", "-a", "-f", mirror_path, pkghash]
+        # Create a private key to sign package with if gpg2 available
+        spack.util.gpg.create(
+            name="test key 1",
+            expires="0",
+            email="spack@googlegroups.com",
+            comment="Spack test key",
+        )
 
-    create_args = ["create", "-a", "-f", mirror_path, pkghash]
-    # Create a private key to sign package with if gpg2 available
-    spack.util.gpg.create(
-        name="test key 1", expires="0", email="spack@googlegroups.com", comment="Spack test key"
-    )
+        create_args.insert(create_args.index("-a"), "--rebuild-index")
 
-    create_args.insert(create_args.index("-a"), "--rebuild-index")
+        args = parser.parse_args(create_args)
+        buildcache.buildcache(parser, args)
+        # trigger overwrite warning
+        buildcache.buildcache(parser, args)
 
-    args = parser.parse_args(create_args)
-    buildcache.buildcache(parser, args)
-    # trigger overwrite warning
-    buildcache.buildcache(parser, args)
+        # Uninstall the package
+        spec.package.do_uninstall(force=True)
 
-    # Uninstall the package
-    pkg.do_uninstall(force=True)
+        install_args = ["install", "-f", pkghash]
+        args = parser.parse_args(install_args)
+        # Test install
+        buildcache.buildcache(parser, args)
 
-    install_args = ["install", "-f", pkghash]
-    args = parser.parse_args(install_args)
-    # Test install
-    buildcache.buildcache(parser, args)
+        files = os.listdir(spec.prefix)
 
-    files = os.listdir(spec.prefix)
+        assert "link_to_dummy.txt" in files
+        assert "dummy.txt" in files
 
-    assert "link_to_dummy.txt" in files
-    assert "dummy.txt" in files
+        # 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"]
 
-    # 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(["keys"])
+        buildcache.buildcache(parser, args)
 
-    args = parser.parse_args(["keys"])
-    buildcache.buildcache(parser, args)
+        args = parser.parse_args(["list"])
+        buildcache.buildcache(parser, args)
 
-    args = parser.parse_args(["list"])
-    buildcache.buildcache(parser, args)
+        args = parser.parse_args(["list"])
+        buildcache.buildcache(parser, args)
 
-    args = parser.parse_args(["list"])
-    buildcache.buildcache(parser, args)
+        args = parser.parse_args(["list", "trivial"])
+        buildcache.buildcache(parser, args)
 
-    args = parser.parse_args(["list", "trivial"])
-    buildcache.buildcache(parser, args)
+        # Copy a key to the mirror to have something to download
+        shutil.copyfile(mock_gpg_keys_path + "/external.key", mirror_path + "/external.key")
 
-    # Copy a key to the mirror to have something to download
-    shutil.copyfile(mock_gpg_keys_path + "/external.key", mirror_path + "/external.key")
+        args = parser.parse_args(["keys"])
+        buildcache.buildcache(parser, args)
 
-    args = parser.parse_args(["keys"])
-    buildcache.buildcache(parser, args)
+        args = parser.parse_args(["keys", "-f"])
+        buildcache.buildcache(parser, args)
 
-    args = parser.parse_args(["keys", "-f"])
-    buildcache.buildcache(parser, args)
+        args = parser.parse_args(["keys", "-i", "-t"])
+        buildcache.buildcache(parser, args)
 
-    args = parser.parse_args(["keys", "-i", "-t"])
-    buildcache.buildcache(parser, args)
 
-    # unregister mirror with spack config
-    mirrors = {}
-    spack.config.set("mirrors", mirrors)
-    shutil.rmtree(mirror_path)
-    stage.destroy()
+def test_relocate_text(tmp_path):
+    """Tests that a text file containing the original directory of an installation, can be
+    relocated to a target directory.
+    """
+    original_dir = "/home/spack/opt/spack"
+    relocation_dir = "/opt/rh/devtoolset/"
+    dummy_txt = tmp_path / "dummy.txt"
+    dummy_txt.write_text(original_dir)
 
+    ensure_binary_is_relocatable(os.path.realpath(dummy_txt))
+    relocate_text([str(dummy_txt)], {original_dir: relocation_dir})
+    text = dummy_txt.read_text()
 
-@pytest.mark.usefixtures("install_mockery")
-def test_relocate_text(tmpdir):
-    spec = Spec("trivial-install-test-package")
-    spec.concretize()
-    with tmpdir.as_cwd():
-        # Validate the text path replacement
-        old_dir = "/home/spack/opt/spack"
-        filename = "dummy.txt"
-        with open(filename, "w") as script:
-            script.write(old_dir)
-            script.close()
-        filenames = [filename]
-        new_dir = "/opt/rh/devtoolset/"
-        # Singleton dict doesn't matter if Ordered
-        relocate_text(filenames, {old_dir: new_dir})
-        with open(filename, "r") as script:
-            for line in script:
-                assert new_dir in line
-        ensure_binary_is_relocatable(os.path.realpath(filename))
+    assert relocation_dir in text
+    assert original_dir not in text
 
 
 def test_relocate_links(tmpdir):
@@ -229,7 +195,6 @@ def test_needs_relocation():
     assert not needs_binary_relocation("text", "x-")
     assert needs_text_relocation("text", "x-")
     assert not needs_text_relocation("symbolic link to", "x-")
-
     assert needs_binary_relocation("application", "x-mach-binary")
 
 
@@ -242,41 +207,41 @@ def test_replace_paths(tmpdir):
         hash_d = "hukkosc7ahff7o65h6cdhvcoxm57d4bw"
         hash_loco = "zy4oigsc4eovn5yhr2lk4aukwzoespob"
 
-        prefix2hash = dict()
+        prefix2hash = {}
 
-        old_spack_dir = os.path.join("%s" % tmpdir, "Users", "developer", "spack")
-        mkdirp(old_spack_dir)
+        old_spack_dir = os.path.join(f"{tmpdir}", "Users", "developer", "spack")
+        fs.mkdirp(old_spack_dir)
 
-        oldprefix_a = os.path.join("%s" % old_spack_dir, "pkgA-%s" % hash_a)
-        oldlibdir_a = os.path.join("%s" % oldprefix_a, "lib")
-        mkdirp(oldlibdir_a)
+        oldprefix_a = os.path.join(f"{old_spack_dir}", f"pkgA-{hash_a}")
+        oldlibdir_a = os.path.join(f"{oldprefix_a}", "lib")
+        fs.mkdirp(oldlibdir_a)
         prefix2hash[str(oldprefix_a)] = hash_a
 
-        oldprefix_b = os.path.join("%s" % old_spack_dir, "pkgB-%s" % hash_b)
-        oldlibdir_b = os.path.join("%s" % oldprefix_b, "lib")
-        mkdirp(oldlibdir_b)
+        oldprefix_b = os.path.join(f"{old_spack_dir}", f"pkgB-{hash_b}")
+        oldlibdir_b = os.path.join(f"{oldprefix_b}", "lib")
+        fs.mkdirp(oldlibdir_b)
         prefix2hash[str(oldprefix_b)] = hash_b
 
-        oldprefix_c = os.path.join("%s" % old_spack_dir, "pkgC-%s" % hash_c)
-        oldlibdir_c = os.path.join("%s" % oldprefix_c, "lib")
-        oldlibdir_cc = os.path.join("%s" % oldlibdir_c, "C")
-        mkdirp(oldlibdir_c)
+        oldprefix_c = os.path.join(f"{old_spack_dir}", f"pkgC-{hash_c}")
+        oldlibdir_c = os.path.join(f"{oldprefix_c}", "lib")
+        oldlibdir_cc = os.path.join(f"{oldlibdir_c}", "C")
+        fs.mkdirp(oldlibdir_c)
         prefix2hash[str(oldprefix_c)] = hash_c
 
-        oldprefix_d = os.path.join("%s" % old_spack_dir, "pkgD-%s" % hash_d)
-        oldlibdir_d = os.path.join("%s" % oldprefix_d, "lib")
-        mkdirp(oldlibdir_d)
+        oldprefix_d = os.path.join(f"{old_spack_dir}", f"pkgD-{hash_d}")
+        oldlibdir_d = os.path.join(f"{oldprefix_d}", "lib")
+        fs.mkdirp(oldlibdir_d)
         prefix2hash[str(oldprefix_d)] = hash_d
 
-        oldprefix_local = os.path.join("%s" % tmpdir, "usr", "local")
-        oldlibdir_local = os.path.join("%s" % oldprefix_local, "lib")
-        mkdirp(oldlibdir_local)
+        oldprefix_local = os.path.join(f"{tmpdir}", "usr", "local")
+        oldlibdir_local = os.path.join(f"{oldprefix_local}", "lib")
+        fs.mkdirp(oldlibdir_local)
         prefix2hash[str(oldprefix_local)] = hash_loco
-        libfile_a = "libA.%s" % suffix
-        libfile_b = "libB.%s" % suffix
-        libfile_c = "libC.%s" % suffix
-        libfile_d = "libD.%s" % suffix
-        libfile_loco = "libloco.%s" % suffix
+        libfile_a = f"libA.{suffix}"
+        libfile_b = f"libB.{suffix}"
+        libfile_c = f"libC.{suffix}"
+        libfile_d = f"libD.{suffix}"
+        libfile_loco = f"libloco.{suffix}"
         old_libnames = [
             os.path.join(oldlibdir_a, libfile_a),
             os.path.join(oldlibdir_b, libfile_b),
@@ -291,33 +256,33 @@ def test_replace_paths(tmpdir):
 
         hash2prefix = dict()
 
-        new_spack_dir = os.path.join("%s" % tmpdir, "Users", "Shared", "spack")
-        mkdirp(new_spack_dir)
+        new_spack_dir = os.path.join(f"{tmpdir}", "Users", "Shared", "spack")
+        fs.mkdirp(new_spack_dir)
 
-        prefix_a = os.path.join(new_spack_dir, "pkgA-%s" % hash_a)
+        prefix_a = os.path.join(new_spack_dir, f"pkgA-{hash_a}")
         libdir_a = os.path.join(prefix_a, "lib")
-        mkdirp(libdir_a)
+        fs.mkdirp(libdir_a)
         hash2prefix[hash_a] = str(prefix_a)
 
-        prefix_b = os.path.join(new_spack_dir, "pkgB-%s" % hash_b)
+        prefix_b = os.path.join(new_spack_dir, f"pkgB-{hash_b}")
         libdir_b = os.path.join(prefix_b, "lib")
-        mkdirp(libdir_b)
+        fs.mkdirp(libdir_b)
         hash2prefix[hash_b] = str(prefix_b)
 
-        prefix_c = os.path.join(new_spack_dir, "pkgC-%s" % hash_c)
+        prefix_c = os.path.join(new_spack_dir, f"pkgC-{hash_c}")
         libdir_c = os.path.join(prefix_c, "lib")
         libdir_cc = os.path.join(libdir_c, "C")
-        mkdirp(libdir_cc)
+        fs.mkdirp(libdir_cc)
         hash2prefix[hash_c] = str(prefix_c)
 
-        prefix_d = os.path.join(new_spack_dir, "pkgD-%s" % hash_d)
+        prefix_d = os.path.join(new_spack_dir, f"pkgD-{hash_d}")
         libdir_d = os.path.join(prefix_d, "lib")
-        mkdirp(libdir_d)
+        fs.mkdirp(libdir_d)
         hash2prefix[hash_d] = str(prefix_d)
 
-        prefix_local = os.path.join("%s" % tmpdir, "usr", "local")
+        prefix_local = os.path.join(f"{tmpdir}", "usr", "local")
         libdir_local = os.path.join(prefix_local, "lib")
-        mkdirp(libdir_local)
+        fs.mkdirp(libdir_local)
         hash2prefix[hash_loco] = str(prefix_local)
 
         new_libnames = [
@@ -386,10 +351,10 @@ def test_replace_paths(tmpdir):
         out_dict = macho_find_paths(
             [oldlibdir_a, oldlibdir_b, oldlibdir_c, oldlibdir_cc, oldlibdir_local],
             [
-                "@rpath/%s" % libfile_a,
-                "@rpath/%s" % libfile_b,
-                "@rpath/%s" % libfile_c,
-                "@rpath/%s" % libfile_loco,
+                f"@rpath/{libfile_a}",
+                f"@rpath/{libfile_b}",
+                f"@rpath/{libfile_c}",
+                f"@rpath/{libfile_loco}",
             ],
             None,
             old_spack_dir,
@@ -397,10 +362,10 @@ def test_replace_paths(tmpdir):
         )
 
         assert out_dict == {
-            "@rpath/%s" % libfile_a: "@rpath/%s" % libfile_a,
-            "@rpath/%s" % libfile_b: "@rpath/%s" % libfile_b,
-            "@rpath/%s" % libfile_c: "@rpath/%s" % libfile_c,
-            "@rpath/%s" % libfile_loco: "@rpath/%s" % libfile_loco,
+            f"@rpath/{libfile_a}": f"@rpath/{libfile_a}",
+            f"@rpath/{libfile_b}": f"@rpath/{libfile_b}",
+            f"@rpath/{libfile_c}": f"@rpath/{libfile_c}",
+            f"@rpath/{libfile_loco}": f"@rpath/{libfile_loco}",
             oldlibdir_a: libdir_a,
             oldlibdir_b: libdir_b,
             oldlibdir_c: libdir_c,
@@ -410,15 +375,15 @@ def test_replace_paths(tmpdir):
 
         out_dict = macho_find_paths(
             [oldlibdir_a, oldlibdir_b, oldlibdir_d, oldlibdir_local],
-            ["@rpath/%s" % libfile_a, "@rpath/%s" % libfile_b, "@rpath/%s" % libfile_loco],
+            [f"@rpath/{libfile_a}", f"@rpath/{libfile_b}", f"@rpath/{libfile_loco}"],
             None,
             old_spack_dir,
             prefix2prefix,
         )
         assert out_dict == {
-            "@rpath/%s" % libfile_a: "@rpath/%s" % libfile_a,
-            "@rpath/%s" % libfile_b: "@rpath/%s" % libfile_b,
-            "@rpath/%s" % libfile_loco: "@rpath/%s" % libfile_loco,
+            f"@rpath/{libfile_a}": f"@rpath/{libfile_a}",
+            f"@rpath/{libfile_b}": f"@rpath/{libfile_b}",
+            f"@rpath/{libfile_loco}": f"@rpath/{libfile_loco}",
             oldlibdir_a: libdir_a,
             oldlibdir_b: libdir_b,
             oldlibdir_d: libdir_d,
@@ -550,18 +515,16 @@ def test_manual_download(
 
     @property
     def _instr(pkg):
-        return "Download instructions for {0}".format(pkg.spec.name)
+        return f"Download instructions for {pkg.spec.name}"
 
     spec = default_mock_concretization("a")
-    pkg = spec.package
-
-    pkg.manual_download = manual
+    spec.package.manual_download = manual
     if instr:
         monkeypatch.setattr(spack.package_base.PackageBase, "download_instr", _instr)
 
-    expected = pkg.download_instr if manual else "All fetchers failed"
+    expected = spec.package.download_instr if manual else "All fetchers failed"
     with pytest.raises(spack.util.web.FetchError, match=expected):
-        pkg.do_fetch()
+        spec.package.do_fetch()
 
 
 @pytest.fixture()