External finding: update default paths; treat .bat as executable on Windows (#39850)

.bat or .exe files can be considered executable on Windows. This PR
expands the regex for detectable packages to allow for the detection
of packages that vendor .bat wrappers (intel mpi for example).

Additional changes:

* Outside of Windows, when searching for executables `path_hints=None`
  was used to indicate that default path hints should be provided,
  and `[]` was taken to mean that no defaults should be chosen
  (in that case, nothing is searched); behavior on Windows has
  now been updated to match.
* Above logic for handling of `path_hints=[]`  has also been extended
  to library search (for both Linux and Windows).
* All exceptions for external packages were documented as timeout
  errors: this commit adds a distinction for other types of errors
  in warning messages to the user.

4 files changed, 89 insertions, 82 deletions

diff --git a/lib/spack/spack/detection/path.py b/lib/spack/spack/detection/path.py
index 4de703ac97..6531ed62da 100644
--- a/lib/spack/spack/detection/path.py
+++ b/lib/spack/spack/detection/path.py
@@ -39,12 +39,21 @@ if sys.platform == "win32":
     DETECTION_TIMEOUT = 120
 
 
-def common_windows_package_paths() -> List[str]:
+def common_windows_package_paths(pkg_cls=None) -> List[str]:
+    """Get the paths for common package installation location on Windows
+    that are outside the PATH
+    Returns [] on unix
+    """
+    if sys.platform != "win32":
+        return []
     paths = WindowsCompilerExternalPaths.find_windows_compiler_bundled_packages()
     paths.extend(find_win32_additional_install_paths())
     paths.extend(WindowsKitExternalPaths.find_windows_kit_bin_paths())
     paths.extend(WindowsKitExternalPaths.find_windows_kit_reg_installed_roots_paths())
     paths.extend(WindowsKitExternalPaths.find_windows_kit_reg_sdk_paths())
+    if pkg_cls:
+        paths.extend(compute_windows_user_path_for_package(pkg_cls))
+        paths.extend(compute_windows_program_path_for_package(pkg_cls))
     return paths
 
 
@@ -62,8 +71,6 @@ def executables_in_path(path_hints: List[str]) -> Dict[str, str]:
         path_hints: list of paths to be searched. If None the list will be
             constructed based on the PATH environment variable.
     """
-    if sys.platform == "win32":
-        path_hints.extend(common_windows_package_paths())
     search_paths = llnl.util.filesystem.search_paths_for_executables(*path_hints)
     return path_to_dict(search_paths)
 
@@ -88,30 +95,42 @@ def libraries_in_ld_and_system_library_path(
             DYLD_LIBRARY_PATH, and DYLD_FALLBACK_LIBRARY_PATH environment
             variables as well as the standard system library paths.
     """
-    path_hints = (
-        path_hints
-        or spack.util.environment.get_path("LD_LIBRARY_PATH")
+    default_lib_search_paths = (
+        spack.util.environment.get_path("LD_LIBRARY_PATH")
         + spack.util.environment.get_path("DYLD_LIBRARY_PATH")
         + spack.util.environment.get_path("DYLD_FALLBACK_LIBRARY_PATH")
         + spack.util.ld_so_conf.host_dynamic_linker_search_paths()
     )
+    path_hints = path_hints if path_hints is not None else default_lib_search_paths
+
     search_paths = llnl.util.filesystem.search_paths_for_libraries(*path_hints)
     return path_to_dict(search_paths)
 
 
-def libraries_in_windows_paths(path_hints: List[str]) -> Dict[str, str]:
-    path_hints.extend(spack.util.environment.get_path("PATH"))
-    search_paths = llnl.util.filesystem.search_paths_for_libraries(*path_hints)
+def libraries_in_windows_paths(path_hints: Optional[List[str]] = None) -> Dict[str, str]:
+    """Get the paths of all libraries available from the system PATH paths.
+
+    For more details, see `libraries_in_ld_and_system_library_path` regarding
+    return type and contents.
+
+    Args:
+        path_hints: list of paths to be searched. If None the list will be
+            constructed based on the set of PATH environment
+            variables as well as the standard system library paths.
+    """
+    search_hints = (
+        path_hints if path_hints is not None else spack.util.environment.get_path("PATH")
+    )
+    search_paths = llnl.util.filesystem.search_paths_for_libraries(*search_hints)
     # on Windows, some libraries (.dlls) are found in the bin directory or sometimes
     # at the search root. Add both of those options to the search scheme
-    search_paths.extend(llnl.util.filesystem.search_paths_for_executables(*path_hints))
-    search_paths.extend(WindowsKitExternalPaths.find_windows_kit_lib_paths())
-    search_paths.extend(WindowsKitExternalPaths.find_windows_kit_bin_paths())
-    search_paths.extend(WindowsKitExternalPaths.find_windows_kit_reg_installed_roots_paths())
-    search_paths.extend(WindowsKitExternalPaths.find_windows_kit_reg_sdk_paths())
-    # SDK and WGL should be handled by above, however on occasion the WDK is in an atypical
-    # location, so we handle that case specifically.
-    search_paths.extend(WindowsKitExternalPaths.find_windows_driver_development_kit_paths())
+    search_paths.extend(llnl.util.filesystem.search_paths_for_executables(*search_hints))
+    if path_hints is None:
+        # if no user provided path was given, add defaults to the search
+        search_paths.extend(WindowsKitExternalPaths.find_windows_kit_lib_paths())
+        # SDK and WGL should be handled by above, however on occasion the WDK is in an atypical
+        # location, so we handle that case specifically.
+        search_paths.extend(WindowsKitExternalPaths.find_windows_driver_development_kit_paths())
     return path_to_dict(search_paths)
 
 
@@ -125,19 +144,8 @@ def _group_by_prefix(paths: Set[str]) -> Dict[str, Set[str]]:
 class Finder:
     """Inspects the file-system looking for packages. Guesses places where to look using PATH."""
 
-    def path_hints(
-        self, *, pkg: "spack.package_base.PackageBase", initial_guess: Optional[List[str]] = None
-    ) -> List[str]:
-        """Returns the list of paths to be searched.
-
-        Args:
-            pkg: package being detected
-            initial_guess: initial list of paths from caller
-        """
-        result = initial_guess or []
-        result.extend(compute_windows_user_path_for_package(pkg))
-        result.extend(compute_windows_program_path_for_package(pkg))
-        return result
+    def default_path_hints(self) -> List[str]:
+        return []
 
     def search_patterns(self, *, pkg: "spack.package_base.PackageBase") -> List[str]:
         """Returns the list of patterns used to match candidate files.
@@ -245,6 +253,8 @@ class Finder:
         Args:
             pkg_name: package being detected
             initial_guess: initial list of paths to search from the caller
+                           if None, default paths are searched. If this
+                           is an empty list, nothing will be searched.
         """
         import spack.repo
 
@@ -252,13 +262,18 @@ class Finder:
         patterns = self.search_patterns(pkg=pkg_cls)
         if not patterns:
             return []
-        path_hints = self.path_hints(pkg=pkg_cls, initial_guess=initial_guess)
-        candidates = self.candidate_files(patterns=patterns, paths=path_hints)
+        if initial_guess is None:
+            initial_guess = self.default_path_hints()
+            initial_guess.extend(common_windows_package_paths(pkg_cls))
+        candidates = self.candidate_files(patterns=patterns, paths=initial_guess)
         result = self.detect_specs(pkg=pkg_cls, paths=candidates)
         return result
 
 
 class ExecutablesFinder(Finder):
+    def default_path_hints(self) -> List[str]:
+        return spack.util.environment.get_path("PATH")
+
     def search_patterns(self, *, pkg: "spack.package_base.PackageBase") -> List[str]:
         result = []
         if hasattr(pkg, "executables") and hasattr(pkg, "platform_executables"):
@@ -298,7 +313,7 @@ class LibrariesFinder(Finder):
         libraries_by_path = (
             libraries_in_ld_and_system_library_path(path_hints=paths)
             if sys.platform != "win32"
-            else libraries_in_windows_paths(paths)
+            else libraries_in_windows_paths(path_hints=paths)
         )
         patterns = [re.compile(x) for x in patterns]
         result = []
@@ -334,21 +349,16 @@ def by_path(
     # TODO: Packages should be able to define both .libraries and .executables in the future
     # TODO: determine_spec_details should get all relevant libraries and executables in one call
     executables_finder, libraries_finder = ExecutablesFinder(), LibrariesFinder()
-
-    executables_path_guess = (
-        spack.util.environment.get_path("PATH") if path_hints is None else path_hints
-    )
-    libraries_path_guess = [] if path_hints is None else path_hints
     detected_specs_by_package: Dict[str, Tuple[concurrent.futures.Future, ...]] = {}
 
     result = collections.defaultdict(list)
     with concurrent.futures.ProcessPoolExecutor(max_workers=max_workers) as executor:
         for pkg in packages_to_search:
             executable_future = executor.submit(
-                executables_finder.find, pkg_name=pkg, initial_guess=executables_path_guess
+                executables_finder.find, pkg_name=pkg, initial_guess=path_hints
             )
             library_future = executor.submit(
-                libraries_finder.find, pkg_name=pkg, initial_guess=libraries_path_guess
+                libraries_finder.find, pkg_name=pkg, initial_guess=path_hints
             )
             detected_specs_by_package[pkg] = executable_future, library_future
 
@@ -359,9 +369,13 @@ def by_path(
                     if detected:
                         _, unqualified_name = spack.repo.partition_package_name(pkg_name)
                         result[unqualified_name].extend(detected)
-                except Exception:
+                except concurrent.futures.TimeoutError:
                     llnl.util.tty.debug(
                         f"[EXTERNAL DETECTION] Skipping {pkg_name}: timeout reached"
                     )
+                except Exception as e:
+                    llnl.util.tty.debug(
+                        f"[EXTERNAL DETECTION] Skipping {pkg_name}: exception occured {e}"
+                    )
 
     return result
diff --git a/lib/spack/spack/test/cmd/external.py b/lib/spack/spack/test/cmd/external.py
index e94d6efe5c..e9a387aac0 100644
--- a/lib/spack/spack/test/cmd/external.py
+++ b/lib/spack/spack/test/cmd/external.py
@@ -28,21 +28,12 @@ def executables_found(monkeypatch):
     return _factory
 
 
-@pytest.fixture
-def _platform_executables(monkeypatch):
-    def _win_exe_ext():
-        return ".bat"
-
-    monkeypatch.setattr(spack.util.path, "win_exe_ext", _win_exe_ext)
-
-
 def define_plat_exe(exe):
     if sys.platform == "win32":
         exe += ".bat"
     return exe
 
 
-@pytest.mark.xfail(sys.platform == "win32", reason="https://github.com/spack/spack/pull/39850")
 def test_find_external_single_package(mock_executable):
     cmake_path = mock_executable("cmake", output="echo cmake version 1.foo")
     search_dir = cmake_path.parent.parent
@@ -54,7 +45,7 @@ def test_find_external_single_package(mock_executable):
     assert len(detected_spec) == 1 and detected_spec[0].spec == Spec("cmake@1.foo")
 
 
-def test_find_external_two_instances_same_package(mock_executable, _platform_executables):
+def test_find_external_two_instances_same_package(mock_executable):
     # Each of these cmake instances is created in a different prefix
     # In Windows, quoted strings are echo'd with quotes includes
     # we need to avoid that for proper regex.
@@ -236,32 +227,7 @@ def test_list_detectable_packages(mutable_config, mutable_mock_repo):
     assert external.returncode == 0
 
 
-@pytest.mark.xfail(sys.platform == "win32", reason="https://github.com/spack/spack/pull/39850")
-def test_packages_yaml_format(mock_executable, mutable_config, monkeypatch, _platform_executables):
-    # Prepare an environment to detect a fake gcc
-    gcc_exe = mock_executable("gcc", output="echo 4.2.1")
-    prefix = os.path.dirname(gcc_exe)
-    monkeypatch.setenv("PATH", prefix)
-
-    # Find the external spec
-    external("find", "gcc")
-
-    # Check entries in 'packages.yaml'
-    packages_yaml = spack.config.get("packages")
-    assert "gcc" in packages_yaml
-    assert "externals" in packages_yaml["gcc"]
-    externals = packages_yaml["gcc"]["externals"]
-    assert len(externals) == 1
-    external_gcc = externals[0]
-    assert external_gcc["spec"] == "gcc@4.2.1 languages=c"
-    assert external_gcc["prefix"] == os.path.dirname(prefix)
-    assert "extra_attributes" in external_gcc
-    extra_attributes = external_gcc["extra_attributes"]
-    assert "prefix" not in extra_attributes
-    assert extra_attributes["compilers"]["c"] == str(gcc_exe)
-
-
-def test_overriding_prefix(mock_executable, mutable_config, monkeypatch, _platform_executables):
+def test_overriding_prefix(mock_executable, mutable_config, monkeypatch):
     gcc_exe = mock_executable("gcc", output="echo 4.2.1")
     search_dir = gcc_exe.parent
 
@@ -282,10 +248,7 @@ def test_overriding_prefix(mock_executable, mutable_config, monkeypatch, _platfo
     assert gcc.external_path == os.path.sep + os.path.join("opt", "gcc", "bin")
 
 
-@pytest.mark.xfail(sys.platform == "win32", reason="https://github.com/spack/spack/pull/39850")
-def test_new_entries_are_reported_correctly(
-    mock_executable, mutable_config, monkeypatch, _platform_executables
-):
+def test_new_entries_are_reported_correctly(mock_executable, mutable_config, monkeypatch):
     # Prepare an environment to detect a fake gcc
     gcc_exe = mock_executable("gcc", output="echo 4.2.1")
     prefix = os.path.dirname(gcc_exe)
diff --git a/lib/spack/spack/test/detection.py b/lib/spack/spack/test/detection.py
new file mode 100644
index 0000000000..6218bc8757
--- /dev/null
+++ b/lib/spack/spack/test/detection.py
@@ -0,0 +1,30 @@
+# Copyright 2013-2023 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+import collections
+
+import spack.detection
+import spack.spec
+
+
+def test_detection_update_config(mutable_config):
+    # mock detected package
+    detected_packages = collections.defaultdict(list)
+    detected_packages["cmake"] = [
+        spack.detection.common.DetectedPackage(
+            spec=spack.spec.Spec("cmake@3.27.5"), prefix="/usr/bin"
+        )
+    ]
+
+    # update config for new package
+    spack.detection.common.update_configuration(detected_packages)
+    # Check entries in 'packages.yaml'
+    packages_yaml = spack.config.get("packages")
+    assert "cmake" in packages_yaml
+    assert "externals" in packages_yaml["cmake"]
+    externals = packages_yaml["cmake"]["externals"]
+    assert len(externals) == 1
+    external_gcc = externals[0]
+    assert external_gcc["spec"] == "cmake@3.27.5"
+    assert external_gcc["prefix"] == "/usr/bin"
diff --git a/lib/spack/spack/util/path.py b/lib/spack/spack/util/path.py
index a46443c083..e2aee48df1 100644
--- a/lib/spack/spack/util/path.py
+++ b/lib/spack/spack/util/path.py
@@ -98,7 +98,7 @@ SPACK_PATH_PADDING_CHARS = "__spack_path_placeholder__"
 
 
 def win_exe_ext():
-    return ".exe"
+    return r"(?:\.bat|\.exe)"
 
 
 def sanitize_filename(filename: str) -> str: