External packages are now registered in the DB (#1167)

* treats correctly a change from `explicit=False` to `explicit=True` in an external package DB entry.
* added unit tests
* fixed issues raised by @tgamblin . In particular the PR is no more hash-changing for packages that are not external.
* added a test to check correctness of a spec/yaml round-trip for things that involve an external
* Don't find external module path at each step of concretization
    * it's not necessary.. The paths are retrieved at the end of concretizaion
* Don't find replacements for external packages.
* Test root of the DAG if external
    * No reason not to test if the root of the DAG is external when external
packages are now first class citizens!
* Create `external` property for Spec (for external_path and external_module)
* Allow users to specify external package paths relative to spack
    * Canonicalize external package paths so that users may specify their
locations relative to spack's directory.
* Update tests to use new external_path and external properly.
* skip license hooks on external

15 files changed, 230 insertions, 57 deletions

diff --git a/lib/spack/spack/build_environment.py b/lib/spack/spack/build_environment.py
index 4a8487daab..06ac65f552 100644
--- a/lib/spack/spack/build_environment.py
+++ b/lib/spack/spack/build_environment.py
@@ -465,7 +465,10 @@ def parent_class_modules(cls):
 
 
 def load_external_modules(pkg):
-    """ traverse the spec list and find any specs that have external modules.
+    """Traverse the spec list associated with a package
+    and find any specs that have external modules.
+
+    :param pkg: package under consideration
     """
     for dep in list(pkg.spec.traverse()):
         if dep.external_module:
diff --git a/lib/spack/spack/concretize.py b/lib/spack/spack/concretize.py
index 2a5ce65fa4..0b29d2874e 100644
--- a/lib/spack/spack/concretize.py
+++ b/lib/spack/spack/concretize.py
@@ -99,7 +99,7 @@ class DefaultConcretizer(object):
 
         # Use a sort key to order the results
         return sorted(usable, key=lambda spec: (
-            not (spec.external or spec.external_module),  # prefer externals
+            not spec.external,                            # prefer externals
             pref_key(spec),                               # respect prefs
             spec.name,                                    # group by name
             reverse_order(spec.versions),                 # latest version
diff --git a/lib/spack/spack/database.py b/lib/spack/spack/database.py
index 3cb8ced342..33eb7bea4c 100644
--- a/lib/spack/spack/database.py
+++ b/lib/spack/spack/database.py
@@ -67,9 +67,9 @@ from spack.version import Version
 _db_dirname = '.spack-db'
 
 # DB version.  This is stuck in the DB file to track changes in format.
-_db_version = Version('0.9.2')
+_db_version = Version('0.9.3')
 
-# Default timeout for spack database locks is 5 min.
+# Timeout for spack database locks in seconds
 _db_lock_timeout = 60
 
 # Types of dependencies tracked by the database
@@ -409,24 +409,40 @@ class Database(object):
                 self._data = {}
 
         transaction = WriteTransaction(
-            self.lock, _read_suppress_error, self._write, _db_lock_timeout)
+            self.lock, _read_suppress_error, self._write, _db_lock_timeout
+        )
 
         with transaction:
             if self._error:
                 tty.warn(
                     "Spack database was corrupt. Will rebuild. Error was:",
-                    str(self._error))
+                    str(self._error)
+                )
                 self._error = None
 
+            # Read first the `spec.yaml` files in the prefixes. They should be
+            # considered authoritative with respect to DB reindexing, as
+            # entries in the DB may be corrupted in a way that still makes
+            # them readable. If we considered DB entries authoritative
+            # instead, we would perpetuate errors over a reindex.
+
             old_data = self._data
             try:
+                # Initialize data in the reconstructed DB
                 self._data = {}
 
-                # Ask the directory layout to traverse the filesystem.
+                # Start inspecting the installed prefixes
+                processed_specs = set()
+
                 for spec in directory_layout.all_specs():
                     # Try to recover explicit value from old DB, but
-                    # default it to False if DB was corrupt.
-                    explicit = False
+                    # default it to True if DB was corrupt. This is
+                    # just to be conservative in case a command like
+                    # "autoremove" is run by the user after a reindex.
+                    tty.debug(
+                        'RECONSTRUCTING FROM SPEC.YAML: {0}'.format(spec)
+                    )
+                    explicit = True
                     if old_data is not None:
                         old_info = old_data.get(spec.dag_hash())
                         if old_info is not None:
@@ -434,6 +450,42 @@ class Database(object):
 
                     self._add(spec, directory_layout, explicit=explicit)
 
+                    processed_specs.add(spec)
+
+                for key, entry in old_data.items():
+                    # We already took care of this spec using
+                    # `spec.yaml` from its prefix.
+                    if entry.spec in processed_specs:
+                        msg = 'SKIPPING RECONSTRUCTION FROM OLD DB: {0}'
+                        msg += ' [already reconstructed from spec.yaml]'
+                        tty.debug(msg.format(entry.spec))
+                        continue
+
+                    # If we arrived here it very likely means that
+                    # we have external specs that are not dependencies
+                    # of other specs. This may be the case for externally
+                    # installed compilers or externally installed
+                    # applications.
+                    tty.debug(
+                        'RECONSTRUCTING FROM OLD DB: {0}'.format(entry.spec)
+                    )
+                    try:
+                        layout = spack.store.layout
+                        if entry.spec.external:
+                            layout = None
+                        kwargs = {
+                            'spec': entry.spec,
+                            'directory_layout': layout,
+                            'explicit': entry.explicit
+                        }
+                        self._add(**kwargs)
+                        processed_specs.add(entry.spec)
+                    except Exception as e:
+                        # Something went wrong, so the spec was not restored
+                        # from old data
+                        tty.debug(e.message)
+                        pass
+
                 self._check_ref_counts()
 
             except:
@@ -542,7 +594,7 @@ class Database(object):
 
         key = spec.dag_hash()
         if key not in self._data:
-            installed = False
+            installed = bool(spec.external)
             path = None
             if not spec.external and directory_layout:
                 path = directory_layout.path_for_spec(spec)
diff --git a/lib/spack/spack/directory_layout.py b/lib/spack/spack/directory_layout.py
index 9d09875484..e7c9d6c590 100644
--- a/lib/spack/spack/directory_layout.py
+++ b/lib/spack/spack/directory_layout.py
@@ -193,7 +193,7 @@ class YamlDirectoryLayout(DirectoryLayout):
         _check_concrete(spec)
 
         if spec.external:
-            return spec.external
+            return spec.external_path
 
         path = spec.format(self.path_scheme)
         return path
diff --git a/lib/spack/spack/hooks/licensing.py b/lib/spack/spack/hooks/licensing.py
index a99099749c..9a244dbdef 100644
--- a/lib/spack/spack/hooks/licensing.py
+++ b/lib/spack/spack/hooks/licensing.py
@@ -31,7 +31,7 @@ from llnl.util.filesystem import join_path, mkdirp
 
 def pre_install(pkg):
     """This hook handles global license setup for licensed software."""
-    if pkg.license_required:
+    if pkg.license_required and not pkg.spec.external:
         set_up_license(pkg)
 
 
@@ -145,7 +145,7 @@ def write_license_file(pkg, license_path):
 def post_install(pkg):
     """This hook symlinks local licenses to the global license for
     licensed software."""
-    if pkg.license_required:
+    if pkg.license_required and not pkg.spec.external:
         symlink_license(pkg)
 
 
diff --git a/lib/spack/spack/hooks/sbang.py b/lib/spack/spack/hooks/sbang.py
index 6f9736a018..fb824470e2 100644
--- a/lib/spack/spack/hooks/sbang.py
+++ b/lib/spack/spack/hooks/sbang.py
@@ -104,8 +104,12 @@ def filter_shebangs_in_directory(directory, filenames=None):
 
 def post_install(pkg):
     """This hook edits scripts so that they call /bin/bash
-       $spack_prefix/bin/sbang instead of something longer than the
-       shebang limit."""
+    $spack_prefix/bin/sbang instead of something longer than the
+    shebang limit.
+    """
+    if pkg.spec.external:
+        tty.debug('SKIP: shebang filtering [external package]')
+        return
 
     for directory, _, filenames in os.walk(pkg.prefix):
         filter_shebangs_in_directory(directory, filenames)
diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py
index 0402926590..bc479c9cf5 100644
--- a/lib/spack/spack/package.py
+++ b/lib/spack/spack/package.py
@@ -1081,6 +1081,51 @@ class PackageBase(with_metaclass(PackageMeta, object)):
             with spack.store.db.prefix_write_lock(self.spec):
                 yield
 
+    def _process_external_package(self, explicit):
+        """Helper function to process external packages. It runs post install
+        hooks and registers the package in the DB.
+
+        :param bool explicit: True if the package was requested explicitly by
+            the user, False if it was pulled in as a dependency of an explicit
+            package.
+        """
+        if self.spec.external_module:
+            message = '{s.name}@{s.version} : has external module in {module}'
+            tty.msg(message.format(s=self, module=self.spec.external_module))
+            message = '{s.name}@{s.version} : is actually installed in {path}'
+            tty.msg(message.format(s=self, path=self.spec.external_path))
+        else:
+            message = '{s.name}@{s.version} : externally installed in {path}'
+            tty.msg(message.format(s=self, path=self.spec.external_path))
+        try:
+            # Check if the package was already registered in the DB
+            # If this is the case, then just exit
+            rec = spack.store.db.get_record(self.spec)
+            message = '{s.name}@{s.version} : already registered in DB'
+            tty.msg(message.format(s=self))
+            # Update the value of rec.explicit if it is necessary
+            self._update_explicit_entry_in_db(rec, explicit)
+
+        except KeyError:
+            # If not register it and generate the module file
+            # For external packages we just need to run
+            # post-install hooks to generate module files
+            message = '{s.name}@{s.version} : generating module file'
+            tty.msg(message.format(s=self))
+            spack.hooks.post_install(self)
+            # Add to the DB
+            message = '{s.name}@{s.version} : registering into DB'
+            tty.msg(message.format(s=self))
+            spack.store.db.add(self.spec, None, explicit=explicit)
+
+    def _update_explicit_entry_in_db(self, rec, explicit):
+        if explicit and not rec.explicit:
+            with spack.store.db.write_transaction():
+                rec = spack.store.db.get_record(self.spec)
+                rec.explicit = True
+                message = '{s.name}@{s.version} : marking the package explicit'
+                tty.msg(message.format(s=self))
+
     def do_install(self,
                    keep_prefix=False,
                    keep_stage=False,
@@ -1121,11 +1166,10 @@ class PackageBase(with_metaclass(PackageMeta, object)):
             raise ValueError("Can only install concrete packages: %s."
                              % self.spec.name)
 
-        # No installation needed if package is external
+        # For external packages the workflow is simplified, and basically
+        # consists in module file generation and registration in the DB
         if self.spec.external:
-            tty.msg("%s is externally installed in %s" %
-                    (self.name, self.spec.external))
-            return
+            return self._process_external_package(explicit)
 
         # Ensure package is not already installed
         layout = spack.store.layout
@@ -1135,14 +1179,10 @@ class PackageBase(with_metaclass(PackageMeta, object)):
                 tty.msg(
                     "Continuing from partial install of %s" % self.name)
             elif layout.check_installed(self.spec):
-                tty.msg(
-                    "%s is already installed in %s" % (self.name, self.prefix))
+                msg = '{0.name} is already installed in {0.prefix}'
+                tty.msg(msg.format(self))
                 rec = spack.store.db.get_record(self.spec)
-                if (not rec.explicit) and explicit:
-                    with spack.store.db.write_transaction():
-                        rec = spack.store.db.get_record(self.spec)
-                        rec.explicit = True
-                return
+                return self._update_explicit_entry_in_db(rec, explicit)
 
         # Dirty argument takes precedence over dirty config setting.
         if dirty is None:
@@ -1150,10 +1190,9 @@ class PackageBase(with_metaclass(PackageMeta, object)):
 
         self._do_install_pop_kwargs(kwargs)
 
-        tty.msg("Installing %s" % self.name)
-
         # First, install dependencies recursively.
         if install_deps:
+            tty.debug('Installing {0} dependencies'.format(self.name))
             for dep in self.spec.dependencies():
                 dep.package.do_install(
                     keep_prefix=keep_prefix,
@@ -1168,6 +1207,8 @@ class PackageBase(with_metaclass(PackageMeta, object)):
                     **kwargs
                 )
 
+        tty.msg('Installing %s' % self.name)
+
         # Set run_tests flag before starting build.
         self.run_tests = run_tests
 
@@ -1265,7 +1306,7 @@ class PackageBase(with_metaclass(PackageMeta, object)):
             # Fork a child to do the actual installation
             spack.build_environment.fork(self, build_process, dirty=dirty)
             # If we installed then we should keep the prefix
-            keep_prefix = True if self.last_phase is None else keep_prefix
+            keep_prefix = self.last_phase is None or keep_prefix
             # note: PARENT of the build process adds the new package to
             # the database, so that we don't need to re-read from file.
             spack.store.db.add(
@@ -1490,12 +1531,19 @@ class PackageBase(with_metaclass(PackageMeta, object)):
                 spack.hooks.pre_uninstall(pkg)
 
             # Uninstalling in Spack only requires removing the prefix.
-            spack.store.layout.remove_install_directory(spec)
+            if not spec.external:
+                msg = 'Deleting package prefix [{0}]'
+                tty.debug(msg.format(spec.short_spec))
+                spack.store.layout.remove_install_directory(spec)
+            # Delete DB entry
+            msg = 'Deleting DB entry [{0}]'
+            tty.debug(msg.format(spec.short_spec))
             spack.store.db.remove(spec)
+        tty.msg("Successfully uninstalled %s" % spec.short_spec)
 
-            # TODO: refactor hooks to use specs, not packages.
-            if pkg is not None:
-                spack.hooks.post_uninstall(pkg)
+        # TODO: refactor hooks to use specs, not packages.
+        if pkg is not None:
+            spack.hooks.post_uninstall(pkg)
 
         tty.msg("Successfully uninstalled %s" % spec.short_spec)
 
diff --git a/lib/spack/spack/package_prefs.py b/lib/spack/spack/package_prefs.py
index f9dac2bef0..8b1fe08154 100644
--- a/lib/spack/spack/package_prefs.py
+++ b/lib/spack/spack/package_prefs.py
@@ -29,6 +29,7 @@ from llnl.util.lang import classproperty
 
 import spack
 import spack.error
+from spack.util.path import canonicalize_path
 from spack.version import *
 
 
@@ -199,7 +200,7 @@ def spec_externals(spec):
     """Return a list of external specs (w/external directory path filled in),
        one for each known external installation."""
     # break circular import.
-    from spack.build_environment import get_path_from_module
+    from spack.build_environment import get_path_from_module  # noqa: F401
 
     allpkgs = get_packages_config()
     name = spec.name
@@ -215,7 +216,8 @@ def spec_externals(spec):
             # skip entries without paths (avoid creating extra Specs)
             continue
 
-        external_spec = spack.spec.Spec(external_spec, external=path)
+        external_spec = spack.spec.Spec(external_spec,
+                                        external_path=canonicalize_path(path))
         if external_spec.satisfies(spec):
             external_specs.append(external_spec)
 
@@ -223,10 +225,8 @@ def spec_externals(spec):
         if not module:
             continue
 
-        path = get_path_from_module(module)
-
         external_spec = spack.spec.Spec(
-            external_spec, external=path, external_module=module)
+            external_spec, external_module=module)
         if external_spec.satisfies(spec):
             external_specs.append(external_spec)
 
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index b4170a0f7a..3d067e083f 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -954,7 +954,7 @@ class Spec(object):
         self._concrete = kwargs.get('concrete', False)
 
         # Allow a spec to be constructed with an external path.
-        self.external  = kwargs.get('external', None)
+        self.external_path = kwargs.get('external_path', None)
         self.external_module = kwargs.get('external_module', None)
 
         # This allows users to construct a spec DAG with literals.
@@ -976,6 +976,10 @@ class Spec(object):
             self._add_dependency(spec, deptypes)
             deptypes = ()
 
+    @property
+    def external(self):
+        return bool(self.external_path) or bool(self.external_module)
+
     def get_dependency(self, name):
         dep = self._dependencies.get(name)
         if dep is not None:
@@ -1349,6 +1353,12 @@ class Spec(object):
         if params:
             d['parameters'] = params
 
+        if self.external:
+            d['external'] = {
+                'path': self.external_path,
+                'module': bool(self.external_module)
+            }
+
         # TODO: restore build dependencies here once we have less picky
         # TODO: concretization.
         deps = self.dependencies_dict(deptype=('link', 'run'))
@@ -1412,6 +1422,22 @@ class Spec(object):
             for name in FlagMap.valid_compiler_flags():
                 spec.compiler_flags[name] = []
 
+        if 'external' in node:
+            spec.external_path = None
+            spec.external_module = None
+            # This conditional is needed because sometimes this function is
+            # called with a node already constructed that contains a 'versions'
+            # and 'external' field. Related to virtual packages provider
+            # indexes.
+            if node['external']:
+                spec.external_path = node['external']['path']
+                spec.external_module = node['external']['module']
+                if spec.external_module is False:
+                    spec.external_module = None
+        else:
+            spec.external_path = None
+            spec.external_module = None
+
         # Don't read dependencies here; from_node_dict() is used by
         # from_yaml() to read the root *and* each dependency spec.
 
@@ -1578,6 +1604,8 @@ class Spec(object):
             done = True
             for spec in list(self.traverse()):
                 replacement = None
+                if spec.external:
+                    continue
                 if spec.virtual:
                     replacement = self._find_provider(spec, self_index)
                     if replacement:
@@ -1614,7 +1642,7 @@ class Spec(object):
                             continue
 
                 # If replacement is external then trim the dependencies
-                if replacement.external or replacement.external_module:
+                if replacement.external:
                     if (spec._dependencies):
                         changed = True
                         spec._dependencies = DependencyMap()
@@ -1632,7 +1660,8 @@ class Spec(object):
                         feq(replacement.architecture, spec.architecture) and
                         feq(replacement._dependencies, spec._dependencies) and
                         feq(replacement.variants, spec.variants) and
-                        feq(replacement.external, spec.external) and
+                        feq(replacement.external_path,
+                            spec.external_path) and
                         feq(replacement.external_module,
                             spec.external_module)):
                     continue
@@ -1691,14 +1720,14 @@ class Spec(object):
             if s.namespace is None:
                 s.namespace = spack.repo.repo_for_pkg(s.name).namespace
 
-        for s in self.traverse(root=False):
+        for s in self.traverse():
             if s.external_module:
                 compiler = spack.compilers.compiler_for_spec(
                     s.compiler, s.architecture)
                 for mod in compiler.modules:
                     load_module(mod)
 
-                s.external = get_path_from_module(s.external_module)
+                s.external_path = get_path_from_module(s.external_module)
 
         # Mark everything in the spec as concrete, as well.
         self._mark_concrete()
@@ -1919,7 +1948,7 @@ class Spec(object):
 
         # if we descend into a virtual spec, there's nothing more
         # to normalize.  Concretize will finish resolving it later.
-        if self.virtual or self.external or self.external_module:
+        if self.virtual or self.external:
             return False
 
         # Combine constraints from package deps with constraints from
@@ -2325,7 +2354,7 @@ class Spec(object):
                        self.variants != other.variants and
                        self._normal != other._normal and
                        self.concrete != other.concrete and
-                       self.external != other.external and
+                       self.external_path != other.external_path and
                        self.external_module != other.external_module and
                        self.compiler_flags != other.compiler_flags)
 
@@ -2341,7 +2370,7 @@ class Spec(object):
         self.compiler_flags = other.compiler_flags.copy()
         self.variants = other.variants.copy()
         self.variants.spec = self
-        self.external = other.external
+        self.external_path = other.external_path
         self.external_module = other.external_module
         self.namespace = other.namespace
 
@@ -2988,7 +3017,7 @@ class SpecParser(spack.parse.Parser):
         spec.variants = VariantMap(spec)
         spec.architecture = None
         spec.compiler = None
-        spec.external = None
+        spec.external_path = None
         spec.external_module = None
         spec.compiler_flags = FlagMap(spec)
         spec._dependents = DependencyMap()
diff --git a/lib/spack/spack/test/cmd/uninstall.py b/lib/spack/spack/test/cmd/uninstall.py
index bfbb9b8148..5765f4613d 100644
--- a/lib/spack/spack/test/cmd/uninstall.py
+++ b/lib/spack/spack/test/cmd/uninstall.py
@@ -53,7 +53,7 @@ def test_uninstall(database):
     uninstall(parser, args)
 
     all_specs = spack.store.layout.all_specs()
-    assert len(all_specs) == 7
+    assert len(all_specs) == 8
     # query specs with multiple configurations
     mpileaks_specs = [s for s in all_specs if s.satisfies('mpileaks')]
     callpath_specs = [s for s in all_specs if s.satisfies('callpath')]
diff --git a/lib/spack/spack/test/concretize.py b/lib/spack/spack/test/concretize.py
index 3b383584ce..2063088184 100644
--- a/lib/spack/spack/test/concretize.py
+++ b/lib/spack/spack/test/concretize.py
@@ -293,7 +293,7 @@ class TestConcretize(object):
     def test_external_package(self):
         spec = Spec('externaltool%gcc')
         spec.concretize()
-        assert spec['externaltool'].external == '/path/to/external_tool'
+        assert spec['externaltool'].external_path == '/path/to/external_tool'
         assert 'externalprereq' not in spec
         assert spec['externaltool'].compiler.satisfies('gcc')
 
@@ -322,8 +322,8 @@ class TestConcretize(object):
     def test_external_and_virtual(self):
         spec = Spec('externaltest')
         spec.concretize()
-        assert spec['externaltool'].external == '/path/to/external_tool'
-        assert spec['stuff'].external == '/path/to/external_virtual_gcc'
+        assert spec['externaltool'].external_path == '/path/to/external_tool'
+        assert spec['stuff'].external_path == '/path/to/external_virtual_gcc'
         assert spec['externaltool'].compiler.satisfies('gcc')
         assert spec['stuff'].compiler.satisfies('gcc')
 
diff --git a/lib/spack/spack/test/concretize_preferences.py b/lib/spack/spack/test/concretize_preferences.py
index bf915064b2..0b9f7a0046 100644
--- a/lib/spack/spack/test/concretize_preferences.py
+++ b/lib/spack/spack/test/concretize_preferences.py
@@ -170,4 +170,4 @@ mpich:
         # ensure that once config is in place, external is used
         spec = Spec('mpi')
         spec.concretize()
-        assert spec['mpich'].external == '/dummy/path'
+        assert spec['mpich'].external_path == '/dummy/path'
diff --git a/lib/spack/spack/test/conftest.py b/lib/spack/spack/test/conftest.py
index 796d95a0cf..120425794f 100644
--- a/lib/spack/spack/test/conftest.py
+++ b/lib/spack/spack/test/conftest.py
@@ -252,6 +252,7 @@ def database(tmpdir_factory, builtin_mock, config):
             _install('mpileaks ^mpich')
             _install('mpileaks ^mpich2')
             _install('mpileaks ^zmpi')
+            _install('externaltest')
 
     t = Database(
         real=real,
@@ -265,6 +266,7 @@ def database(tmpdir_factory, builtin_mock, config):
         t.install('mpileaks ^mpich')
         t.install('mpileaks ^mpich2')
         t.install('mpileaks ^zmpi')
+        t.install('externaltest')
 
     yield t
 
diff --git a/lib/spack/spack/test/database.py b/lib/spack/spack/test/database.py
index 0d7999cd36..a4b35e1df7 100644
--- a/lib/spack/spack/test/database.py
+++ b/lib/spack/spack/test/database.py
@@ -86,11 +86,17 @@ def _check_merkleiness():
 
 def _check_db_sanity(install_db):
     """Utiilty function to check db against install layout."""
-    expected = sorted(spack.store.layout.all_specs())
+    pkg_in_layout = sorted(spack.store.layout.all_specs())
     actual = sorted(install_db.query())
 
-    assert len(expected) == len(actual)
-    for e, a in zip(expected, actual):
+    externals = sorted([x for x in actual if x.external])
+    nexpected = len(pkg_in_layout) + len(externals)
+
+    assert nexpected == len(actual)
+
+    non_external_in_db = sorted([x for x in actual if not x.external])
+
+    for e, a in zip(pkg_in_layout, non_external_in_db):
         assert e == a
 
     _check_merkleiness()
@@ -127,7 +133,7 @@ def test_005_db_exists(database):
 def test_010_all_install_sanity(database):
     """Ensure that the install layout reflects what we think it does."""
     all_specs = spack.store.layout.all_specs()
-    assert len(all_specs) == 13
+    assert len(all_specs) == 14
 
     # Query specs with multiple configurations
     mpileaks_specs = [s for s in all_specs if s.satisfies('mpileaks')]
@@ -207,7 +213,7 @@ def test_050_basic_query(database):
     """Ensure querying database is consistent with what is installed."""
     install_db = database.mock.db
     # query everything
-    assert len(spack.store.db.query()) == 13
+    assert len(spack.store.db.query()) == 16
 
     # query specs with multiple configurations
     mpileaks_specs = install_db.query('mpileaks')
@@ -365,3 +371,22 @@ def test_110_no_write_with_exception_on_install(database):
     # reload DB and make sure cmake was not written.
     with install_db.read_transaction():
         assert install_db.query('cmake', installed=any) == []
+
+
+def test_external_entries_in_db(database):
+    install_db = database.mock.db
+
+    rec = install_db.get_record('mpileaks ^zmpi')
+    assert rec.spec.external_path is None
+    assert rec.spec.external_module is None
+
+    rec = install_db.get_record('externaltool')
+    assert rec.spec.external_path == '/path/to/external_tool'
+    assert rec.spec.external_module is None
+    assert rec.explicit is False
+
+    rec.spec.package.do_install(fake=True, explicit=True)
+    rec = install_db.get_record('externaltool')
+    assert rec.spec.external_path == '/path/to/external_tool'
+    assert rec.spec.external_module is None
+    assert rec.explicit is True
diff --git a/lib/spack/spack/test/spec_yaml.py b/lib/spack/spack/test/spec_yaml.py
index 0bcd2de3cf..adf262a60e 100644
--- a/lib/spack/spack/test/spec_yaml.py
+++ b/lib/spack/spack/test/spec_yaml.py
@@ -52,6 +52,16 @@ def test_normal_spec(builtin_mock):
     check_yaml_round_trip(spec)
 
 
+def test_external_spec(config, builtin_mock):
+    spec = Spec('externaltool')
+    spec.concretize()
+    check_yaml_round_trip(spec)
+
+    spec = Spec('externaltest')
+    spec.concretize()
+    check_yaml_round_trip(spec)
+
+
 def test_ambiguous_version_spec(builtin_mock):
     spec = Spec('mpileaks@1.0:5.0,6.1,7.3+debug~opt')
     spec.normalize()