From 04258f9ccef24852fa1e2f39a20491e0a9b8b65d Mon Sep 17 00:00:00 2001
From: "John W. Parent" <45471568+johnwparent@users.noreply.github.com>
Date: Thu, 16 May 2024 13:56:04 -0400
Subject: [PATCH] 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.
---
 lib/spack/llnl/util/filesystem.py                    |  2 +-
 lib/spack/spack/directory_layout.py                  |  3 ++-
 lib/spack/spack/environment/environment.py           |  4 ++--
 lib/spack/spack/platforms/cray.py                    |  3 ++-
 lib/spack/spack/relocate.py                          |  2 +-
 lib/spack/spack/rewiring.py                          |  4 ++--
 lib/spack/spack/test/bindist.py                      |  5 +++--
 lib/spack/spack/test/cmd/env.py                      |  7 ++++---
 lib/spack/spack/test/llnl/util/filesystem.py         |  6 +++---
 lib/spack/spack/test/modules/common.py               |  4 +++-
 lib/spack/spack/test/packaging.py                    | 10 +++++-----
 lib/spack/spack/test/stage.py                        |  3 ++-
 lib/spack/spack/verify.py                            |  5 +++--
 var/spack/repos/builtin/packages/ibm-java/package.py |  4 +++-
 var/spack/repos/builtin/packages/kaldi/package.py    |  4 +++-
 var/spack/repos/builtin/packages/lua/package.py      |  4 +++-
 16 files changed, 42 insertions(+), 28 deletions(-)

diff --git a/lib/spack/llnl/util/filesystem.py b/lib/spack/llnl/util/filesystem.py
index f9cee1e118..57baa509c3 100644
--- a/lib/spack/llnl/util/filesystem.py
+++ b/lib/spack/llnl/util/filesystem.py
@@ -822,7 +822,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 @@
 
 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 @@ def deprecated_file_path(self, deprecated_spec, deprecator_spec=None):
         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 cd7221d425..22935d525b 100644
--- a/lib/spack/spack/environment/environment.py
+++ b/lib/spack/spack/environment/environment.py
@@ -22,7 +22,7 @@
 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 @@ def _current_root(self):
         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 archspec.cpu
 
 import llnl.util.tty as tty
+from llnl.util.symlink import readlink
 
 import spack.target
 import spack.version
@@ -133,7 +134,7 @@ def craype_type_and_version(cls):
         # 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 0bbbba7ef0..509d610225 100644
--- a/lib/spack/spack/relocate.py
+++ b/lib/spack/spack/relocate.py
@@ -565,7 +565,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 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 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 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 @@
 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 @@ def test_symlinks_true(self, stage):
 
             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 @@ def test_allow_broken_symlinks(self, stage):
             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 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 @@
 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 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 @@
 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 @@ def install(self, spec, prefix):
         # 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 @@
 from fnmatch import fnmatch
 from os.path import join
 
+from llnl.util.symlink import readlink
+
 from spack.package import *
 
 
@@ -105,7 +107,7 @@ def install(self, spec, prefix):
                 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 @@ def symlink_luajit(self):
                 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):