From 222cef9b7c6efb30c7f100d33e1d809e3dc1007d Mon Sep 17 00:00:00 2001 From: "John W. Parent" <45471568+johnwparent@users.noreply.github.com> Date: Mon, 31 Oct 2022 12:36:52 -0400 Subject: [PATCH] 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 | 6 +- etc/spack/defaults/windows/config.yaml | 2 +- lib/spack/llnl/util/filesystem.py | 114 +++++++++--------- lib/spack/spack/fetch_strategy.py | 7 +- lib/spack/spack/package_base.py | 40 ++++-- lib/spack/spack/spec.py | 4 +- lib/spack/spack/test/concretize.py | 8 +- .../repos/builtin/packages/clingo/package.py | 7 ++ 8 files changed, 115 insertions(+), 73 deletions(-) diff --git a/.github/workflows/windows_python.yml b/.github/workflows/windows_python.yml index f1b828ceaa2..7e6caa0189d 100644 --- a/.github/workflows/windows_python.yml +++ b/.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: diff --git a/etc/spack/defaults/windows/config.yaml b/etc/spack/defaults/windows/config.yaml index 956bc97c20a..367bf831cff 100644 --- a/etc/spack/defaults/windows/config.yaml +++ b/etc/spack/defaults/windows/config.yaml @@ -1,5 +1,5 @@ config: locks: false - concretizer: original + concretizer: clingo build_stage:: - '$spack/.staging' diff --git a/lib/spack/llnl/util/filesystem.py b/lib/spack/llnl/util/filesystem.py index 511f7c4f598..fa1188c133e 100644 --- a/lib/spack/llnl/util/filesystem.py +++ b/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: - pkg_libs = set(self.pkg.libs.directories) - else: - pkg_libs = set((self.pkg.prefix.lib, self.pkg.prefix.lib64)) + return set([self.pkg.prefix.bin]) | self._additional_library_dependents - return pkg_libs | set([self.pkg.prefix.bin]) | self.internal_links - - @property - def internal_links(self): + def add_library_dependent(self, *dest): """ - linking that would need to be established within the package itself. Useful for links - against extension modules/build time executables/internal linkage - """ - return self._internal_links + Add paths to directories or libraries/binaries to set of + common paths that need to link against other libraries - def add_internal_links(self, *dest): - """ - 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 + Specified paths should fall outside of a package's common + link paths, i.e. the 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): - if not path_contains_subdirectory(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 + for library, lib_dir in itertools.product(self.rpaths, self.library_dependents): + self._link(library, lib_dir) @system_path_filter diff --git a/lib/spack/spack/fetch_strategy.py b/lib/spack/spack/fetch_strategy.py index ea85c6a6824..df993978af6 100644 --- a/lib/spack/spack/fetch_strategy.py +++ b/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] diff --git a/lib/spack/spack/package_base.py b/lib/spack/spack/package_base.py index 3c3bcaa84db..cf96beca580 100644 --- a/lib/spack/spack/package_base.py +++ b/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): - """Add additional rpaths that are not implicitly included in the search - scheme + def win_add_library_dependent(self): + """Return extra set of directories that require linking for package + + 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 diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py index 2f4120a5f84..a05451b7b41 100644 --- a/lib/spack/spack/spec.py +++ b/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 diff --git a/lib/spack/spack/test/concretize.py b/lib/spack/spack/test/concretize.py index ccdc19a1743..ffc05a7fc0e 100644 --- a/lib/spack/spack/test/concretize.py +++ b/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") diff --git a/var/spack/repos/builtin/packages/clingo/package.py b/var/spack/repos/builtin/packages/clingo/package.py index a3d39d8e629..a24ffb0956d 100644 --- a/var/spack/repos/builtin/packages/clingo/package.py +++ b/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 []