Packages must be added to DB to be considered installed (#8038)

Fixes #8036

Before this PR Package.installed was returning True if the spec prefix
existed, without checking the DB. This is wrong for external packages,
whose prefix exists before being registered into the DB. Now the property
checks for both the prefix and a DB entry.

4 files changed, 64 insertions, 6 deletions

diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py
index f0aa67fc8f..c79237dbef 100644
--- a/lib/spack/spack/package.py
+++ b/lib/spack/spack/package.py
@@ -995,7 +995,23 @@ class PackageBase(with_metaclass(PackageMeta, PackageViewMixin, object)):
 
     @property
     def installed(self):
-        return os.path.isdir(self.prefix)
+        """Installation status of a package.
+
+        Returns:
+            True if the package has been installed, False otherwise.
+        """
+        has_prefix = os.path.isdir(self.prefix)
+        try:
+            # If the spec is in the DB, check the installed
+            # attribute of the record
+            rec = spack.store.db.get_record(self.spec)
+            db_says_installed = rec.installed
+        except KeyError:
+            # If the spec is not in the DB, the method
+            #  above raises a Key error
+            db_says_installed = False
+
+        return has_prefix and db_says_installed
 
     @property
     def prefix(self):
@@ -1650,11 +1666,18 @@ class PackageBase(with_metaclass(PackageMeta, PackageViewMixin, object)):
     def check_for_unfinished_installation(
             self, keep_prefix=False, restage=False):
         """Check for leftover files from partially-completed prior install to
-           prepare for a new install attempt. Options control whether these
-           files are reused (vs. destroyed). This function considers a package
-           fully-installed if there is a DB entry for it (in that way, it is
-           more strict than Package.installed). The return value is used to
-           indicate when the prefix exists but the install is not complete.
+        prepare for a new install attempt.
+
+        Options control whether these files are reused (vs. destroyed).
+
+        Args:
+            keep_prefix (bool): True if the installation prefix needs to be
+                kept, False otherwise
+            restage (bool): False if the stage has to be kept, True otherwise
+
+        Returns:
+            True if the prefix exists but the install is not complete, False
+            otherwise.
         """
         if self.spec.external:
             raise ExternalPackageError("Attempted to repair external spec %s" %
diff --git a/lib/spack/spack/test/data/packages.yaml b/lib/spack/spack/test/data/packages.yaml
index 923d63173a..c7256ddb33 100644
--- a/lib/spack/spack/test/data/packages.yaml
+++ b/lib/spack/spack/test/data/packages.yaml
@@ -3,6 +3,7 @@ packages:
     buildable: False
     paths:
       externaltool@1.0%gcc@4.5.0: /path/to/external_tool
+      externaltool@0.9%gcc@4.5.0: /usr
   externalvirtual:
     buildable: False
     paths:
diff --git a/lib/spack/spack/test/database.py b/lib/spack/spack/test/database.py
index 2e61853349..4c5db3089b 100644
--- a/lib/spack/spack/test/database.py
+++ b/lib/spack/spack/test/database.py
@@ -27,6 +27,7 @@ These tests check the database is functioning properly,
 both in memory and in its file
 """
 import datetime
+import functools
 import multiprocessing
 import os
 import pytest
@@ -42,6 +43,23 @@ from spack.util.executable import Executable
 pytestmark = pytest.mark.db
 
 
+@pytest.fixture()
+def usr_folder_exists(monkeypatch):
+    """The ``/usr`` folder is assumed to be existing in some tests. This
+    fixture makes it such that its existence is mocked, so we have no
+    requirements on the system running tests.
+    """
+    isdir = os.path.isdir
+
+    @functools.wraps(os.path.isdir)
+    def mock_isdir(path):
+        if path == '/usr':
+            return True
+        return isdir(path)
+
+    monkeypatch.setattr(os.path, 'isdir', mock_isdir)
+
+
 def _print_ref_counts():
     """Print out all ref counts for the graph used here, for debugging"""
     recs = []
@@ -436,3 +454,18 @@ def test_external_entries_in_db(database):
     assert rec.spec.external_path == '/path/to/external_tool'
     assert rec.spec.external_module is None
     assert rec.explicit is True
+
+
+@pytest.mark.regression('8036')
+def test_regression_issue_8036(mutable_database, usr_folder_exists):
+    # The test ensures that the external package prefix is treated as
+    # existing. Even when the package prefix exists, the package should
+    # not be considered installed until it is added to the database with
+    # do_install.
+    s = spack.spec.Spec('externaltool@0.9')
+    s.concretize()
+    assert not s.package.installed
+
+    # Now install the external package and check again the `installed` property
+    s.package.do_install(fake=True)
+    assert s.package.installed
diff --git a/var/spack/repos/builtin.mock/packages/externaltool/package.py b/var/spack/repos/builtin.mock/packages/externaltool/package.py
index c8867f3ef5..9f0e3f6f73 100644
--- a/var/spack/repos/builtin.mock/packages/externaltool/package.py
+++ b/var/spack/repos/builtin.mock/packages/externaltool/package.py
@@ -30,6 +30,7 @@ class Externaltool(Package):
     url      = "http://somewhere.com/tool-1.0.tar.gz"
 
     version('1.0', '1234567890abcdef1234567890abcdef')
+    version('0.9', '1234567890abcdef1234567890abcdef')
 
     depends_on('externalprereq')