Windows: fix library loading and enable Clingo bootstrapping (#33400)

Changes to improve locating shared libraries on Windows, which in
turn enables the use of Clingo. This PR attempts to establish a
proper distinction between linking on Windows vs. Linux/Mac: on
Windows, linking is always done with .lib files (never .dll files).
This somewhat complicates the model since the Spec.lib method could
return libraries that were used for both linking and loading, but
since these are not always the same on Windows, it was decided to
treat Spec.libs as being for link-time libraries. Additional functions
are added to help dependents locate run-time libraries.

* Clingo is now the default concretizer on Windows
* Clingo is now the concretizer used for unit tests on Windows
* Fix a permissions issue that can occur while moving Git files during
  fetching/staging
* Packages can now implement "win_add_library_dependent" to register
  files/directories that include libraries that would need to link
  to dependency dlls
* Packages can now implement "win_add_rpath" to register the locations
  of dlls that dependents would want to load
* "Spec.libs" on Windows is updated to return link-time libraries
  (i.e. .lib files, rather than .dll files)
* PackageBase.rpath on Windows is now updated to return the most-likely
  locations where .dlls will be found (which is generally in the bin/
  directory)

.github/workflows/windows_python.yml

@@ -23,7 +23,7 @@ jobs:
         python-version: 3.9
     - name: Install Python packages
       run: |
-          python -m pip install --upgrade pip six pywin32 setuptools codecov pytest-cov
+          python -m pip install --upgrade pip six pywin32 setuptools codecov pytest-cov clingo
     - name: Create local develop
       run: |
         .\spack\.github\workflows\setup_git.ps1
@@ -49,7 +49,7 @@ jobs:
         python-version: 3.9
     - name: Install Python packages
       run: |
-          python -m pip install --upgrade pip six pywin32 setuptools codecov coverage pytest-cov
+          python -m pip install --upgrade pip six pywin32 setuptools codecov coverage pytest-cov clingo
     - name: Create local develop
       run: |
         .\spack\.github\workflows\setup_git.ps1
@@ -81,7 +81,7 @@ jobs:
         echo F|xcopy .\spack\share\spack\qa\configuration\windows_config.yaml $env:USERPROFILE\.spack\windows\config.yaml
         spack external find cmake
         spack external find ninja
-        spack install abseil-cpp
+        spack -d install abseil-cpp
   make-installer:
     runs-on: windows-latest
     steps:

etc/spack/defaults/windows/config.yaml

@@ -1,5 +1,5 @@
 config:
   locks: false
-  concretizer: original
+  concretizer: clingo
   build_stage::
     - '$spack/.staging'

lib/spack/llnl/util/filesystem.py

@@ -1083,7 +1083,11 @@ def temp_cwd():
         with working_dir(tmp_dir):
             yield tmp_dir
     finally:
-        shutil.rmtree(tmp_dir)
+        kwargs = {}
+        if is_windows:
+            kwargs["ignore_errors"] = False
+            kwargs["onerror"] = readonly_file_handler(ignore_errors=True)
+        shutil.rmtree(tmp_dir, **kwargs)
 
 
 @contextmanager
@@ -2095,7 +2099,7 @@ def find_system_libraries(libraries, shared=True):
     return libraries_found
 
 
-def find_libraries(libraries, root, shared=True, recursive=False):
+def find_libraries(libraries, root, shared=True, recursive=False, runtime=True):
     """Returns an iterable of full paths to libraries found in a root dir.
 
     Accepts any glob characters accepted by fnmatch:
@@ -2116,6 +2120,10 @@ def find_libraries(libraries, root, shared=True, recursive=False):
             otherwise for static. Defaults to True.
         recursive (bool): if False search only root folder,
             if True descends top-down from the root. Defaults to False.
+        runtime (bool): Windows only option, no-op elsewhere. If true,
+            search for runtime shared libs (.DLL), otherwise, search
+            for .Lib files. If shared is false, this has no meaning.
+            Defaults to True.
 
     Returns:
         LibraryList: The libraries that have been found
@@ -2130,7 +2138,9 @@ def find_libraries(libraries, root, shared=True, recursive=False):
 
     if is_windows:
         static_ext = "lib"
-        shared_ext = "dll"
+        # For linking (runtime=False) you need the .lib files regardless of
+        # whether you are doing a shared or static link
+        shared_ext = "dll" if runtime else "lib"
     else:
         # Used on both Linux and macOS
         static_ext = "a"
@@ -2174,13 +2184,13 @@ def find_libraries(libraries, root, shared=True, recursive=False):
     return LibraryList(found_libs)
 
 
-def find_all_shared_libraries(root, recursive=False):
+def find_all_shared_libraries(root, recursive=False, runtime=True):
     """Convenience function that returns the list of all shared libraries found
     in the directory passed as argument.
 
     See documentation for `llnl.util.filesystem.find_libraries` for more information
     """
-    return find_libraries("*", root=root, shared=True, recursive=recursive)
+    return find_libraries("*", root=root, shared=True, recursive=recursive, runtime=runtime)
 
 
 def find_all_static_libraries(root, recursive=False):
@@ -2226,48 +2236,36 @@ def __init__(self, package, link_install_prefix=True):
         self.pkg = package
         self._addl_rpaths = set()
         self.link_install_prefix = link_install_prefix
-        self._internal_links = set()
+        self._additional_library_dependents = set()
 
     @property
-    def link_dest(self):
+    def library_dependents(self):
         """
         Set of directories where package binaries/libraries are located.
         """
-        if hasattr(self.pkg, "libs") and self.pkg.libs:
+        return set([self.pkg.prefix.bin]) | self._additional_library_dependents
-            pkg_libs = set(self.pkg.libs.directories)
-        else:
-            pkg_libs = set((self.pkg.prefix.lib, self.pkg.prefix.lib64))
 
-        return pkg_libs | set([self.pkg.prefix.bin]) | self.internal_links
+    def add_library_dependent(self, *dest):
-
-    @property
-    def internal_links(self):
         """
-        linking that would need to be established within the package itself. Useful for links
+        Add paths to directories or libraries/binaries to set of
-        against extension modules/build time executables/internal linkage
+        common paths that need to link against other libraries
-        """
-        return self._internal_links
 
-    def add_internal_links(self, *dest):
+        Specified paths should fall outside of a package's common
-        """
+        link paths, i.e. the bin
-        Incorporate additional paths into the rpath (sym)linking scheme.
-
-        Paths provided to this method are linked against by a package's libraries
-        and libraries found at these paths are linked against a package's binaries.
-        (i.e. /site-packages -> /bin and /bin -> /site-packages)
-
-        Specified paths should be outside of a package's lib, lib64, and bin
         directories.
         """
-        self._internal_links = self._internal_links | set(*dest)
+        for pth in dest:
+            if os.path.isfile(pth):
+                self._additional_library_dependents.add(os.path.dirname)
+            else:
+                self._additional_library_dependents.add(pth)
 
     @property
-    def link_targets(self):
+    def rpaths(self):
         """
         Set of libraries this package needs to link against during runtime
         These packages will each be symlinked into the packages lib and binary dir
         """
-
         dependent_libs = []
         for path in self.pkg.rpath:
             dependent_libs.extend(list(find_all_shared_libraries(path, recursive=True)))
@@ -2275,18 +2273,43 @@ def link_targets(self):
             dependent_libs.extend(list(find_all_shared_libraries(extra_path, recursive=True)))
         return set(dependent_libs)
 
-    def include_additional_link_paths(self, *paths):
+    def add_rpath(self, *paths):
         """
         Add libraries found at the root of provided paths to runtime linking
 
         These are libraries found outside of the typical scope of rpath linking
-        that require manual inclusion in a runtime linking scheme
+        that require manual inclusion in a runtime linking scheme.
+        These links are unidirectional, and are only
+        intended to bring outside dependencies into this package
 
         Args:
             *paths (str): arbitrary number of paths to be added to runtime linking
         """
         self._addl_rpaths = self._addl_rpaths | set(paths)
 
+    def _link(self, path, dest):
+        file_name = os.path.basename(path)
+        dest_file = os.path.join(dest, file_name)
+        if os.path.exists(dest):
+            try:
+                symlink(path, dest_file)
+            # For py2 compatibility, we have to catch the specific Windows error code
+            # associate with trying to create a file that already exists (winerror 183)
+            except OSError as e:
+                if e.winerror == 183:
+                    # We have either already symlinked or we are encoutering a naming clash
+                    # either way, we don't want to overwrite existing libraries
+                    already_linked = islink(dest_file)
+                    tty.debug(
+                        "Linking library %s to %s failed, " % (path, dest_file) + "already linked."
+                        if already_linked
+                        else "library with name %s already exists at location %s."
+                        % (file_name, dest)
+                    )
+                    pass
+                else:
+                    raise e
+
     def establish_link(self):
         """
         (sym)link packages to runtime dependencies based on RPath configuration for
@@ -2298,29 +2321,8 @@ def establish_link(self):
 
         # for each binary install dir in self.pkg (i.e. pkg.prefix.bin, pkg.prefix.lib)
         # install a symlink to each dependent library
-        for library, lib_dir in itertools.product(self.link_targets, self.link_dest):
+        for library, lib_dir in itertools.product(self.rpaths, self.library_dependents):
-            if not path_contains_subdirectory(library, lib_dir):
+            self._link(library, lib_dir)
-                file_name = os.path.basename(library)
-                dest_file = os.path.join(lib_dir, file_name)
-                if os.path.exists(lib_dir):
-                    try:
-                        symlink(library, dest_file)
-                    # For py2 compatibility, we have to catch the specific Windows error code
-                    # associate with trying to create a file that already exists (winerror 183)
-                    except OSError as e:
-                        if e.winerror == 183:
-                            # We have either already symlinked or we are encoutering a naming clash
-                            # either way, we don't want to overwrite existing libraries
-                            already_linked = islink(dest_file)
-                            tty.debug(
-                                "Linking library %s to %s failed, " % (library, dest_file)
-                                + "already linked."
-                                if already_linked
-                                else "library with name %s already exists." % file_name
-                            )
-                            pass
-                        else:
-                            raise e
 
 
 @system_path_filter

lib/spack/spack/fetch_strategy.py

@@ -865,7 +865,12 @@ def clone(self, dest=None, commit=None, branch=None, tag=None, bare=False):
                 repo_name = get_single_file(".")
                 if self.stage:
                     self.stage.srcdir = repo_name
-                shutil.move(repo_name, dest)
+                shutil.copytree(repo_name, dest, symlinks=True)
+                shutil.rmtree(
+                    repo_name,
+                    ignore_errors=False,
+                    onerror=fs.readonly_file_handler(ignore_errors=True),
+                )
 
             with working_dir(dest):
                 checkout_args = ["checkout", commit]

lib/spack/spack/package_base.py

@@ -140,11 +140,30 @@ class WindowsRPathMeta(object):
     they would a genuine RPATH, i.e. adding directories that contain
     runtime library dependencies"""
 
-    def add_search_paths(self, *path):
+    def win_add_library_dependent(self):
-        """Add additional rpaths that are not implicitly included in the search
+        """Return extra set of directories that require linking for package
-        scheme
+
+        This method should be overridden by packages that produce
+        binaries/libraries/python extension modules/etc that are installed into
+        directories outside a package's `bin`, `lib`, and `lib64` directories,
+        but still require linking against one of the packages dependencies, or
+        other components of the package itself. No-op otherwise.
+
+        Returns:
+            List of additional directories that require linking
         """
-        self.win_rpath.include_additional_link_paths(*path)
+        return []
+
+    def win_add_rpath(self):
+        """Return extra set of rpaths for package
+
+        This method should be overridden by packages needing to
+        include additional paths to be searched by rpath. No-op otherwise
+
+        Returns:
+            List of additional rpaths
+        """
+        return []
 
     def windows_establish_runtime_linkage(self):
         """Establish RPATH on Windows
@@ -152,6 +171,8 @@ def windows_establish_runtime_linkage(self):
         Performs symlinking to incorporate rpath dependencies to Windows runtime search paths
         """
         if is_windows:
+            self.win_rpath.add_library_dependent(*self.win_add_library_dependent())
+            self.win_rpath.add_rpath(*self.win_add_rpath())
             self.win_rpath.establish_link()
 
 
@@ -2571,12 +2592,17 @@ def fetch_remote_versions(self, concurrency=128):
     @property
     def rpath(self):
         """Get the rpath this package links with, as a list of paths."""
-        rpaths = [self.prefix.lib, self.prefix.lib64]
         deps = self.spec.dependencies(deptype="link")
-        rpaths.extend(d.prefix.lib for d in deps if os.path.isdir(d.prefix.lib))
+
-        rpaths.extend(d.prefix.lib64 for d in deps if os.path.isdir(d.prefix.lib64))
+        # on Windows, libraries of runtime interest are typically
+        # stored in the bin directory
         if is_windows:
+            rpaths = [self.prefix.bin]
             rpaths.extend(d.prefix.bin for d in deps if os.path.isdir(d.prefix.bin))
+        else:
+            rpaths = [self.prefix.lib, self.prefix.lib64]
+            rpaths.extend(d.prefix.lib for d in deps if os.path.isdir(d.prefix.lib))
+            rpaths.extend(d.prefix.lib64 for d in deps if os.path.isdir(d.prefix.lib64))
         return rpaths
 
     @property

lib/spack/spack/spec.py

@@ -1030,7 +1030,9 @@ def _libs_default_handler(descriptor, spec, cls):
     )
 
     for shared in search_shared:
-        libs = fs.find_libraries(name, home, shared=shared, recursive=True)
+        # Since we are searching for link libraries, on Windows search only for
+        # ".Lib" extensions by default as those represent import libraries for implict links.
+        libs = fs.find_libraries(name, home, shared=shared, recursive=True, runtime=False)
         if libs:
             return libs
 

lib/spack/spack/test/concretize.py

@@ -2,7 +2,7 @@
 # Spack Project Developers. See the top-level COPYRIGHT file for details.
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
-import os
+import posixpath
 import sys
 
 import jinja2
@@ -459,7 +459,7 @@ def test_compiler_inheritance(self, compiler_str):
     def test_external_package(self):
         spec = Spec("externaltool%gcc")
         spec.concretize()
-        assert spec["externaltool"].external_path == os.path.sep + os.path.join(
+        assert spec["externaltool"].external_path == posixpath.sep + posixpath.join(
             "path", "to", "external_tool"
         )
         assert "externalprereq" not in spec
@@ -490,10 +490,10 @@ def test_nobuild_package(self):
     def test_external_and_virtual(self):
         spec = Spec("externaltest")
         spec.concretize()
-        assert spec["externaltool"].external_path == os.path.sep + os.path.join(
+        assert spec["externaltool"].external_path == posixpath.sep + posixpath.join(
             "path", "to", "external_tool"
         )
-        assert spec["stuff"].external_path == os.path.sep + os.path.join(
+        assert spec["stuff"].external_path == posixpath.sep + posixpath.join(
             "path", "to", "external_virtual_gcc"
         )
         assert spec["externaltool"].compiler.satisfies("gcc")

var/spack/repos/builtin/packages/clingo/package.py

@@ -3,6 +3,7 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
+import os
 
 from spack.compiler import UnsupportedCompilerFlag
 from spack.package import *
@@ -120,3 +121,9 @@ def cmake_args(self):
             args += ["-DCLINGO_BUILD_WITH_PYTHON=OFF"]
 
         return args
+
+    def win_add_library_dependent(self):
+        if "+python" in self.spec:
+            return [os.path.join(self.prefix, self.spec["python"].package.platlib)]
+        else:
+            return []