link: directly bind to os.* on non-windows (#44400)

The windows wrappers for basic functions like `os.symlink`,
`os.readlink` and `os.path.islink` in the `llnl.util.symlink` module
have bugs, and trigger more file system operations on non-windows than
they should.

This commit just binds `llnl.util.symlink.symlink = os.symlink` etc so
built-in functions are used on non-windows

lib/spack/llnl/util/filesystem.py

@@ -766,7 +766,6 @@ def copy_tree(
     src: str,
     dest: str,
     symlinks: bool = True,
-    allow_broken_symlinks: bool = sys.platform != "win32",
     ignore: Optional[Callable[[str], bool]] = None,
     _permissions: bool = False,
 ):
@@ -789,8 +788,6 @@ def copy_tree(
         src (str): the directory to copy
         dest (str): the destination directory
         symlinks (bool): whether or not to preserve symlinks
-        allow_broken_symlinks (bool): whether or not to allow broken (dangling) symlinks,
-            On Windows, setting this to True will raise an exception. Defaults to true on unix.
         ignore (typing.Callable): function indicating which files to ignore
         _permissions (bool): for internal use only
 
@@ -798,8 +795,6 @@ def copy_tree(
         IOError: if *src* does not match any files or directories
         ValueError: if *src* is a parent directory of *dest*
     """
-    if allow_broken_symlinks and sys.platform == "win32":
-        raise llnl.util.symlink.SymlinkError("Cannot allow broken symlinks on Windows!")
     if _permissions:
         tty.debug("Installing {0} to {1}".format(src, dest))
     else:
@@ -872,16 +867,14 @@ def escaped_path(path):
                 copy_mode(s, d)
 
     for target, d, s in links:
-        symlink(target, d, allow_broken_symlinks=allow_broken_symlinks)
+        symlink(target, d)
         if _permissions:
             set_install_permissions(d)
             copy_mode(s, d)
 
 
 @system_path_filter
-def install_tree(
+def install_tree(src, dest, symlinks=True, ignore=None):
-    src, dest, symlinks=True, ignore=None, allow_broken_symlinks=sys.platform != "win32"
-):
     """Recursively install an entire directory tree rooted at *src*.
 
     Same as :py:func:`copy_tree` with the addition of setting proper
@@ -892,21 +885,12 @@ def install_tree(
         dest (str): the destination directory
         symlinks (bool): whether or not to preserve symlinks
         ignore (typing.Callable): function indicating which files to ignore
-        allow_broken_symlinks (bool): whether or not to allow broken (dangling) symlinks,
-            On Windows, setting this to True will raise an exception.
 
     Raises:
         IOError: if *src* does not match any files or directories
         ValueError: if *src* is a parent directory of *dest*
     """
-    copy_tree(
+    copy_tree(src, dest, symlinks=symlinks, ignore=ignore, _permissions=True)
-        src,
-        dest,
-        symlinks=symlinks,
-        allow_broken_symlinks=allow_broken_symlinks,
-        ignore=ignore,
-        _permissions=True,
-    )
 
 
 @system_path_filter

lib/spack/llnl/util/symlink.py

@@ -8,6 +8,7 @@
 import subprocess
 import sys
 import tempfile
+from typing import Union
 
 from llnl.util import lang, tty
 
@@ -16,92 +17,66 @@
 if sys.platform == "win32":
     from win32file import CreateHardLink
 
-is_windows = sys.platform == "win32"
 
+def _windows_symlink(
+    src: str, dst: str, target_is_directory: bool = False, *, dir_fd: Union[int, None] = None
+):
+    """On Windows with System Administrator privileges this will be a normal symbolic link via
+    os.symlink. On Windows without privledges the link will be a junction for a directory and a
+    hardlink for a file. On Windows the various link types are:
 
-def symlink(source_path: str, link_path: str, allow_broken_symlinks: bool = not is_windows):
+    Symbolic Link: A link to a file or directory on the same or different volume (drive letter) or
-    """
+    even to a remote file or directory (using UNC in its path). Need System Administrator
-    Create a link.
+    privileges to make these.
 
-    On non-Windows and Windows with System Administrator
+    Hard Link: A link to a file on the same volume (drive letter) only. Every file (file's data)
-    privleges this will be a normal symbolic link via
+    has at least 1 hard link (file's name). But when this method creates a new hard link there will
-    os.symlink.
+    be 2. Deleting all hard links effectively deletes the file. Don't need System Administrator
+    privileges.
 
-    On Windows without privledges the link will be a
+    Junction: A link to a directory on the same or different volume (drive letter) but not to a
-    junction for a directory and a hardlink for a file.
+    remote directory. Don't need System Administrator privileges."""
-    On Windows the various link types are:
+    source_path = os.path.normpath(src)
-
-    Symbolic Link: A link to a file or directory on the
-    same or different volume (drive letter) or even to
-    a remote file or directory (using UNC in its path).
-    Need System Administrator privileges to make these.
-
-    Hard Link: A link to a file on the same volume (drive
-    letter) only. Every file (file's data) has at least 1
-    hard link (file's name). But when this method creates
-    a new hard link there will be 2. Deleting all hard
-    links effectively deletes the file. Don't need System
-    Administrator privileges.
-
-    Junction: A link to a directory on the same or different
-    volume (drive letter) but not to a remote directory. Don't
-    need System Administrator privileges.
-
-    Parameters:
-        source_path (str): The real file or directory that the link points to.
-            Must be absolute OR relative to the link.
-        link_path (str): The path where the link will exist.
-        allow_broken_symlinks (bool): On Linux or Mac, don't raise an exception if the source_path
-            doesn't exist. This will still raise an exception on Windows.
-    """
-    source_path = os.path.normpath(source_path)
     win_source_path = source_path
-    link_path = os.path.normpath(link_path)
+    link_path = os.path.normpath(dst)
 
-    # Never allow broken links on Windows.
+    # Perform basic checks to make sure symlinking will succeed
-    if sys.platform == "win32" and allow_broken_symlinks:
+    if os.path.lexists(link_path):
-        raise ValueError("allow_broken_symlinks parameter cannot be True on Windows.")
+        raise AlreadyExistsError(f"Link path ({link_path}) already exists. Cannot create link.")
 
-    if not allow_broken_symlinks:
+    if not os.path.exists(source_path):
-        # Perform basic checks to make sure symlinking will succeed
+        if os.path.isabs(source_path):
-        if os.path.lexists(link_path):
+            # An absolute source path that does not exist will result in a broken link.
-            raise AlreadyExistsError(
+            raise SymlinkError(
-                f"Link path ({link_path}) already exists. Cannot create link."
+                f"Source path ({source_path}) is absolute but does not exist. Resulting "
+                f"link would be broken so not making link."
             )
-
+        else:
-        if not os.path.exists(source_path):
+            # os.symlink can create a link when the given source path is relative to
-            if os.path.isabs(source_path) and not allow_broken_symlinks:
+            # the link path. Emulate this behavior and check to see if the source exists
-                # An absolute source path that does not exist will result in a broken link.
+            # relative to the link path ahead of link creation to prevent broken
-                raise SymlinkError(
+            # links from being made.
-                    f"Source path ({source_path}) is absolute but does not exist. Resulting "
+            link_parent_dir = os.path.dirname(link_path)
-                    f"link would be broken so not making link."
+            relative_path = os.path.join(link_parent_dir, source_path)
-                )
+            if os.path.exists(relative_path):
+                # In order to work on windows, the source path needs to be modified to be
+                # relative because hardlink/junction dont resolve relative paths the same
+                # way as os.symlink. This is ignored on other operating systems.
+                win_source_path = relative_path
             else:
-                # os.symlink can create a link when the given source path is relative to
+                raise SymlinkError(
-                # the link path. Emulate this behavior and check to see if the source exists
+                    f"The source path ({source_path}) is not relative to the link path "
-                # relative to the link path ahead of link creation to prevent broken
+                    f"({link_path}). Resulting link would be broken so not making link."
-                # links from being made.
+                )
-                link_parent_dir = os.path.dirname(link_path)
-                relative_path = os.path.join(link_parent_dir, source_path)
-                if os.path.exists(relative_path):
-                    # In order to work on windows, the source path needs to be modified to be
-                    # relative because hardlink/junction dont resolve relative paths the same
-                    # way as os.symlink. This is ignored on other operating systems.
-                    win_source_path = relative_path
-                elif not allow_broken_symlinks:
-                    raise SymlinkError(
-                        f"The source path ({source_path}) is not relative to the link path "
-                        f"({link_path}). Resulting link would be broken so not making link."
-                    )
 
     # Create the symlink
-    if sys.platform == "win32" and not _windows_can_symlink():
+    if not _windows_can_symlink():
         _windows_create_link(win_source_path, link_path)
     else:
         os.symlink(source_path, link_path, target_is_directory=os.path.isdir(source_path))
 
 
-def islink(path: str) -> bool:
+def _windows_islink(path: str) -> bool:
     """Override os.islink to give correct answer for spack logic.
 
     For Non-Windows: a link can be determined with the os.path.islink method.
@@ -269,7 +244,7 @@ def _windows_create_hard_link(path: str, link: str):
         CreateHardLink(link, path)
 
 
-def readlink(path: str, *, dir_fd=None):
+def _windows_readlink(path: str, *, dir_fd=None):
     """Spack utility to override of os.readlink method to work cross platform"""
     if _windows_is_hardlink(path):
         return _windows_read_hard_link(path)
@@ -338,6 +313,16 @@ def resolve_link_target_relative_to_the_link(link):
     return os.path.join(link_dir, target)
 
 
+if sys.platform == "win32":
+    symlink = _windows_symlink
+    readlink = _windows_readlink
+    islink = _windows_islink
+else:
+    symlink = os.symlink
+    readlink = os.readlink
+    islink = os.path.islink
+
+
 class SymlinkError(RuntimeError):
     """Exception class for errors raised while creating symlinks,
     junctions and hard links

lib/spack/spack/installer.py

@@ -600,9 +600,7 @@ def dump_packages(spec: "spack.spec.Spec", path: str) -> None:
         if node is spec:
             spack.repo.PATH.dump_provenance(node, dest_pkg_dir)
         elif source_pkg_dir:
-            fs.install_tree(
+            fs.install_tree(source_pkg_dir, dest_pkg_dir)
-                source_pkg_dir, dest_pkg_dir, allow_broken_symlinks=(sys.platform != "win32")
-            )
 
 
 def get_dependent_ids(spec: "spack.spec.Spec") -> List[str]:
@@ -2380,9 +2378,7 @@ def _install_source(self) -> None:
         src_target = os.path.join(pkg.spec.prefix, "share", pkg.name, "src")
         tty.debug(f"{self.pre} Copying source to {src_target}")
 
-        fs.install_tree(
+        fs.install_tree(pkg.stage.source_path, src_target)
-            pkg.stage.source_path, src_target, allow_broken_symlinks=(sys.platform != "win32")
-        )
 
     def _real_install(self) -> None:
         import spack.builder

lib/spack/spack/test/llnl/util/filesystem.py

@@ -278,8 +278,8 @@ def test_symlinks_false(self, stage):
     def test_allow_broken_symlinks(self, stage):
         """Test installing with a broken symlink."""
         with fs.working_dir(str(stage)):
-            symlink("nonexistant.txt", "source/broken", allow_broken_symlinks=True)
+            symlink("nonexistant.txt", "source/broken")
-            fs.install_tree("source", "dest", symlinks=True, allow_broken_symlinks=True)
+            fs.install_tree("source", "dest", symlinks=True)
             assert os.path.islink("dest/broken")
             assert not os.path.exists(readlink("dest/broken"))
 

lib/spack/spack/test/llnl/util/symlink.py

@@ -20,7 +20,7 @@ def test_symlink_file(tmpdir):
         fd, real_file = tempfile.mkstemp(prefix="real", suffix=".txt", dir=test_dir)
         link_file = str(tmpdir.join("link.txt"))
         assert os.path.exists(link_file) is False
-        symlink.symlink(source_path=real_file, link_path=link_file)
+        symlink.symlink(real_file, link_file)
         assert os.path.exists(link_file)
         assert symlink.islink(link_file)
 
@@ -32,11 +32,12 @@ def test_symlink_dir(tmpdir):
         real_dir = os.path.join(test_dir, "real_dir")
         link_dir = os.path.join(test_dir, "link_dir")
         os.mkdir(real_dir)
-        symlink.symlink(source_path=real_dir, link_path=link_dir)
+        symlink.symlink(real_dir, link_dir)
         assert os.path.exists(link_dir)
         assert symlink.islink(link_dir)
 
 
+@pytest.mark.skipif(sys.platform != "win32", reason="Test is only for Windows")
 def test_symlink_source_not_exists(tmpdir):
     """Test the symlink.symlink method for the case where a source path does not exist"""
     with tmpdir.as_cwd():
@@ -44,7 +45,7 @@ def test_symlink_source_not_exists(tmpdir):
         real_dir = os.path.join(test_dir, "real_dir")
         link_dir = os.path.join(test_dir, "link_dir")
         with pytest.raises(symlink.SymlinkError):
-            symlink.symlink(source_path=real_dir, link_path=link_dir, allow_broken_symlinks=False)
+            symlink._windows_symlink(real_dir, link_dir)
 
 
 def test_symlink_src_relative_to_link(tmpdir):
@@ -61,18 +62,16 @@ def test_symlink_src_relative_to_link(tmpdir):
         fd, real_file = tempfile.mkstemp(prefix="real", suffix=".txt", dir=subdir_2)
         link_file = os.path.join(subdir_1, "link.txt")
 
-        symlink.symlink(
+        symlink.symlink(f"b/{os.path.basename(real_file)}", f"a/{os.path.basename(link_file)}")
-            source_path=f"b/{os.path.basename(real_file)}",
-            link_path=f"a/{os.path.basename(link_file)}",
-        )
         assert os.path.exists(link_file)
         assert symlink.islink(link_file)
         # Check dirs
         assert not os.path.lexists(link_dir)
-        symlink.symlink(source_path="b", link_path="a/c")
+        symlink.symlink("b", "a/c")
         assert os.path.lexists(link_dir)
 
 
+@pytest.mark.skipif(sys.platform != "win32", reason="Test is only for Windows")
 def test_symlink_src_not_relative_to_link(tmpdir):
     """Test the symlink.symlink functionality where the source value does not exist relative to
     the link and not relative to the cwd. NOTE that this symlink api call is EXPECTED to raise
@@ -88,19 +87,18 @@ def test_symlink_src_not_relative_to_link(tmpdir):
         link_file = str(tmpdir.join("link.txt"))
         # Expected SymlinkError because source path does not exist relative to link path
         with pytest.raises(symlink.SymlinkError):
-            symlink.symlink(
+            symlink._windows_symlink(
-                source_path=f"d/{os.path.basename(real_file)}",
+                f"d/{os.path.basename(real_file)}", f"a/{os.path.basename(link_file)}"
-                link_path=f"a/{os.path.basename(link_file)}",
-                allow_broken_symlinks=False,
             )
         assert not os.path.exists(link_file)
         # Check dirs
         assert not os.path.lexists(link_dir)
         with pytest.raises(symlink.SymlinkError):
-            symlink.symlink(source_path="d", link_path="a/c", allow_broken_symlinks=False)
+            symlink._windows_symlink("d", "a/c")
         assert not os.path.lexists(link_dir)
 
 
+@pytest.mark.skipif(sys.platform != "win32", reason="Test is only for Windows")
 def test_symlink_link_already_exists(tmpdir):
     """Test the symlink.symlink method for the case where a link already exists"""
     with tmpdir.as_cwd():
@@ -108,10 +106,10 @@ def test_symlink_link_already_exists(tmpdir):
         real_dir = os.path.join(test_dir, "real_dir")
         link_dir = os.path.join(test_dir, "link_dir")
         os.mkdir(real_dir)
-        symlink.symlink(real_dir, link_dir, allow_broken_symlinks=False)
+        symlink._windows_symlink(real_dir, link_dir)
         assert os.path.exists(link_dir)
         with pytest.raises(symlink.SymlinkError):
-            symlink.symlink(source_path=real_dir, link_path=link_dir, allow_broken_symlinks=False)
+            symlink._windows_symlink(real_dir, link_dir)
 
 
 @pytest.mark.skipif(not symlink._windows_can_symlink(), reason="Test requires elevated privileges")
@@ -122,7 +120,7 @@ def test_symlink_win_file(tmpdir):
         test_dir = str(tmpdir)
         fd, real_file = tempfile.mkstemp(prefix="real", suffix=".txt", dir=test_dir)
         link_file = str(tmpdir.join("link.txt"))
-        symlink.symlink(source_path=real_file, link_path=link_file)
+        symlink.symlink(real_file, link_file)
         # Verify that all expected conditions are met
         assert os.path.exists(link_file)
         assert symlink.islink(link_file)
@@ -140,7 +138,7 @@ def test_symlink_win_dir(tmpdir):
         real_dir = os.path.join(test_dir, "real")
         link_dir = os.path.join(test_dir, "link")
         os.mkdir(real_dir)
-        symlink.symlink(source_path=real_dir, link_path=link_dir)
+        symlink.symlink(real_dir, link_dir)
         # Verify that all expected conditions are met
         assert os.path.exists(link_dir)
         assert symlink.islink(link_dir)