Git commit versions bugfix: Environments and Concretization (#29717)

Spack added support in #24639 for ad-hoc Git-commit-hash-based
versions: A user can install a package x@hash, where X is a package
that stores its source code in a Git repository, and the hash refers
to a commit in that repository which is not recorded as an explicit
version in the package.py file for X.

A couple issues were found relating to this:

* If an environment defines an alternative package repo (i.e. with
  repos.yaml), and spack.yaml contains user Specs with ad-hoc
  Git-commit-hash-based versions for packages in that repo,
  then as part of retrieving the data needed for version comparisons
  it will attempt to retrieve the package before the environment's
  configuration is instantiated.
* The bookkeeping information added to compare ad-hoc git versions was
  being stripped from Specs during concretization (such that user
  Specs which succeeded before concretizing would then fail after)

This addresses the issues:

* The first issue is resolved by deferring access to the associated
  Package until the versions are actually compared to one another.
* The second issue is resolved by ensuring that the Git bookkeeping
  information is explicitly applied to Specs after they are concretized.

This also:

* Resolves an ambiguity in the mock_git_version_info fixture used to
  create a tree of Git commits and provide a list where each index
  maps to a known commit.
* Isolates the cache used for Git repositories in tests using the
  mock_git_version_info fixture
* Adds a TODO which points out that if the remote Git repository
  overwrites tags, that Spack will then fail when using
  ad-hoc Git-commit-hash-based versions

7 files changed, 127 insertions, 32 deletions

diff --git a/lib/spack/spack/fetch_strategy.py b/lib/spack/spack/fetch_strategy.py
index 834a408443..1a7976bf64 100644
--- a/lib/spack/spack/fetch_strategy.py
+++ b/lib/spack/spack/fetch_strategy.py
@@ -1595,7 +1595,7 @@ def for_package_version(pkg, version):
     # if it's a commit, we must use a GitFetchStrategy
     if version.is_commit and hasattr(pkg, "git"):
         # Populate the version with comparisons to other commits
-        version.generate_commit_lookup(pkg)
+        version.generate_commit_lookup(pkg.name)
         fetcher = GitFetchStrategy(git=pkg.git, commit=str(version))
         return fetcher
 
diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py
index bbce46026d..6a79f37acd 100644
--- a/lib/spack/spack/solver/asp.py
+++ b/lib/spack/spack/solver/asp.py
@@ -2050,6 +2050,14 @@ class SpecBuilder(object):
         for s in self._specs.values():
             spack.spec.Spec.ensure_no_deprecated(s)
 
+        # Add git version lookup info to concrete Specs (this is generated for
+        # abstract specs as well but the Versions may be replaced during the
+        # concretization process)
+        for root in self._specs.values():
+            for spec in root.traverse():
+                if spec.version.is_commit:
+                    spec.version.generate_commit_lookup(spec.fullname)
+
         return self._specs
 
 
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index b4c778ee76..e933984a06 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -5047,9 +5047,7 @@ class SpecParser(spack.parse.Parser):
                 spec.name and spec.versions.concrete and
                 isinstance(spec.version, vn.Version) and spec.version.is_commit
             ):
-                pkg = spec.package
-                if hasattr(pkg, 'git'):
-                    spec.version.generate_commit_lookup(pkg)
+                spec.version.generate_commit_lookup(spec.fullname)
 
         return specs
 
diff --git a/lib/spack/spack/test/cmd/install.py b/lib/spack/spack/test/cmd/install.py
index 3fdfe24aa4..402b500c3e 100644
--- a/lib/spack/spack/test/cmd/install.py
+++ b/lib/spack/spack/test/cmd/install.py
@@ -263,6 +263,7 @@ def test_install_commit(
                         'git', 'file://%s' % repo_path,
                         raising=False)
 
+    # Use the earliest commit in the respository
     commit = commits[-1]
     spec = spack.spec.Spec('git-test-commit@%s' % commit)
     spec.concretize()
diff --git a/lib/spack/spack/test/conftest.py b/lib/spack/spack/test/conftest.py
index 32c73af18c..d4d2c64aaa 100644
--- a/lib/spack/spack/test/conftest.py
+++ b/lib/spack/spack/test/conftest.py
@@ -75,7 +75,17 @@ commit_counter = 0
 
 
 @pytest.fixture
-def mock_git_version_info(tmpdir, scope="function"):
+def override_git_repos_cache_path(tmpdir):
+    saved = spack.paths.user_repos_cache_path
+    tmp_path = tmpdir.mkdir('git-repo-cache-path-for-tests')
+    spack.paths.user_repos_cache_path = str(tmp_path)
+    yield
+    spack.paths.user_repos_cache_path = saved
+
+
+@pytest.fixture
+def mock_git_version_info(tmpdir, override_git_repos_cache_path,
+                          scope="function"):
     """Create a mock git repo with known structure
 
     The structure of commits in this repo is as follows::
@@ -116,10 +126,16 @@ def mock_git_version_info(tmpdir, scope="function"):
         git('config', 'user.name', 'Spack')
         git('config', 'user.email', 'spack@spack.io')
 
+        commits = []
+
+        def latest_commit():
+            return git('rev-list', '-n1', 'HEAD', output=str, error=str).strip()
+
         # Add two commits on main branch
         write_file(filename, '[]')
         git('add', filename)
         commit('first commit')
+        commits.append(latest_commit())
 
         # Get name of default branch (differs by git version)
         main = git('rev-parse', '--abbrev-ref', 'HEAD', output=str, error=str).strip()
@@ -127,37 +143,42 @@ def mock_git_version_info(tmpdir, scope="function"):
         # Tag second commit as v1.0
         write_file(filename, "[1, 0]")
         commit('second commit')
+        commits.append(latest_commit())
         git('tag', 'v1.0')
 
         # Add two commits and a tag on 1.x branch
         git('checkout', '-b', '1.x')
         write_file(filename, "[1, 0, '', 1]")
         commit('first 1.x commit')
+        commits.append(latest_commit())
 
         write_file(filename, "[1, 1]")
         commit('second 1.x commit')
+        commits.append(latest_commit())
         git('tag', 'v1.1')
 
         # Add two commits and a tag on main branch
         git('checkout', main)
         write_file(filename, "[1, 0, '', 1]")
         commit('third main commit')
+        commits.append(latest_commit())
         write_file(filename, "[2, 0]")
         commit('fourth main commit')
+        commits.append(latest_commit())
         git('tag', 'v2.0')
 
         # Add two more commits on 1.x branch to ensure we aren't cheating by using time
         git('checkout', '1.x')
         write_file(filename, "[1, 1, '', 1]")
         commit('third 1.x commit')
+        commits.append(latest_commit())
         write_file(filename, "[1, 2]")
         commit('fourth 1.x commit')
+        commits.append(latest_commit())
         git('tag', '1.2')  # test robust parsing to different syntax, no v
 
-        # Get the commits in topo order
-        log = git('log', '--all', '--pretty=format:%H', '--topo-order',
-                  output=str, error=str)
-        commits = [c for c in log.split('\n') if c]
+        # The commits are ordered with the last commit first in the list
+        commits = list(reversed(commits))
 
     # Return the git directory to install, the filename used, and the commits
     yield repo_path, filename, commits
diff --git a/lib/spack/spack/test/versions.py b/lib/spack/spack/test/versions.py
index 9b89222162..dcffcf379f 100644
--- a/lib/spack/spack/test/versions.py
+++ b/lib/spack/spack/test/versions.py
@@ -607,6 +607,36 @@ def test_versions_from_git(mock_git_version_info, monkeypatch, mock_packages):
         assert str(comparator) == expected
 
 
+@pytest.mark.skipif(sys.platform == 'win32',
+                    reason="Not supported on Windows (yet)")
+def test_git_hash_comparisons(
+        mock_git_version_info, install_mockery, mock_packages, monkeypatch):
+    """Check that hashes compare properly to versions
+    """
+    repo_path, filename, commits = mock_git_version_info
+    monkeypatch.setattr(spack.package.PackageBase,
+                        'git', 'file://%s' % repo_path,
+                        raising=False)
+
+    # Spec based on earliest commit
+    spec0 = spack.spec.Spec('git-test-commit@%s' % commits[-1])
+    spec0.concretize()
+    assert spec0.satisfies('@:0')
+    assert not spec0.satisfies('@1.0')
+
+    # Spec based on second commit (same as version 1.0)
+    spec1 = spack.spec.Spec('git-test-commit@%s' % commits[-2])
+    spec1.concretize()
+    assert spec1.satisfies('@1.0')
+    assert not spec1.satisfies('@1.1:')
+
+    # Spec based on 4th commit (in timestamp order)
+    spec4 = spack.spec.Spec('git-test-commit@%s' % commits[-4])
+    spec4.concretize()
+    assert spec4.satisfies('@1.1')
+    assert spec4.satisfies('@1.0:1.2')
+
+
 def test_version_range_nonempty():
     assert Version('1.2.9') in VersionRange('1.2.0', '1.2')
     assert Version('1.1.1') in ver('1.0:1')
diff --git a/lib/spack/spack/version.py b/lib/spack/spack/version.py
index d3dd01c3a5..495cfee6e3 100644
--- a/lib/spack/spack/version.py
+++ b/lib/spack/spack/version.py
@@ -171,7 +171,7 @@ class Version(object):
         "version",
         "separators",
         "string",
-        "commit_lookup",
+        "_commit_lookup",
         "is_commit",
         "commit_version",
     ]
@@ -188,7 +188,7 @@ class Version(object):
             raise ValueError("Bad characters in version string: %s" % string)
 
         # An object that can lookup git commits to compare them to versions
-        self.commit_lookup = None
+        self._commit_lookup = None
         self.commit_version = None
         segments = SEGMENT_REGEX.findall(string)
         self.version = tuple(
@@ -467,7 +467,13 @@ class Version(object):
         else:
             return VersionList()
 
-    def generate_commit_lookup(self, pkg):
+    @property
+    def commit_lookup(self):
+        if self._commit_lookup:
+            self._commit_lookup.get(self.string)
+            return self._commit_lookup
+
+    def generate_commit_lookup(self, pkg_name):
         """
         Use the git fetcher to look up a version for a commit.
 
@@ -483,17 +489,13 @@ class Version(object):
             fetcher: the fetcher to use.
             versions: the known versions of the package
         """
-        if self.commit_lookup:
-            return
 
         # Sanity check we have a commit
         if not self.is_commit:
             tty.die("%s is not a commit." % self)
 
         # Generate a commit looker-upper
-        self.commit_lookup = CommitLookup(pkg)
-        self.commit_lookup.get(self.string)
-        self.commit_lookup.save()
+        self._commit_lookup = CommitLookup(pkg_name)
 
 
 class VersionRange(object):
@@ -991,24 +993,55 @@ class CommitLookup(object):
     Version.is_commit returns True to allow for comparisons between git commits
     and versions as represented by tags in the git repository.
     """
-    def __init__(self, pkg):
-        self.pkg = pkg
-
-        # We require the full git repository history
-        import spack.fetch_strategy  # break cycle
-        fetcher = spack.fetch_strategy.GitFetchStrategy(git=pkg.git)
-        fetcher.get_full_repo = True
-        self.fetcher = fetcher
+    def __init__(self, pkg_name):
+        self.pkg_name = pkg_name
 
         self.data = {}
 
-        # Cache data in misc_cache
-        key_base = 'git_metadata'
-        if not self.repository_uri.startswith('/'):
-            key_base += '/'
-        self.cache_key = key_base + self.repository_uri
-        spack.caches.misc_cache.init_entry(self.cache_key)
-        self.cache_path = spack.caches.misc_cache.cache_path(self.cache_key)
+        self._pkg = None
+        self._fetcher = None
+        self._cache_key = None
+        self._cache_path = None
+
+    # The following properties are used as part of a lazy reference scheme
+    # to avoid querying the package repository until it is necessary (and
+    # in particular to wait until after the configuration has been
+    # assembled)
+    @property
+    def cache_key(self):
+        if not self._cache_key:
+            key_base = 'git_metadata'
+            if not self.repository_uri.startswith('/'):
+                key_base += '/'
+            self._cache_key = key_base + self.repository_uri
+
+            # Cache data in misc_cache
+            # If this is the first lazy access, initialize the cache as well
+            spack.caches.misc_cache.init_entry(self.cache_key)
+        return self._cache_key
+
+    @property
+    def cache_path(self):
+        if not self._cache_path:
+            self._cache_path = spack.caches.misc_cache.cache_path(
+                self.cache_key)
+        return self._cache_path
+
+    @property
+    def pkg(self):
+        if not self._pkg:
+            self._pkg = spack.repo.get(self.pkg_name)
+        return self._pkg
+
+    @property
+    def fetcher(self):
+        if not self._fetcher:
+            # We require the full git repository history
+            import spack.fetch_strategy  # break cycle
+            fetcher = spack.fetch_strategy.GitFetchStrategy(git=self.pkg.git)
+            fetcher.get_full_repo = True
+            self._fetcher = fetcher
+        return self._fetcher
 
     @property
     def repository_uri(self):
@@ -1073,6 +1106,10 @@ class CommitLookup(object):
 
         # Lookup commit info
         with working_dir(dest):
+            # TODO: we need to update the local tags if they changed on the
+            # remote instance, simply adding '-f' may not be sufficient
+            # (if commits are deleted on the remote, this command alone
+            # won't properly update the local rev-list)
             self.fetcher.git("fetch", '--tags')
 
             # Ensure commit is an object known to git