Simplify the detection protocol for packages

Packages can implement “detect_version” to support detection
of external instances of a package. This is generally easier
than implementing “determine_spec_details”. The API for
determine_version is similar: for example you can return
“None” to indicate that an executable is not an instance
of a package.

Users may implement a “determine_variants” method for a package.
When doing external detection, executables are grouped by version
and each group results in a single invocation of “determine_variants”
for the associated spec. The method returns a string specifying
the variants for the package. The method may additionally return
a dictionary representing extra attributes for the package.

These will be stored in the spec yaml and can be retrieved
from self.spec.extra_attributes

The Spack GCC package has been updated with an implementation
of “determine_variants” which adds the following extra
attributes to the package: c, cxx, fortran

7 files changed, 453 insertions, 80 deletions

diff --git a/lib/spack/docs/packaging_guide.rst b/lib/spack/docs/packaging_guide.rst
index d3a888b1fc..52f690c4d6 100644
--- a/lib/spack/docs/packaging_guide.rst
+++ b/lib/spack/docs/packaging_guide.rst
@@ -4054,21 +4054,223 @@ File functions
 Making a package discoverable with ``spack external find``
 ----------------------------------------------------------
 
-To make a package discoverable with
-:ref:`spack external find <cmd-spack-external-find>` you must
-define one or more executables associated with the package and must
-implement a method to generate a Spec when given an executable.
+The simplest way to make a package discoverable with
+:ref:`spack external find <cmd-spack-external-find>` is to:
 
-The executables are specified as a package level ``executables``
-attribute which is a list of strings (see example below); each string
-is treated as a regular expression (e.g. 'gcc' would match 'gcc', 'gcc-8.3',
-'my-weird-gcc', etc.).
+1. Define the executables associated with the package
+2. Implement a method to determine the versions of these executables
 
-The method ``determine_spec_details`` has the following signature:
+^^^^^^^^^^^^^^^^^
+Minimal detection
+^^^^^^^^^^^^^^^^^
+
+The first step is fairly simple, as it requires only to
+specify a package level ``executables`` attribute:
+
+.. code-block:: python
+
+   class Foo(Package):
+       # Each string provided here is treated as a regular expression, and
+       # would match for example 'foo', 'foobar', and 'bazfoo'.
+       executables = ['foo']
+
+This attribute must be a list of strings. Each string is a regular
+expression (e.g. 'gcc' would match 'gcc', 'gcc-8.3', 'my-weird-gcc', etc.) to
+determine a set of system executables that might be part or this package. Note
+that to match only executables named 'gcc' the regular expression ``'^gcc$'``
+must be used.
+
+Finally to determine the version of each executable the ``determine_version``
+method must be implemented:
+
+.. code-block:: python
+
+   @classmethod
+   def determine_version(cls, exe):
+       """Return either the version of the executable passed as argument
+       or ``None`` if the version cannot be determined.
+
+       Args:
+           exe (str): absolute path to the executable being examined
+       """
+
+This method receives as input the path to a single executable and must return
+as output its version as a string; if the user cannot determine the version
+or determines that the executable is not an instance of the package, they can
+return None and the exe will be discarded as a candidate.
+Implementing the two steps above is mandatory, and gives the package the
+basic ability to detect if a spec is present on the system at a given version.
+
+.. note::
+   Any executable for which the ``determine_version`` method returns ``None``
+   will be discarded and won't appear in later stages of the workflow described below.
+
+^^^^^^^^^^^^^^^^^^^^^^^^
+Additional functionality
+^^^^^^^^^^^^^^^^^^^^^^^^
+
+Besides the two mandatory steps described above, there are also optional
+methods that can be implemented to either increase the amount of details
+being detected or improve the robustness of the detection logic in a package.
+
+""""""""""""""""""""""""""""""
+Variants and custom attributes
+""""""""""""""""""""""""""""""
+
+The ``determine_variants`` method can be optionally implemented in a package
+to detect additional details of the spec:
+
+.. code-block:: python
+
+   @classmethod
+   def determine_variants(cls, exes, version_str):
+       """Return either a variant string, a tuple of a variant string
+       and a dictionary of extra attributes that will be recorded in
+       packages.yaml or a list of those items.
+
+       Args:
+           exes (list of str): list of executables (absolute paths) that
+               live in the same prefix and share the same version
+           version_str (str): version associated with the list of
+               executables, as detected by ``determine_version``
+       """
+
+This method takes as input a list of executables that live in the same prefix and
+share the same version string, and returns either:
+
+1. A variant string
+2. A tuple of a variant string and a dictionary of extra attributes
+3. A list of items matching either 1 or 2 (if multiple specs are detected
+   from the set of executables)
+
+If extra attributes are returned, they will be recorded in ``packages.yaml``
+and be available for later reuse. As an example, the ``gcc`` package will record
+by default the different compilers found and an entry in ``packages.yaml``
+would look like:
+
+.. code-block:: yaml
+
+   packages:
+     gcc:
+       externals:
+       - spec: 'gcc@9.0.1 languages=c,c++,fortran'
+         prefix: /usr
+         extra_attributes:
+           compilers:
+             c: /usr/bin/x86_64-linux-gnu-gcc-9
+             c++: /usr/bin/x86_64-linux-gnu-g++-9
+             fortran: /usr/bin/x86_64-linux-gnu-gfortran-9
+
+This allows us, for instance, to keep track of executables that would be named
+differently if built by Spack (e.g. ``x86_64-linux-gnu-gcc-9``
+instead of just ``gcc``).
+
+.. TODO: we need to gather some more experience on overriding 'prefix'
+   and other special keywords in extra attributes, but as soon as we are
+   confident that this is the way to go we should document the process.
+   See https://github.com/spack/spack/pull/16526#issuecomment-653783204
+
+"""""""""""""""""""""""""""
+Filter matching executables
+"""""""""""""""""""""""""""
+
+Sometimes defining the appropriate regex for the ``executables``
+attribute might prove to be difficult, especially if one has to
+deal with corner cases or exclude "red herrings". To help keeping
+the regular expressions as simple as possible, each package can
+optionally implement a ``filter_executables`` method:
+
+.. code-block:: python
+
+    @classmethod
+    def filter_detected_exes(cls, prefix, exes_in_prefix):
+        """Return a filtered list of the executables in prefix"""
+
+which takes as input a prefix and a list of matching executables and
+returns a filtered list of said executables.
+
+Using this method has the advantage of allowing custom logic for
+filtering, and does not restrict the user to regular expressions
+only.  Consider the case of detecting the GNU C++ compiler. If we
+try to search for executables that match ``g++``, that would have
+the unwanted side effect of selecting also ``clang++`` - which is
+a C++ compiler provided by another package - if present on the system.
+Trying to select executables that contain ``g++`` but not ``clang``
+would be quite complicated to do using regex only. Employing the
+``filter_detected_exes`` method it becomes:
+
+.. code-block:: python
+
+   class Gcc(Package):
+      executables = ['g++']
+
+      def filter_detected_exes(cls, prefix, exes_in_prefix):
+         return [x for x in exes_in_prefix if 'clang' not in x]
+
+Another possibility that this method opens is to apply certain
+filtering logic when specific conditions are met (e.g. take some
+decisions on an OS and not on another).
+
+^^^^^^^^^^^^^^^^^^
+Validate detection
+^^^^^^^^^^^^^^^^^^
+
+To increase detection robustness, packagers may also implement a method
+to validate the detected Spec objects:
+
+.. code-block:: python
+
+   @classmethod
+   def validate_detected_spec(cls, spec, extra_attributes):
+       """Validate a detected spec. Raise an exception if validation fails."""
+
+This method receives a detected spec along with its extra attributes and can be
+used to check that certain conditions are met by the spec. Packagers can either
+use assertions or raise an ``InvalidSpecDetected`` exception when the check fails.
+In case the conditions are not honored the spec will be discarded and any message
+associated with the assertion or the exception will be logged as the reason for
+discarding it.
+
+As an example, a package that wants to check that the ``compilers`` attribute is
+in the extra attributes can implement this method like this:
+
+.. code-block:: python
+
+   @classmethod
+   def validate_detected_spec(cls, spec, extra_attributes):
+       """Check that 'compilers' is in the extra attributes."""
+       msg = ('the extra attribute "compilers" must be set for '
+              'the detected spec "{0}"'.format(spec))
+       assert 'compilers' in extra_attributes, msg
+
+or like this:
+
+.. code-block:: python
+
+   @classmethod
+   def validate_detected_spec(cls, spec, extra_attributes):
+       """Check that 'compilers' is in the extra attributes."""
+       if 'compilers' not in extra_attributes:
+           msg = ('the extra attribute "compilers" must be set for '
+                  'the detected spec "{0}"'.format(spec))
+           raise InvalidSpecDetected(msg)
+
+.. _determine_spec_details:
+
+^^^^^^^^^^^^^^^^^^^^^^^^^
+Custom detection workflow
+^^^^^^^^^^^^^^^^^^^^^^^^^
+
+In the rare case when the mechanisms described so far don't fit the
+detection of a package, the implementation of all the methods above
+can be disregarded and instead a custom ``determine_spec_details``
+method can be implemented directly in the package class (note that
+the definition of the ``executables`` attribute is still required):
 
 .. code-block:: python
 
-   def determine_spec_details(prefix, exes_in_prefix):
+   @classmethod
+   def determine_spec_details(cls, prefix, exes_in_prefix):
        # exes_in_prefix = a set of paths, each path is an executable
        # prefix = a prefix that is common to each path in exes_in_prefix
 
@@ -4076,14 +4278,13 @@ The method ``determine_spec_details`` has the following signature:
        # the package. Return one or more Specs for each instance of the
        # package which is thought to be installed in the provided prefix
 
-``determine_spec_details`` takes as parameters a set of discovered
-executables (which match those specified by the user) as well as a
-common prefix shared by all of those executables. The function must
-return one or more Specs associated with the executables (it can also
-return ``None`` to indicate that no provided executables are associated
-with the package).
+This method takes as input a set of discovered executables (which match
+those specified by the user) as well as a common prefix shared by all
+of those executables. The function must return one or more :py:class:`spack.spec.Spec` associated
+with the executables (it can also return ``None`` to indicate that no
+provided executables are associated with the package).
 
-Say for example we have a package called ``foo-package`` which
+As an example, consider a made-up package called ``foo-package`` which
 builds an executable called ``foo``. ``FooPackage`` would appear as
 follows:
 
@@ -4110,7 +4311,9 @@ follows:
            exe = spack.util.executable.Executable(exe_path)
            output = exe('--version')
            version_str = ...  # parse output for version string
-           return Spec('foo-package@{0}'.format(version_str))
+           return Spec.from_detection(
+               'foo-package@{0}'.format(version_str)
+           )
 
 .. _package-lifecycle:
 
diff --git a/lib/spack/spack/cmd/external.py b/lib/spack/spack/cmd/external.py
index 6fe7825070..170b5b0395 100644
--- a/lib/spack/spack/cmd/external.py
+++ b/lib/spack/spack/cmd/external.py
@@ -2,22 +2,24 @@
 # Spack Project Developers. See the top-level COPYRIGHT file for details.
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
-
 from __future__ import print_function
-from collections import defaultdict, namedtuple
+
 import argparse
 import os
 import re
-import six
+import sys
+from collections import defaultdict, namedtuple
 
+import llnl.util.filesystem
+import llnl.util.tty as tty
+import llnl.util.tty.colify as colify
+import six
 import spack
 import spack.error
-import llnl.util.tty as tty
-import spack.util.spack_yaml as syaml
 import spack.util.environment
-import llnl.util.filesystem
+import spack.util.spack_yaml as syaml
 
-description = "add external packages to Spack configuration"
+description = "manage external packages in Spack configuration"
 section = "config"
 level = "short"
 
@@ -26,12 +28,18 @@ def setup_parser(subparser):
     sp = subparser.add_subparsers(
         metavar='SUBCOMMAND', dest='external_command')
 
-    find_parser = sp.add_parser('find', help=external_find.__doc__)
+    find_parser = sp.add_parser(
+        'find', help='add external packages to packages.yaml'
+    )
     find_parser.add_argument(
         '--not-buildable', action='store_true', default=False,
         help="packages with detected externals won't be built with Spack")
     find_parser.add_argument('packages', nargs=argparse.REMAINDER)
 
+    sp.add_parser(
+        'list', help='list detectable packages, by repository and name'
+    )
+
 
 def is_executable(path):
     return os.path.isfile(path) and os.access(path, os.X_OK)
@@ -92,7 +100,16 @@ def _generate_pkg_config(external_pkg_entries):
             continue
 
         external_items = [('spec', str(e.spec)), ('prefix', e.base_dir)]
-        external_items.extend(e.spec.extra_attributes.items())
+        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)
         )
@@ -272,17 +289,29 @@ def _get_external_packages(packages_to_check, system_path_to_exe=None):
                     spec.validate_detection()
                 except Exception as e:
                     msg = ('"{0}" has been detected on the system but will '
-                           'not be added to packages.yaml [{1}]')
+                           '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(parser, args):
-    action = {'find': external_find}
+def external_list(args):
+    # Trigger a read of all packages, might take a long time.
+    list(spack.repo.path.all_packages())
+    # Print all the detectable packages
+    tty.msg("Detectable packages per repository")
+    for namespace, pkgs in sorted(spack.package.detectable_packages.items()):
+        print("Repository:", namespace)
+        colify.colify(pkgs, indent=4, output=sys.stdout)
 
+
+def external(parser, args):
+    action = {'find': external_find, 'list': external_list}
     action[args.external_command](args)
diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py
index c5cae4f9b0..d5cb3065a8 100644
--- a/lib/spack/spack/package.py
+++ b/lib/spack/spack/package.py
@@ -11,6 +11,7 @@ packages.
 
 """
 import base64
+import collections
 import contextlib
 import copy
 import functools
@@ -23,19 +24,14 @@ import sys
 import textwrap
 import time
 import traceback
-from six import StringIO
-from six import string_types
-from six import with_metaclass
-from ordereddict_backport import OrderedDict
 
-import llnl.util.tty as tty
+import six
 
-import spack.config
-import spack.paths
-import spack.store
+import llnl.util.tty as tty
 import spack.compilers
-import spack.directives
+import spack.config
 import spack.dependency
+import spack.directives
 import spack.directory_layout
 import spack.error
 import spack.fetch_strategy as fs
@@ -43,15 +39,19 @@ import spack.hooks
 import spack.mirror
 import spack.mixins
 import spack.multimethod
+import spack.paths
 import spack.repo
+import spack.store
 import spack.url
 import spack.util.environment
 import spack.util.web
-import spack.multimethod
-
 from llnl.util.filesystem import mkdirp, touch, working_dir
 from llnl.util.lang import memoized
 from llnl.util.link_tree import LinkTree
+from ordereddict_backport import OrderedDict
+from six import StringIO
+from six import string_types
+from six import with_metaclass
 from spack.filesystem_view import YamlFilesystemView
 from spack.installer import \
     install_args_docstring, PackageInstaller, InstallError
@@ -141,7 +141,104 @@ class InstallPhase(object):
             return other
 
 
+#: Registers which are the detectable packages, by repo and package name
+#: Need a pass of package repositories to be filled.
+detectable_packages = collections.defaultdict(list)
+
+
+class DetectablePackageMeta(object):
+    """Check if a package is detectable and add default implementations
+    for the detection function.
+    """
+    def __init__(cls, name, bases, attr_dict):
+        # If a package has the executables attribute then it's
+        # assumed to be detectable
+        if hasattr(cls, 'executables'):
+            @classmethod
+            def determine_spec_details(cls, prefix, exes_in_prefix):
+                """Allow ``spack external find ...`` to locate installations.
+
+                Args:
+                    prefix (str): the directory containing the executables
+                    exes_in_prefix (set): the executables that match the regex
+
+                Returns:
+                    The list of detected specs for this package
+                """
+                exes_by_version = collections.defaultdict(list)
+                # The default filter function is the identity function for the
+                # list of executables
+                filter_fn = getattr(cls, 'filter_detected_exes',
+                                    lambda x, exes: exes)
+                exes_in_prefix = filter_fn(prefix, exes_in_prefix)
+                for exe in exes_in_prefix:
+                    try:
+                        version_str = cls.determine_version(exe)
+                        if version_str:
+                            exes_by_version[version_str].append(exe)
+                    except Exception as e:
+                        msg = ('An error occurred when trying to detect '
+                               'the version of "{0}" [{1}]')
+                        tty.debug(msg.format(exe, str(e)))
+
+                specs = []
+                for version_str, exes in exes_by_version.items():
+                    variants = cls.determine_variants(exes, version_str)
+                    # Normalize output to list
+                    if not isinstance(variants, list):
+                        variants = [variants]
+
+                    for variant in variants:
+                        if isinstance(variant, six.string_types):
+                            variant = (variant, {})
+                        variant_str, extra_attributes = variant
+                        spec_str = '{0}@{1} {2}'.format(
+                            cls.name, version_str, variant_str
+                        )
+
+                        # Pop a few reserved keys from extra attributes, since
+                        # they have a different semantics
+                        external_path = extra_attributes.pop('prefix', None)
+                        external_modules = extra_attributes.pop(
+                            'modules', None
+                        )
+                        spec = spack.spec.Spec(
+                            spec_str,
+                            external_path=external_path,
+                            external_modules=external_modules
+                        )
+                        specs.append(spack.spec.Spec.from_detection(
+                            spec, extra_attributes=extra_attributes
+                        ))
+
+                return sorted(specs)
+
+            @classmethod
+            def determine_variants(cls, exes, version_str):
+                return ''
+
+            # Register the class as a detectable package
+            detectable_packages[cls.namespace].append(cls.name)
+
+            # Attach function implementations to the detectable class
+            default = False
+            if not hasattr(cls, 'determine_spec_details'):
+                default = True
+                cls.determine_spec_details = determine_spec_details
+
+            if default and not hasattr(cls, 'determine_version'):
+                msg = ('the package "{0}" in the "{1}" repo needs to define'
+                       ' the "determine_version" method to be detectable')
+                NotImplementedError(msg.format(cls.name, cls.namespace))
+
+            if default and not hasattr(cls, 'determine_variants'):
+                cls.determine_variants = determine_variants
+
+        super(DetectablePackageMeta, cls).__init__(name, bases, attr_dict)
+
+
 class PackageMeta(
+    DetectablePackageMeta,
     spack.directives.DirectiveMeta,
     spack.mixins.PackageMixinsMeta,
     spack.multimethod.MultiMethodMeta
diff --git a/lib/spack/spack/pkgkit.py b/lib/spack/spack/pkgkit.py
index e657144bb4..e2a29894f7 100644
--- a/lib/spack/spack/pkgkit.py
+++ b/lib/spack/spack/pkgkit.py
@@ -39,7 +39,7 @@ from spack.mixins import filter_compiler_wrappers
 
 from spack.version import Version, ver
 
-from spack.spec import Spec
+from spack.spec import Spec, InvalidSpecDetected
 
 from spack.dependency import all_deptypes
 
diff --git a/lib/spack/spack/schema/packages.py b/lib/spack/spack/schema/packages.py
index e97e953b83..16a8a223ef 100644
--- a/lib/spack/spack/schema/packages.py
+++ b/lib/spack/spack/schema/packages.py
@@ -117,7 +117,7 @@ schema = {
 
 
 def update(data):
-    """Update in-place the data to remove deprecated properties.
+    """Update the data in place to remove deprecated properties.
 
     Args:
         data (dict): dictionary to be updated
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index 941656a964..843bb6fbb1 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -4571,3 +4571,7 @@ class SpecDependencyNotFoundError(spack.error.SpecError):
 
 class SpecDeprecatedError(spack.error.SpecError):
     """Raised when a spec concretizes to a deprecated spec or dependency."""
+
+
+class InvalidSpecDetected(spack.error.SpecError):
+    """Raised when a detected spec doesn't pass validation checks."""
diff --git a/lib/spack/spack/test/cmd/external.py b/lib/spack/spack/test/cmd/external.py
index 31175843ec..547d20de24 100644
--- a/lib/spack/spack/test/cmd/external.py
+++ b/lib/spack/spack/test/cmd/external.py
@@ -2,10 +2,8 @@
 # Spack Project Developers. See the top-level COPYRIGHT file for details.
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
-
-import pytest
 import os
-import stat
+import os.path
 
 import spack
 from spack.spec import Spec
@@ -13,30 +11,10 @@ from spack.cmd.external import ExternalPackageEntry
 from spack.main import SpackCommand
 
 
-@pytest.fixture()
-def create_exe(tmpdir_factory):
-    def _create_exe(exe_name, content):
-        base_prefix = tmpdir_factory.mktemp('base-prefix')
-        base_prefix.ensure('bin', dir=True)
-        exe_path = str(base_prefix.join('bin', exe_name))
-        with open(exe_path, 'w') as f:
-            f.write("""\
-#!/bin/bash
-
-echo "{0}"
-""".format(content))
-
-        st = os.stat(exe_path)
-        os.chmod(exe_path, st.st_mode | stat.S_IEXEC)
-        return exe_path
-
-    yield _create_exe
-
-
-def test_find_external_single_package(create_exe):
+def test_find_external_single_package(mock_executable):
     pkgs_to_check = [spack.repo.get('cmake')]
 
-    cmake_path = create_exe("cmake", "cmake version 1.foo")
+    cmake_path = mock_executable("cmake", output='echo "cmake version 1.foo"')
     system_path_to_exe = {cmake_path: 'cmake'}
 
     pkg_to_entries = spack.cmd.external._get_external_packages(
@@ -48,12 +26,16 @@ def test_find_external_single_package(create_exe):
     assert single_entry.spec == Spec('cmake@1.foo')
 
 
-def test_find_external_two_instances_same_package(create_exe):
+def test_find_external_two_instances_same_package(mock_executable):
     pkgs_to_check = [spack.repo.get('cmake')]
 
     # Each of these cmake instances is created in a different prefix
-    cmake_path1 = create_exe("cmake", "cmake version 1.foo")
-    cmake_path2 = create_exe("cmake", "cmake version 3.17.2")
+    cmake_path1 = mock_executable(
+        "cmake", output='echo "cmake version 1.foo"', subdir=('base1', 'bin')
+    )
+    cmake_path2 = mock_executable(
+        "cmake", output='echo "cmake version 3.17.2"', subdir=('base2', 'bin')
+    )
     system_path_to_exe = {
         cmake_path1: 'cmake',
         cmake_path2: 'cmake'}
@@ -86,8 +68,8 @@ def test_find_external_update_config(mutable_config):
     assert {'spec': 'cmake@3.17.2', 'prefix': '/x/y2/'} in cmake_externals
 
 
-def test_get_executables(working_env, create_exe):
-    cmake_path1 = create_exe("cmake", "cmake version 1.foo")
+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()
@@ -97,11 +79,11 @@ def test_get_executables(working_env, create_exe):
 external = SpackCommand('external')
 
 
-def test_find_external_cmd(mutable_config, working_env, create_exe):
+def test_find_external_cmd(mutable_config, working_env, mock_executable):
     """Test invoking 'spack external find' with additional package arguments,
     which restricts the set of packages that Spack looks for.
     """
-    cmake_path1 = create_exe("cmake", "cmake version 1.foo")
+    cmake_path1 = mock_executable("cmake", output="echo cmake version 1.foo")
     prefix = os.path.dirname(os.path.dirname(cmake_path1))
 
     os.environ['PATH'] = ':'.join([os.path.dirname(cmake_path1)])
@@ -115,12 +97,12 @@ def test_find_external_cmd(mutable_config, working_env, create_exe):
 
 
 def test_find_external_cmd_not_buildable(
-        mutable_config, working_env, create_exe):
+        mutable_config, working_env, mock_executable):
     """When the user invokes 'spack external find --not-buildable', the config
     for any package where Spack finds an external version should be marked as
     not buildable.
     """
-    cmake_path1 = create_exe("cmake", "cmake version 1.foo")
+    cmake_path1 = mock_executable("cmake", output="echo cmake version 1.foo")
     os.environ['PATH'] = ':'.join([os.path.dirname(cmake_path1)])
     external('find', '--not-buildable', 'cmake')
     pkgs_cfg = spack.config.get('packages')
@@ -128,13 +110,13 @@ def test_find_external_cmd_not_buildable(
 
 
 def test_find_external_cmd_full_repo(
-        mutable_config, working_env, create_exe, mutable_mock_repo):
+        mutable_config, working_env, mock_executable, mutable_mock_repo):
     """Test invoking 'spack external find' with no additional arguments, which
     iterates through each package in the repository.
     """
 
-    exe_path1 = create_exe(
-        "find-externals1-exe", "find-externals1 version 1.foo"
+    exe_path1 = mock_executable(
+        "find-externals1-exe", output="echo find-externals1 version 1.foo"
     )
     prefix = os.path.dirname(os.path.dirname(exe_path1))
 
@@ -182,3 +164,61 @@ def test_find_external_merge(mutable_config, mutable_mock_repo):
             'prefix': '/preexisting-prefix/'} in pkg_externals
     assert {'spec': 'find-externals1@1.2',
             'prefix': '/x/y2/'} in pkg_externals
+
+
+def test_list_detectable_packages(mutable_config, mutable_mock_repo):
+    external("list")
+    assert external.returncode == 0
+
+
+def test_packages_yaml_format(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)
+    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'] == gcc_exe
+
+
+def test_overriding_prefix(mock_executable, mutable_config, monkeypatch):
+    # Prepare an environment to detect a fake gcc that
+    # override its external prefix
+    gcc_exe = mock_executable('gcc', output="echo 4.2.1")
+    prefix = os.path.dirname(gcc_exe)
+    monkeypatch.setenv('PATH', prefix)
+
+    @classmethod
+    def _determine_variants(cls, exes, version_str):
+        return 'languages=c', {
+            'prefix': '/opt/gcc/bin',
+            'compilers': {'c': exes[0]}
+        }
+
+    gcc_cls = spack.repo.path.get_pkg_class('gcc')
+    monkeypatch.setattr(gcc_cls, 'determine_variants', _determine_variants)
+
+    # 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
+    assert externals[0]['prefix'] == '/opt/gcc/bin'