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

scheibelp · web-flow · commit 2aec5b65f372 · 2022-04-12T09:58:26.000-07:00
Spack added support in spack#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
diff --git 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
@@ -2050,6 +2050,14 @@ def build_specs(self, function_tuples):
         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
@@ -5047,9 +5047,7 @@ def do_parse(self):
                 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
@@ -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
@@ -75,7 +75,17 @@ def write_file(filename, contents):
 
 
 @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,48 +126,59 @@ def commit(message):
         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()
 
         # 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
@@ -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
@@ -171,7 +171,7 @@ class Version(object):
         "version",
         "separators",
         "string",
-        "commit_lookup",
+        "_commit_lookup",
         "is_commit",
         "commit_version",
     ]
@@ -188,7 +188,7 @@ def __init__(self, string):
             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 @@ def intersection(self, other):
         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 @@ def generate_commit_lookup(self, pkg):
             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 @@ def lookup_commit(self, commit):
 
         # 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
diff --git a/var/spack/repos/builtin.mock/packages/git-test-commit/package.py b/var/spack/repos/builtin.mock/packages/git-test-commit/package.py
@@ -17,6 +17,8 @@ class GitTestCommit(Package):
     version('2.0', tag='v2.0')
 
     def install(self, spec, prefix):
+        # It is assumed for the test which installs this package, that it will
+        # be using the earliest commit, which is contained in the range @:0
         assert spec.satisfies('@:0')
         mkdir(prefix.bin)
 
