Prefer llnl.util.symlink.readlink to os.readlink (#44126)

Symlinks on Windows can use longpath prefixes (\\?\); these are fine
in the context of win32 API interactions but break numerous facets of
Spack behavior that rely on string parsing/matching (archiving,
binary distributions, tarball extraction, view regen, etc).

Spack's internal readlink method (llnl.util.symlink.readlink)
gracefully handles this by removing the prefix and otherwise behaving
exactly as os.readlink does, so we should prefer that in all cases.

16 files changed, 42 insertions, 28 deletions

diff --git a/lib/spack/llnl/util/filesystem.py b/lib/spack/llnl/util/filesystem.py
index 7fe9b9305d..f79cd48543 100644
--- a/lib/spack/llnl/util/filesystem.py
+++ b/lib/spack/llnl/util/filesystem.py
@@ -843,7 +843,7 @@ def copy_tree(
             if islink(s):
                 link_target = resolve_link_target_relative_to_the_link(s)
                 if symlinks:
-                    target = os.readlink(s)
+                    target = readlink(s)
                     if os.path.isabs(target):
 
                         def escaped_path(path):
diff --git a/lib/spack/spack/directory_layout.py b/lib/spack/spack/directory_layout.py
index b7a9b93431..923e89cf77 100644
--- a/lib/spack/spack/directory_layout.py
+++ b/lib/spack/spack/directory_layout.py
@@ -15,6 +15,7 @@ from pathlib import Path
 
 import llnl.util.filesystem as fs
 import llnl.util.tty as tty
+from llnl.util.symlink import readlink
 
 import spack.config
 import spack.hash_types as ht
@@ -181,7 +182,7 @@ class DirectoryLayout:
         base_dir = (
             self.path_for_spec(deprecator_spec)
             if deprecator_spec
-            else os.readlink(deprecated_spec.prefix)
+            else readlink(deprecated_spec.prefix)
         )
 
         yaml_path = os.path.join(
diff --git a/lib/spack/spack/environment/environment.py b/lib/spack/spack/environment/environment.py
index b3c35db36a..bc31f820b0 100644
--- a/lib/spack/spack/environment/environment.py
+++ b/lib/spack/spack/environment/environment.py
@@ -22,7 +22,7 @@ import llnl.util.filesystem as fs
 import llnl.util.tty as tty
 import llnl.util.tty.color as clr
 from llnl.util.link_tree import ConflictingSpecsError
-from llnl.util.symlink import symlink
+from llnl.util.symlink import readlink, symlink
 
 import spack.compilers
 import spack.concretize
@@ -662,7 +662,7 @@ class ViewDescriptor:
         if not os.path.islink(self.root):
             return None
 
-        root = os.readlink(self.root)
+        root = readlink(self.root)
         if os.path.isabs(root):
             return root
 
diff --git a/lib/spack/spack/platforms/cray.py b/lib/spack/spack/platforms/cray.py
index e8cc833c51..6c6802fd39 100644
--- a/lib/spack/spack/platforms/cray.py
+++ b/lib/spack/spack/platforms/cray.py
@@ -10,6 +10,7 @@ import re
 import archspec.cpu
 
 import llnl.util.tty as tty
+from llnl.util.symlink import readlink
 
 import spack.target
 import spack.version
@@ -133,7 +134,7 @@ class Cray(Platform):
         # Take the default version from known symlink path
         default_path = os.path.join(craype_dir, "default")
         if os.path.islink(default_path):
-            version = spack.version.Version(os.readlink(default_path))
+            version = spack.version.Version(readlink(default_path))
             return (craype_type, version)
 
         # If no default version, sort available versions and return latest
diff --git a/lib/spack/spack/relocate.py b/lib/spack/spack/relocate.py
index 30dc89f6e8..357dd92f84 100644
--- a/lib/spack/spack/relocate.py
+++ b/lib/spack/spack/relocate.py
@@ -566,7 +566,7 @@ def make_link_relative(new_links, orig_links):
         orig_links (list): original links
     """
     for new_link, orig_link in zip(new_links, orig_links):
-        target = os.readlink(orig_link)
+        target = readlink(orig_link)
         relative_target = os.path.relpath(target, os.path.dirname(orig_link))
         os.unlink(new_link)
         symlink(relative_target, new_link)
diff --git a/lib/spack/spack/rewiring.py b/lib/spack/spack/rewiring.py
index 297b0bd232..15d739562c 100644
--- a/lib/spack/spack/rewiring.py
+++ b/lib/spack/spack/rewiring.py
@@ -9,7 +9,7 @@ import shutil
 import tempfile
 from collections import OrderedDict
 
-from llnl.util.symlink import symlink
+from llnl.util.symlink import readlink, symlink
 
 import spack.binary_distribution as bindist
 import spack.error
@@ -26,7 +26,7 @@ def _relocate_spliced_links(links, orig_prefix, new_prefix):
     in our case. This still needs to be called after the copy to destination
     because it expects the new directory structure to be in place."""
     for link in links:
-        link_target = os.readlink(os.path.join(orig_prefix, link))
+        link_target = readlink(os.path.join(orig_prefix, link))
         link_target = re.sub("^" + orig_prefix, new_prefix, link_target)
         new_link_path = os.path.join(new_prefix, link)
         os.unlink(new_link_path)
diff --git a/lib/spack/spack/test/bindist.py b/lib/spack/spack/test/bindist.py
index b30955641f..be88666b17 100644
--- a/lib/spack/spack/test/bindist.py
+++ b/lib/spack/spack/test/bindist.py
@@ -22,6 +22,7 @@ import pytest
 import archspec.cpu
 
 from llnl.util.filesystem import join_path, visit_directory_tree
+from llnl.util.symlink import readlink
 
 import spack.binary_distribution as bindist
 import spack.caches
@@ -1062,10 +1063,10 @@ def test_tarball_common_prefix(dummy_prefix, tmpdir):
         assert set(os.listdir(os.path.join("prefix2", "share"))) == {"file"}
 
         # Relative symlink should still be correct
-        assert os.readlink(os.path.join("prefix2", "bin", "relative_app_link")) == "app"
+        assert readlink(os.path.join("prefix2", "bin", "relative_app_link")) == "app"
 
         # Absolute symlink should remain absolute -- this is for relocation to fix up.
-        assert os.readlink(os.path.join("prefix2", "bin", "absolute_app_link")) == os.path.join(
+        assert readlink(os.path.join("prefix2", "bin", "absolute_app_link")) == os.path.join(
             dummy_prefix, "bin", "app"
         )
 
diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py
index e1136e3bbe..c1147d49fe 100644
--- a/lib/spack/spack/test/cmd/env.py
+++ b/lib/spack/spack/test/cmd/env.py
@@ -15,6 +15,7 @@ import pytest
 import llnl.util.filesystem as fs
 import llnl.util.link_tree
 import llnl.util.tty as tty
+from llnl.util.symlink import readlink
 
 import spack.cmd.env
 import spack.config
@@ -4414,8 +4415,8 @@ def test_env_view_resolves_identical_file_conflicts(tmp_path, install_mockery, m
     #   view-file/bin/
     #     x                                          # expect this x to be linked
 
-    assert os.readlink(tmp_path / "view" / "bin" / "x") == bottom.bin.x
-    assert os.readlink(tmp_path / "view" / "bin" / "y") == top.bin.y
+    assert readlink(tmp_path / "view" / "bin" / "x") == bottom.bin.x
+    assert readlink(tmp_path / "view" / "bin" / "y") == top.bin.y
 
 
 def test_env_view_ignores_different_file_conflicts(tmp_path, install_mockery, mock_fetch):
@@ -4426,4 +4427,4 @@ def test_env_view_ignores_different_file_conflicts(tmp_path, install_mockery, mo
         install()
         prefix_dependent = e.matching_spec("view-ignore-conflict").prefix
     # The dependent's file is linked into the view
-    assert os.readlink(tmp_path / "view" / "bin" / "x") == prefix_dependent.bin.x
+    assert readlink(tmp_path / "view" / "bin" / "x") == prefix_dependent.bin.x
diff --git a/lib/spack/spack/test/llnl/util/filesystem.py b/lib/spack/spack/test/llnl/util/filesystem.py
index f04c2455cc..ea17a8fc8a 100644
--- a/lib/spack/spack/test/llnl/util/filesystem.py
+++ b/lib/spack/spack/test/llnl/util/filesystem.py
@@ -14,7 +14,7 @@ from contextlib import contextmanager
 import pytest
 
 import llnl.util.filesystem as fs
-from llnl.util.symlink import islink, symlink
+from llnl.util.symlink import islink, readlink, symlink
 
 import spack.paths
 
@@ -181,7 +181,7 @@ class TestCopyTree:
 
             assert os.path.exists("dest/a/b2")
             with fs.working_dir("dest/a"):
-                assert os.path.exists(os.readlink("b2"))
+                assert os.path.exists(readlink("b2"))
 
             assert os.path.realpath("dest/f/2") == os.path.abspath("dest/a/b/2")
             assert os.path.realpath("dest/2") == os.path.abspath("dest/1")
@@ -281,7 +281,7 @@ class TestInstallTree:
             symlink("nonexistant.txt", "source/broken", allow_broken_symlinks=True)
             fs.install_tree("source", "dest", symlinks=True, allow_broken_symlinks=True)
             assert os.path.islink("dest/broken")
-            assert not os.path.exists(os.readlink("dest/broken"))
+            assert not os.path.exists(readlink("dest/broken"))
 
     def test_glob_src(self, stage):
         """Test using a glob as the source."""
diff --git a/lib/spack/spack/test/modules/common.py b/lib/spack/spack/test/modules/common.py
index 7586728a8b..f668896346 100644
--- a/lib/spack/spack/test/modules/common.py
+++ b/lib/spack/spack/test/modules/common.py
@@ -7,6 +7,8 @@ import stat
 
 import pytest
 
+from llnl.util.symlink import readlink
+
 import spack.cmd.modules
 import spack.config
 import spack.error
@@ -78,7 +80,7 @@ def test_modules_default_symlink(
 
     link_path = os.path.join(os.path.dirname(mock_module_filename), "default")
     assert os.path.islink(link_path)
-    assert os.readlink(link_path) == mock_module_filename
+    assert readlink(link_path) == mock_module_filename
 
     generator.remove()
     assert not os.path.lexists(link_path)
diff --git a/lib/spack/spack/test/packaging.py b/lib/spack/spack/test/packaging.py
index 92ff4f6961..a633ad4bf0 100644
--- a/lib/spack/spack/test/packaging.py
+++ b/lib/spack/spack/test/packaging.py
@@ -16,7 +16,7 @@ from collections import OrderedDict
 import pytest
 
 from llnl.util import filesystem as fs
-from llnl.util.symlink import symlink
+from llnl.util.symlink import readlink, symlink
 
 import spack.binary_distribution as bindist
 import spack.cmd.buildcache as buildcache
@@ -181,12 +181,12 @@ def test_relocate_links(tmpdir):
         relocate_links(["to_self", "to_dependency", "to_system"], prefix_to_prefix)
 
         # These two are relocated
-        assert os.readlink("to_self") == str(tmpdir.join("new_prefix_a", "file"))
-        assert os.readlink("to_dependency") == str(tmpdir.join("new_prefix_b", "file"))
+        assert readlink("to_self") == str(tmpdir.join("new_prefix_a", "file"))
+        assert readlink("to_dependency") == str(tmpdir.join("new_prefix_b", "file"))
 
         # These two are not.
-        assert os.readlink("to_system") == system_path
-        assert os.readlink("to_self_but_relative") == "relative"
+        assert readlink("to_system") == system_path
+        assert readlink("to_self_but_relative") == "relative"
 
 
 def test_needs_relocation():
diff --git a/lib/spack/spack/test/stage.py b/lib/spack/spack/test/stage.py
index 9648ef34e1..b576a42f68 100644
--- a/lib/spack/spack/test/stage.py
+++ b/lib/spack/spack/test/stage.py
@@ -15,6 +15,7 @@ import sys
 import pytest
 
 from llnl.util.filesystem import getuid, mkdirp, partition_path, touch, working_dir
+from llnl.util.symlink import readlink
 
 import spack.error
 import spack.paths
@@ -872,7 +873,7 @@ def _create_files_from_tree(base, tree):
 
 def _create_tree_from_dir_recursive(path):
     if os.path.islink(path):
-        return os.readlink(path)
+        return readlink(path)
     elif os.path.isdir(path):
         tree = {}
         for name in os.listdir(path):
diff --git a/lib/spack/spack/verify.py b/lib/spack/spack/verify.py
index 10d6f91da0..7125481c6d 100644
--- a/lib/spack/spack/verify.py
+++ b/lib/spack/spack/verify.py
@@ -9,6 +9,7 @@ import stat
 from typing import Any, Dict
 
 import llnl.util.tty as tty
+from llnl.util.symlink import readlink
 
 import spack.filesystem_view
 import spack.store
@@ -38,7 +39,7 @@ def create_manifest_entry(path: str) -> Dict[str, Any]:
     data: Dict[str, Any] = {"mode": s.st_mode, "owner": s.st_uid, "group": s.st_gid}
 
     if stat.S_ISLNK(s.st_mode):
-        data["dest"] = os.readlink(path)
+        data["dest"] = readlink(path)
 
     elif stat.S_ISREG(s.st_mode):
         data["hash"] = compute_hash(path)
@@ -90,7 +91,7 @@ def check_entry(path, data):
     # instead of `lstat(...).st_mode`. So, ignore mode errors for symlinks.
     if not stat.S_ISLNK(s.st_mode) and s.st_mode != data["mode"]:
         res.add_error(path, "mode")
-    elif stat.S_ISLNK(s.st_mode) and os.readlink(path) != data.get("dest"):
+    elif stat.S_ISLNK(s.st_mode) and readlink(path) != data.get("dest"):
         res.add_error(path, "link")
     elif stat.S_ISREG(s.st_mode):
         # Check file contents against hash and listed as file
diff --git a/var/spack/repos/builtin/packages/ibm-java/package.py b/var/spack/repos/builtin/packages/ibm-java/package.py
index 97d63d65c8..376233effd 100644
--- a/var/spack/repos/builtin/packages/ibm-java/package.py
+++ b/var/spack/repos/builtin/packages/ibm-java/package.py
@@ -6,6 +6,8 @@
 import os
 import platform
 
+from llnl.util.symlink import readlink
+
 from spack.package import *
 
 
@@ -94,7 +96,7 @@ class IbmJava(Package):
         # The archive.bin file is quite fussy and doesn't work as a
         # symlink.
         if os.path.islink(archive):
-            targ = os.readlink(archive)
+            targ = readlink(archive)
             os.unlink(archive)
             copy(targ, archive)
 
diff --git a/var/spack/repos/builtin/packages/kaldi/package.py b/var/spack/repos/builtin/packages/kaldi/package.py
index 576c0850da..9e0d6bee9c 100644
--- a/var/spack/repos/builtin/packages/kaldi/package.py
+++ b/var/spack/repos/builtin/packages/kaldi/package.py
@@ -7,6 +7,8 @@ import os
 from fnmatch import fnmatch
 from os.path import join
 
+from llnl.util.symlink import readlink
+
 from spack.package import *
 
 
@@ -105,7 +107,7 @@ class Kaldi(Package):  # Does not use Autotools
                 for name in files:
                     if name.endswith("." + dso_suffix):
                         fpath = join(root, name)
-                        src = os.readlink(fpath)
+                        src = readlink(fpath)
                         install(src, prefix.lib)
 
             for root, dirs, files in os.walk("."):
diff --git a/var/spack/repos/builtin/packages/lua/package.py b/var/spack/repos/builtin/packages/lua/package.py
index e326ef988b..d418de07d3 100644
--- a/var/spack/repos/builtin/packages/lua/package.py
+++ b/var/spack/repos/builtin/packages/lua/package.py
@@ -6,6 +6,8 @@
 import glob
 import os
 
+from llnl.util.symlink import readlink
+
 import spack.build_environment
 from spack.package import *
 from spack.util.executable import Executable
@@ -79,7 +81,7 @@ class LuaImplPackage(MakefilePackage):
                 assert len(luajits) >= 1
                 luajit = luajits[0]
                 if os.path.islink(luajit):
-                    luajit = os.readlink(luajit)
+                    luajit = readlink(luajit)
                 symlink(luajit, "lua")
 
         with working_dir(self.prefix.include):