Move detection logic in its own package (#26119)

The logic to perform detection of already installed
packages has been extracted from cmd/external.py
and put into the spack.detection package.

In this way it can be reused programmatically for
other purposes, like bootstrapping.

The new implementation accounts for cases where the
executables are placed in a subdirectory within <prefix>/bin

lib/spack/spack/cmd/external.py

@@ -5,23 +5,17 @@
 from __future__ import print_function
 
 import argparse
-import os
-import re
 import sys
-from collections import defaultdict, namedtuple
 
-import six
-
-import llnl.util.filesystem
 import llnl.util.tty as tty
 import llnl.util.tty.colify as colify
 
 import spack
 import spack.cmd
 import spack.cmd.common.arguments
+import spack.detection
 import spack.error
 import spack.util.environment
-import spack.util.spack_yaml as syaml
 
 description = "manage external packages in Spack configuration"
 section = "config"
@@ -53,104 +47,6 @@ def setup_parser(subparser):
     )
 
 
-def is_executable(path):
-    return os.path.isfile(path) and os.access(path, os.X_OK)
-
-
-def _get_system_executables():
-    """Get the paths of all executables available from the current PATH.
-
-    For convenience, this is constructed as a dictionary where the keys are
-    the executable paths and the values are the names of the executables
-    (i.e. the basename of the executable path).
-
-    There may be multiple paths with the same basename. In this case it is
-    assumed there are two different instances of the executable.
-    """
-    path_hints = spack.util.environment.get_path('PATH')
-    search_paths = llnl.util.filesystem.search_paths_for_executables(
-        *path_hints)
-
-    path_to_exe = {}
-    # Reverse order of search directories so that an exe in the first PATH
-    # entry overrides later entries
-    for search_path in reversed(search_paths):
-        for exe in os.listdir(search_path):
-            exe_path = os.path.join(search_path, exe)
-            if is_executable(exe_path):
-                path_to_exe[exe_path] = exe
-    return path_to_exe
-
-
-ExternalPackageEntry = namedtuple(
-    'ExternalPackageEntry',
-    ['spec', 'base_dir'])
-
-
-def _generate_pkg_config(external_pkg_entries):
-    """Generate config according to the packages.yaml schema for a single
-    package.
-
-    This does not generate the entire packages.yaml. For example, given some
-    external entries for the CMake package, this could return::
-
-        {
-            'externals': [{
-                'spec': 'cmake@3.17.1',
-                'prefix': '/opt/cmake-3.17.1/'
-            }, {
-                'spec': 'cmake@3.16.5',
-                'prefix': '/opt/cmake-3.16.5/'
-            }]
-       }
-    """
-
-    pkg_dict = syaml.syaml_dict()
-    pkg_dict['externals'] = []
-    for e in external_pkg_entries:
-        if not _spec_is_valid(e.spec):
-            continue
-
-        external_items = [('spec', str(e.spec)), ('prefix', e.base_dir)]
-        if e.spec.external_modules:
-            external_items.append(('modules', e.spec.external_modules))
-
-        if e.spec.extra_attributes:
-            external_items.append(
-                ('extra_attributes',
-                 syaml.syaml_dict(e.spec.extra_attributes.items()))
-            )
-
-        # external_items.extend(e.spec.extra_attributes.items())
-        pkg_dict['externals'].append(
-            syaml.syaml_dict(external_items)
-        )
-
-    return pkg_dict
-
-
-def _spec_is_valid(spec):
-    try:
-        str(spec)
-    except spack.error.SpackError:
-        # It is assumed here that we can at least extract the package name from
-        # the spec so we can look up the implementation of
-        # determine_spec_details
-        tty.warn('Constructed spec for {0} does not have a string'
-                 ' representation'.format(spec.name))
-        return False
-
-    try:
-        spack.spec.Spec(str(spec))
-    except spack.error.SpackError:
-        tty.warn('Constructed spec has a string representation but the string'
-                 ' representation does not evaluate to a valid spec: {0}'
-                 .format(str(spec)))
-        return False
-
-    return True
-
-
 def external_find(args):
     # Construct the list of possible packages to be detected
     packages_to_check = []
@@ -176,9 +72,9 @@ def external_find(args):
     if not args.tags and not packages_to_check:
         packages_to_check = spack.repo.path.all_packages()
 
-    pkg_to_entries = _get_external_packages(packages_to_check)
+    detected_packages = spack.detection.by_executable(packages_to_check)
-    new_entries = _update_pkg_config(
+    new_entries = spack.detection.update_configuration(
-        args.scope, pkg_to_entries, args.not_buildable
+        detected_packages, scope=args.scope, buildable=not args.not_buildable
     )
     if new_entries:
         path = spack.config.config.get_config_filename(args.scope, 'packages')
@@ -190,163 +86,6 @@ def external_find(args):
         tty.msg('No new external packages detected')
 
 
-def _group_by_prefix(paths):
-    groups = defaultdict(set)
-    for p in paths:
-        groups[os.path.dirname(p)].add(p)
-    return groups.items()
-
-
-def _convert_to_iterable(single_val_or_multiple):
-    x = single_val_or_multiple
-    if x is None:
-        return []
-    elif isinstance(x, six.string_types):
-        return [x]
-    elif isinstance(x, spack.spec.Spec):
-        # Specs are iterable, but a single spec should be converted to a list
-        return [x]
-
-    try:
-        iter(x)
-        return x
-    except TypeError:
-        return [x]
-
-
-def _determine_base_dir(prefix):
-    # Given a prefix where an executable is found, assuming that prefix ends
-    # with /bin/, strip off the 'bin' directory to get a Spack-compatible
-    # prefix
-    assert os.path.isdir(prefix)
-    if os.path.basename(prefix) == 'bin':
-        return os.path.dirname(prefix)
-
-
-def _get_predefined_externals():
-    # Pull from all scopes when looking for preexisting external package
-    # entries
-    pkg_config = spack.config.get('packages')
-    already_defined_specs = set()
-    for pkg_name, per_pkg_cfg in pkg_config.items():
-        for item in per_pkg_cfg.get('externals', []):
-            already_defined_specs.add(spack.spec.Spec(item['spec']))
-    return already_defined_specs
-
-
-def _update_pkg_config(scope, pkg_to_entries, not_buildable):
-    predefined_external_specs = _get_predefined_externals()
-
-    pkg_to_cfg, all_new_specs = {}, []
-    for pkg_name, ext_pkg_entries in pkg_to_entries.items():
-        new_entries = list(
-            e for e in ext_pkg_entries
-            if (e.spec not in predefined_external_specs))
-
-        pkg_config = _generate_pkg_config(new_entries)
-        all_new_specs.extend([
-            spack.spec.Spec(x['spec']) for x in pkg_config.get('externals', [])
-        ])
-        if not_buildable:
-            pkg_config['buildable'] = False
-        pkg_to_cfg[pkg_name] = pkg_config
-
-    pkgs_cfg = spack.config.get('packages', scope=scope)
-
-    pkgs_cfg = spack.config.merge_yaml(pkgs_cfg, pkg_to_cfg)
-    spack.config.set('packages', pkgs_cfg, scope=scope)
-
-    return all_new_specs
-
-
-def _get_external_packages(packages_to_check, system_path_to_exe=None):
-    if not system_path_to_exe:
-        system_path_to_exe = _get_system_executables()
-
-    exe_pattern_to_pkgs = defaultdict(list)
-    for pkg in packages_to_check:
-        if hasattr(pkg, 'executables'):
-            for exe in pkg.executables:
-                exe_pattern_to_pkgs[exe].append(pkg)
-
-    pkg_to_found_exes = defaultdict(set)
-    for exe_pattern, pkgs in exe_pattern_to_pkgs.items():
-        compiled_re = re.compile(exe_pattern)
-        for path, exe in system_path_to_exe.items():
-            if compiled_re.search(exe):
-                for pkg in pkgs:
-                    pkg_to_found_exes[pkg].add(path)
-
-    pkg_to_entries = defaultdict(list)
-    resolved_specs = {}  # spec -> exe found for the spec
-
-    for pkg, exes in pkg_to_found_exes.items():
-        if not hasattr(pkg, 'determine_spec_details'):
-            tty.warn("{0} must define 'determine_spec_details' in order"
-                     " for Spack to detect externally-provided instances"
-                     " of the package.".format(pkg.name))
-            continue
-
-        # TODO: iterate through this in a predetermined order (e.g. by package
-        # name) to get repeatable results when there are conflicts. Note that
-        # if we take the prefixes returned by _group_by_prefix, then consider
-        # them in the order that they appear in PATH, this should be sufficient
-        # to get repeatable results.
-        for prefix, exes_in_prefix in _group_by_prefix(exes):
-            # TODO: multiple instances of a package can live in the same
-            # prefix, and a package implementation can return multiple specs
-            # for one prefix, but without additional details (e.g. about the
-            # naming scheme which differentiates them), the spec won't be
-            # usable.
-            specs = _convert_to_iterable(
-                pkg.determine_spec_details(prefix, exes_in_prefix))
-
-            if not specs:
-                tty.debug(
-                    'The following executables in {0} were decidedly not '
-                    'part of the package {1}: {2}'
-                    .format(prefix, pkg.name, ', '.join(
-                        _convert_to_iterable(exes_in_prefix)))
-                )
-
-            for spec in specs:
-                pkg_prefix = _determine_base_dir(prefix)
-
-                if not pkg_prefix:
-                    tty.debug("{0} does not end with a 'bin/' directory: it"
-                              " cannot be added as a Spack package"
-                              .format(prefix))
-                    continue
-
-                if spec in resolved_specs:
-                    prior_prefix = ', '.join(
-                        _convert_to_iterable(resolved_specs[spec]))
-
-                    tty.debug(
-                        "Executables in {0} and {1} are both associated"
-                        " with the same spec {2}"
-                        .format(prefix, prior_prefix, str(spec)))
-                    continue
-                else:
-                    resolved_specs[spec] = prefix
-
-                try:
-                    spec.validate_detection()
-                except Exception as e:
-                    msg = ('"{0}" has been detected on the system but will '
-                           'not be added to packages.yaml [reason={1}]')
-                    tty.warn(msg.format(spec, str(e)))
-                    continue
-
-                if spec.external_path:
-                    pkg_prefix = spec.external_path
-
-                pkg_to_entries[pkg.name].append(
-                    ExternalPackageEntry(spec=spec, base_dir=pkg_prefix))
-
-    return pkg_to_entries
-
-
 def external_list(args):
     # Trigger a read of all packages, might take a long time.
     list(spack.repo.path.all_packages())

lib/spack/spack/detection/__init__.py

@@ -0,0 +1,14 @@
+# Copyright 2013-2021 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)
+from .common import DetectedPackage, executable_prefix, update_configuration
+from .path import by_executable, executables_in_path
+
+__all__ = [
+    'DetectedPackage',
+    'by_executable',
+    'executables_in_path',
+    'executable_prefix',
+    'update_configuration'
+]

lib/spack/spack/detection/common.py

@@ -0,0 +1,177 @@
+# Copyright 2013-2021 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)
+"""Define a common data structure to represent external packages and a
+function to update packages.yaml given a list of detected packages.
+
+Ideally, each detection method should be placed in a specific subpackage
+and implement at least a function that returns a list of DetectedPackage
+objects. The update in packages.yaml can then be done using the function
+provided here.
+
+The module also contains other functions that might be useful across different
+detection mechanisms.
+"""
+import collections
+import os
+import os.path
+
+import six
+
+import llnl.util.tty
+
+import spack.config
+import spack.spec
+import spack.util.spack_yaml
+
+#: Information on a package that has been detected
+DetectedPackage = collections.namedtuple(
+    'DetectedPackage', ['spec', 'prefix']
+)
+
+
+def _externals_in_packages_yaml():
+    """Return all the specs mentioned as externals in packages.yaml"""
+    packages_yaml = spack.config.get('packages')
+    already_defined_specs = set()
+    for pkg_name, package_configuration in packages_yaml.items():
+        for item in package_configuration.get('externals', []):
+            already_defined_specs.add(spack.spec.Spec(item['spec']))
+    return already_defined_specs
+
+
+def _pkg_config_dict(external_pkg_entries):
+    """Generate a package specific config dict according to the packages.yaml schema.
+
+    This does not generate the entire packages.yaml. For example, given some
+    external entries for the CMake package, this could return::
+
+        {
+            'externals': [{
+                'spec': 'cmake@3.17.1',
+                'prefix': '/opt/cmake-3.17.1/'
+            }, {
+                'spec': 'cmake@3.16.5',
+                'prefix': '/opt/cmake-3.16.5/'
+            }]
+       }
+    """
+    pkg_dict = spack.util.spack_yaml.syaml_dict()
+    pkg_dict['externals'] = []
+    for e in external_pkg_entries:
+        if not _spec_is_valid(e.spec):
+            continue
+
+        external_items = [('spec', str(e.spec)), ('prefix', e.prefix)]
+        if e.spec.external_modules:
+            external_items.append(('modules', e.spec.external_modules))
+
+        if e.spec.extra_attributes:
+            external_items.append(
+                ('extra_attributes',
+                 spack.util.spack_yaml.syaml_dict(e.spec.extra_attributes.items()))
+            )
+
+        # external_items.extend(e.spec.extra_attributes.items())
+        pkg_dict['externals'].append(
+            spack.util.spack_yaml.syaml_dict(external_items)
+        )
+
+    return pkg_dict
+
+
+def _spec_is_valid(spec):
+    try:
+        str(spec)
+    except spack.error.SpackError:
+        # It is assumed here that we can at least extract the package name from
+        # the spec so we can look up the implementation of
+        # determine_spec_details
+        msg = 'Constructed spec for {0} does not have a string representation'
+        llnl.util.tty.warn(msg.format(spec.name))
+        return False
+
+    try:
+        spack.spec.Spec(str(spec))
+    except spack.error.SpackError:
+        llnl.util.tty.warn(
+            'Constructed spec has a string representation but the string'
+            ' representation does not evaluate to a valid spec: {0}'
+            .format(str(spec))
+        )
+        return False
+
+    return True
+
+
+def is_executable(file_path):
+    """Return True if the path passed as argument is that of an executable"""
+    return os.path.isfile(file_path) and os.access(file_path, os.X_OK)
+
+
+def _convert_to_iterable(single_val_or_multiple):
+    x = single_val_or_multiple
+    if x is None:
+        return []
+    elif isinstance(x, six.string_types):
+        return [x]
+    elif isinstance(x, spack.spec.Spec):
+        # Specs are iterable, but a single spec should be converted to a list
+        return [x]
+
+    try:
+        iter(x)
+        return x
+    except TypeError:
+        return [x]
+
+
+def executable_prefix(executable_dir):
+    """Given a directory where an executable is found, guess the prefix
+    (i.e. the "root" directory of that installation) and return it.
+
+    Args:
+        executable_dir: directory where an executable is found
+    """
+    # Given a prefix where an executable is found, assuming that prefix
+    # contains /bin/, strip off the 'bin' directory to get a Spack-compatible
+    # prefix
+    assert os.path.isdir(executable_dir)
+
+    components = executable_dir.split(os.sep)
+    if 'bin' not in components:
+        return None
+    idx = components.index('bin')
+    return os.sep.join(components[:idx])
+
+
+def update_configuration(detected_packages, scope=None, buildable=True):
+    """Add the packages passed as arguments to packages.yaml
+
+    Args:
+        detected_packages (list): list of DetectedPackage objects to be added
+        scope (str): configuration scope where to add the detected packages
+        buildable (bool): whether the detected packages are buildable or not
+    """
+    predefined_external_specs = _externals_in_packages_yaml()
+    pkg_to_cfg, all_new_specs = {}, []
+    for package_name, entries in detected_packages.items():
+        new_entries = [
+            e for e in entries if (e.spec not in predefined_external_specs)
+        ]
+
+        pkg_config = _pkg_config_dict(new_entries)
+        all_new_specs.extend([
+            spack.spec.Spec(x['spec']) for x in pkg_config.get('externals', [])
+        ])
+        if buildable is False:
+            pkg_config['buildable'] = False
+        pkg_to_cfg[package_name] = pkg_config
+
+    pkgs_cfg = spack.config.get('packages', scope=scope)
+
+    pkgs_cfg = spack.config.merge_yaml(pkgs_cfg, pkg_to_cfg)
+    spack.config.set('packages', pkgs_cfg, scope=scope)
+
+    return all_new_specs

lib/spack/spack/detection/path.py

@@ -0,0 +1,149 @@
+# Copyright 2013-2021 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)
+"""Detection of software installed in the system based on paths inspections
+and running executables.
+"""
+import collections
+import os
+import os.path
+import re
+
+import llnl.util.filesystem
+import llnl.util.tty
+
+import spack.util.environment
+
+from .common import (
+    DetectedPackage,
+    _convert_to_iterable,
+    executable_prefix,
+    is_executable,
+)
+
+
+def executables_in_path(path_hints=None):
+    """Get the paths of all executables available from the current PATH.
+
+    For convenience, this is constructed as a dictionary where the keys are
+    the executable paths and the values are the names of the executables
+    (i.e. the basename of the executable path).
+
+    There may be multiple paths with the same basename. In this case it is
+    assumed there are two different instances of the executable.
+
+    Args:
+        path_hints (list): list of paths to be searched. If None the list will be
+            constructed based on the PATH environment variable.
+    """
+    path_hints = path_hints or spack.util.environment.get_path('PATH')
+    search_paths = llnl.util.filesystem.search_paths_for_executables(*path_hints)
+
+    path_to_exe = {}
+    # Reverse order of search directories so that an exe in the first PATH
+    # entry overrides later entries
+    for search_path in reversed(search_paths):
+        for exe in os.listdir(search_path):
+            exe_path = os.path.join(search_path, exe)
+            if is_executable(exe_path):
+                path_to_exe[exe_path] = exe
+    return path_to_exe
+
+
+def _group_by_prefix(paths):
+    groups = collections.defaultdict(set)
+    for p in paths:
+        groups[os.path.dirname(p)].add(p)
+    return groups.items()
+
+
+def by_executable(packages_to_check, path_hints=None):
+    """Return the list of packages that have been detected on the system,
+    searching by path.
+
+    Args:
+        packages_to_check (list): list of packages to be detected
+        path_hints (list): list of paths to be searched. If None the list will be
+            constructed based on the PATH environment variable.
+    """
+    path_to_exe_name = executables_in_path(path_hints=path_hints)
+    exe_pattern_to_pkgs = collections.defaultdict(list)
+    for pkg in packages_to_check:
+        if hasattr(pkg, 'executables'):
+            for exe in pkg.executables:
+                exe_pattern_to_pkgs[exe].append(pkg)
+
+    pkg_to_found_exes = collections.defaultdict(set)
+    for exe_pattern, pkgs in exe_pattern_to_pkgs.items():
+        compiled_re = re.compile(exe_pattern)
+        for path, exe in path_to_exe_name.items():
+            if compiled_re.search(exe):
+                for pkg in pkgs:
+                    pkg_to_found_exes[pkg].add(path)
+
+    pkg_to_entries = collections.defaultdict(list)
+    resolved_specs = {}  # spec -> exe found for the spec
+
+    for pkg, exes in pkg_to_found_exes.items():
+        if not hasattr(pkg, 'determine_spec_details'):
+            llnl.util.tty.warn(
+                "{0} must define 'determine_spec_details' in order"
+                " for Spack to detect externally-provided instances"
+                " of the package.".format(pkg.name))
+            continue
+
+        for prefix, exes_in_prefix in sorted(_group_by_prefix(exes)):
+            # TODO: multiple instances of a package can live in the same
+            # prefix, and a package implementation can return multiple specs
+            # for one prefix, but without additional details (e.g. about the
+            # naming scheme which differentiates them), the spec won't be
+            # usable.
+            specs = _convert_to_iterable(
+                pkg.determine_spec_details(prefix, exes_in_prefix)
+            )
+
+            if not specs:
+                llnl.util.tty.debug(
+                    'The following executables in {0} were decidedly not '
+                    'part of the package {1}: {2}'
+                    .format(prefix, pkg.name, ', '.join(
+                        _convert_to_iterable(exes_in_prefix)))
+                )
+
+            for spec in specs:
+                pkg_prefix = executable_prefix(prefix)
+
+                if not pkg_prefix:
+                    msg = "no bin/ dir found in {0}. Cannot add it as a Spack package"
+                    llnl.util.tty.debug(msg.format(prefix))
+                    continue
+
+                if spec in resolved_specs:
+                    prior_prefix = ', '.join(
+                        _convert_to_iterable(resolved_specs[spec]))
+
+                    llnl.util.tty.debug(
+                        "Executables in {0} and {1} are both associated"
+                        " with the same spec {2}"
+                        .format(prefix, prior_prefix, str(spec)))
+                    continue
+                else:
+                    resolved_specs[spec] = prefix
+
+                try:
+                    spec.validate_detection()
+                except Exception as e:
+                    msg = ('"{0}" has been detected on the system but will '
+                           'not be added to packages.yaml [reason={1}]')
+                    llnl.util.tty.warn(msg.format(spec, str(e)))
+                    continue
+
+                if spec.external_path:
+                    pkg_prefix = spec.external_path
+
+                pkg_to_entries[pkg.name].append(
+                    DetectedPackage(spec=spec, prefix=pkg_prefix)
+                )
+
+    return pkg_to_entries

lib/spack/spack/test/cmd/external.py

@@ -8,19 +8,29 @@
 import pytest
 
 import spack
-from spack.cmd.external import ExternalPackageEntry
+import spack.detection
+import spack.detection.path
 from spack.main import SpackCommand
 from spack.spec import Spec
 
 
-def test_find_external_single_package(mock_executable):
+@pytest.fixture
+def executables_found(monkeypatch):
+    def _factory(result):
+        def _mock_search(path_hints=None):
+            return result
+
+        monkeypatch.setattr(spack.detection.path, 'executables_in_path', _mock_search)
+    return _factory
+
+
+def test_find_external_single_package(mock_executable, executables_found):
     pkgs_to_check = [spack.repo.get('cmake')]
+    executables_found({
+        mock_executable("cmake", output='echo "cmake version 1.foo"'): 'cmake'
+    })
 
-    cmake_path = mock_executable("cmake", output='echo "cmake version 1.foo"')
+    pkg_to_entries = spack.detection.by_executable(pkgs_to_check)
-    system_path_to_exe = {cmake_path: 'cmake'}
-
-    pkg_to_entries = spack.cmd.external._get_external_packages(
-        pkgs_to_check, system_path_to_exe)
 
     pkg, entries = next(iter(pkg_to_entries.items()))
     single_entry = next(iter(entries))
@@ -28,7 +38,7 @@ def test_find_external_single_package(mock_executable):
     assert single_entry.spec == Spec('cmake@1.foo')
 
 
-def test_find_external_two_instances_same_package(mock_executable):
+def test_find_external_two_instances_same_package(mock_executable, executables_found):
     pkgs_to_check = [spack.repo.get('cmake')]
 
     # Each of these cmake instances is created in a different prefix
@@ -38,30 +48,30 @@ def test_find_external_two_instances_same_package(mock_executable):
     cmake_path2 = mock_executable(
         "cmake", output='echo "cmake version 3.17.2"', subdir=('base2', 'bin')
     )
-    system_path_to_exe = {
+    executables_found({
         cmake_path1: 'cmake',
-        cmake_path2: 'cmake'}
+        cmake_path2: 'cmake'
+    })
 
-    pkg_to_entries = spack.cmd.external._get_external_packages(
+    pkg_to_entries = spack.detection.by_executable(pkgs_to_check)
-        pkgs_to_check, system_path_to_exe)
 
     pkg, entries = next(iter(pkg_to_entries.items()))
-    spec_to_path = dict((e.spec, e.base_dir) for e in entries)
+    spec_to_path = dict((e.spec, e.prefix) for e in entries)
     assert spec_to_path[Spec('cmake@1.foo')] == (
-        spack.cmd.external._determine_base_dir(os.path.dirname(cmake_path1)))
+        spack.detection.executable_prefix(os.path.dirname(cmake_path1)))
     assert spec_to_path[Spec('cmake@3.17.2')] == (
-        spack.cmd.external._determine_base_dir(os.path.dirname(cmake_path2)))
+        spack.detection.executable_prefix(os.path.dirname(cmake_path2)))
 
 
 def test_find_external_update_config(mutable_config):
     entries = [
-        ExternalPackageEntry(Spec.from_detection('cmake@1.foo'), '/x/y1/'),
+        spack.detection.DetectedPackage(Spec.from_detection('cmake@1.foo'), '/x/y1/'),
-        ExternalPackageEntry(Spec.from_detection('cmake@3.17.2'), '/x/y2/'),
+        spack.detection.DetectedPackage(Spec.from_detection('cmake@3.17.2'), '/x/y2/'),
     ]
     pkg_to_entries = {'cmake': entries}
 
     scope = spack.config.default_modify_scope('packages')
-    spack.cmd.external._update_pkg_config(scope, pkg_to_entries, False)
+    spack.detection.update_configuration(pkg_to_entries, scope=scope, buildable=True)
 
     pkgs_cfg = spack.config.get('packages')
     cmake_cfg = pkgs_cfg['cmake']
@@ -75,7 +85,7 @@ def test_get_executables(working_env, mock_executable):
     cmake_path1 = mock_executable("cmake", output="echo cmake version 1.foo")
 
     os.environ['PATH'] = ':'.join([os.path.dirname(cmake_path1)])
-    path_to_exe = spack.cmd.external._get_system_executables()
+    path_to_exe = spack.detection.executables_in_path()
     assert path_to_exe[cmake_path1] == 'cmake'
 
 
@@ -149,16 +159,16 @@ def test_find_external_merge(mutable_config, mutable_mock_repo):
 
     mutable_config.update_config('packages', pkgs_cfg_init)
     entries = [
-        ExternalPackageEntry(
+        spack.detection.DetectedPackage(
             Spec.from_detection('find-externals1@1.1'), '/x/y1/'
         ),
-        ExternalPackageEntry(
+        spack.detection.DetectedPackage(
             Spec.from_detection('find-externals1@1.2'), '/x/y2/'
         )
     ]
     pkg_to_entries = {'find-externals1': entries}
     scope = spack.config.default_modify_scope('packages')
-    spack.cmd.external._update_pkg_config(scope, pkg_to_entries, False)
+    spack.detection.update_configuration(pkg_to_entries, scope=scope, buildable=True)
 
     pkgs_cfg = spack.config.get('packages')
     pkg_cfg = pkgs_cfg['find-externals1']