From f51c9fc6c3fcfcf0144c1813c1ce84391e46e773 Mon Sep 17 00:00:00 2001 From: "John W. Parent" <45471568+johnwparent@users.noreply.github.com> Date: Fri, 23 Feb 2024 16:30:11 -0500 Subject: [PATCH] Windows path handling: change representation for paths with spaces (#42754) Some builds on Windows break when encountering paths with spaces. This reencodes some paths in Windows 8.3 filename format (when on Windows): this serves as an equivalent identifier for the file, but in a form that does not have spaces. 8.3 filenames are also truncated in length, which could be helpful, but that is not the primary intended purpose of using this format. Overall * nmake/msbuild packages do this generally for the install prefix * curl/perl require additional modifications (as written now, each package may require calls to `windows_sfn` to work when the Spack root/install/staging prefixes contain spaces) Some items for follow-up: * Spack itself does not create paths with spaces "on top" of whatever the user configures or where it is placed (e.g. the Spack root, the staging directory, etc.), so it might be possible to edit some of these paths once and avoid a proliferation of individual `windows_sfn` calls in individual packages. * This approach may result in the insertion of 8.3-style paths into build artifacts (on Windows), handling this may require additional bookkeeping (e.g. when relocating). --- lib/spack/llnl/util/filesystem.py | 41 +++++++++++++++++++ lib/spack/spack/build_systems/msbuild.py | 2 +- lib/spack/spack/build_systems/nmake.py | 6 ++- .../repos/builtin/packages/curl/package.py | 6 ++- .../repos/builtin/packages/perl/package.py | 6 ++- 5 files changed, 57 insertions(+), 4 deletions(-) diff --git a/lib/spack/llnl/util/filesystem.py b/lib/spack/llnl/util/filesystem.py index b13fc00bd09..a9cfc10091f 100644 --- a/lib/spack/llnl/util/filesystem.py +++ b/lib/spack/llnl/util/filesystem.py @@ -1240,6 +1240,47 @@ def get_single_file(directory): return fnames[0] +@system_path_filter +def windows_sfn(path: os.PathLike): + """Returns 8.3 Filename (SFN) representation of + path + + 8.3 Filenames (SFN or short filename) is a file + naming convention used prior to Win95 that Windows + still (and will continue to) support. This convention + caps filenames at 8 characters, and most importantly + does not allow for spaces in addition to other specifications. + The scheme is generally the same as a normal Windows + file scheme, but all spaces are removed and the filename + is capped at 6 characters. The remaining characters are + replaced with ~N where N is the number file in a directory + that a given file represents i.e. Program Files and Program Files (x86) + would be PROGRA~1 and PROGRA~2 respectively. + Further, all file/directory names are all caps (although modern Windows + is case insensitive in practice). + Conversion is accomplished by fileapi.h GetShortPathNameW + + Returns paths in 8.3 Filename form + + Note: this method is a no-op on Linux + + Args: + path: Path to be transformed into SFN (8.3 filename) format + """ + # This should not be run-able on linux/macos + if sys.platform != "win32": + return path + path = str(path) + import ctypes + + k32 = ctypes.WinDLL("kernel32", use_last_error=True) + # stub Windows types TCHAR[LENGTH] + TCHAR_arr = ctypes.c_wchar * len(path) + ret_str = TCHAR_arr() + k32.GetShortPathNameW(path, ret_str, len(path)) + return ret_str.value + + @contextmanager def temp_cwd(): tmp_dir = tempfile.mkdtemp() diff --git a/lib/spack/spack/build_systems/msbuild.py b/lib/spack/spack/build_systems/msbuild.py index 4de8c86e0ac..1520ebc21d0 100644 --- a/lib/spack/spack/build_systems/msbuild.py +++ b/lib/spack/spack/build_systems/msbuild.py @@ -69,7 +69,7 @@ class MSBuildBuilder(BaseBuilder): @property def build_directory(self): """Return the directory containing the MSBuild solution or vcxproj.""" - return self.pkg.stage.source_path + return fs.windows_sfn(self.pkg.stage.source_path) @property def toolchain_version(self): diff --git a/lib/spack/spack/build_systems/nmake.py b/lib/spack/spack/build_systems/nmake.py index b9148dcd9fb..f8823548dee 100644 --- a/lib/spack/spack/build_systems/nmake.py +++ b/lib/spack/spack/build_systems/nmake.py @@ -77,7 +77,11 @@ def ignore_quotes(self): @property def build_directory(self): """Return the directory containing the makefile.""" - return self.pkg.stage.source_path if not self.makefile_root else self.makefile_root + return ( + fs.windows_sfn(self.pkg.stage.source_path) + if not self.makefile_root + else fs.windows_sfn(self.makefile_root) + ) @property def std_nmake_args(self): diff --git a/var/spack/repos/builtin/packages/curl/package.py b/var/spack/repos/builtin/packages/curl/package.py index 62a4d1bef83..a093786b22b 100644 --- a/var/spack/repos/builtin/packages/curl/package.py +++ b/var/spack/repos/builtin/packages/curl/package.py @@ -8,6 +8,8 @@ import re import sys +from llnl.util.filesystem import windows_sfn + from spack.build_systems.autotools import AutotoolsBuilder from spack.build_systems.nmake import NMakeBuilder from spack.package import * @@ -470,7 +472,8 @@ def nmake_args(self): # The trailing path seperator is REQUIRED for cURL to install # otherwise cURLs build system will interpret the path as a file # and the install will fail with ambiguous errors - args.append("WITH_PREFIX=%s" % self.prefix + "\\") + inst_prefix = self.prefix + "\\" + args.append(f"WITH_PREFIX={windows_sfn(inst_prefix)}") return args def install(self, pkg, spec, prefix): @@ -485,6 +488,7 @@ def install(self, pkg, spec, prefix): env["CC"] = "" env["CXX"] = "" winbuild_dir = os.path.join(self.stage.source_path, "winbuild") + winbuild_dir = windows_sfn(winbuild_dir) with working_dir(winbuild_dir): nmake("/f", "Makefile.vc", *self.nmake_args(), ignore_quotes=True) with working_dir(os.path.join(self.stage.source_path, "builds")): diff --git a/var/spack/repos/builtin/packages/perl/package.py b/var/spack/repos/builtin/packages/perl/package.py index aea185f73df..ad44d760f3e 100644 --- a/var/spack/repos/builtin/packages/perl/package.py +++ b/var/spack/repos/builtin/packages/perl/package.py @@ -16,6 +16,7 @@ import sys from contextlib import contextmanager +from llnl.util.filesystem import windows_sfn from llnl.util.lang import match_predicate from llnl.util.symlink import symlink @@ -287,7 +288,7 @@ def nmake_arguments(self): args.append("CCTYPE=%s" % self.compiler.short_msvc_version) else: raise RuntimeError("Perl unsupported for non MSVC compilers on Windows") - args.append("INST_TOP=%s" % self.prefix.replace("/", "\\")) + args.append("INST_TOP=%s" % windows_sfn(self.prefix.replace("/", "\\"))) args.append("INST_ARCH=\\$(ARCHNAME)") if self.spec.satisfies("~shared"): args.append("ALL_STATIC=%s" % "define") @@ -368,6 +369,7 @@ def build(self, spec, prefix): def build_test(self): if sys.platform == "win32": win32_dir = os.path.join(self.stage.source_path, "win32") + win32_dir = windows_sfn(win32_dir) with working_dir(win32_dir): nmake("test", ignore_quotes=True) else: @@ -376,6 +378,7 @@ def build_test(self): def install(self, spec, prefix): if sys.platform == "win32": win32_dir = os.path.join(self.stage.source_path, "win32") + win32_dir = windows_sfn(win32_dir) with working_dir(win32_dir): nmake("install", *self.nmake_arguments(), ignore_quotes=True) else: @@ -409,6 +412,7 @@ def install_cpanm(self): if sys.platform == "win32": maker = nmake cpan_dir = join_path(self.stage.source_path, cpan_dir) + cpan_dir = windows_sfn(cpan_dir) if "+cpanm" in spec: with working_dir(cpan_dir): perl = spec["perl"].command