Requirements and preferences should not define (non-git) versions (#37687)

Ensure that requirements `packages:*:require:@x` and preferences `packages:*:version:[x]`
fail concretization when no version defined in the package satisfies `x`. This always holds
except for git versions -- they are defined on the fly.

6 files changed, 177 insertions, 11 deletions

diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py
index 220d3440ae..78c53f44d5 100644
--- a/lib/spack/spack/solver/asp.py
+++ b/lib/spack/spack/solver/asp.py
@@ -1759,13 +1759,30 @@ class SpackSolverSetup(object):
             # All the preferred version from packages.yaml, versions in external
             # specs will be computed later
             version_preferences = packages_yaml.get(pkg_name, {}).get("version", [])
-            for idx, v in enumerate(version_preferences):
-                # v can be a string so force it into an actual version for comparisons
-                ver = vn.Version(v)
+            version_defs = []
+            pkg_class = spack.repo.path.get_pkg_class(pkg_name)
+            for vstr in version_preferences:
+                v = vn.ver(vstr)
+                if isinstance(v, vn.GitVersion):
+                    version_defs.append(v)
+                else:
+                    satisfying_versions = list(x for x in pkg_class.versions if x.satisfies(v))
+                    if not satisfying_versions:
+                        raise spack.config.ConfigError(
+                            "Preference for version {0} does not match any version "
+                            " defined in {1}".format(str(v), pkg_name)
+                        )
+                    # Amongst all defined versions satisfying this specific
+                    # preference, the highest-numbered version is the
+                    # most-preferred: therefore sort satisfying versions
+                    # from greatest to least
+                    version_defs.extend(sorted(satisfying_versions, reverse=True))
+
+            for weight, vdef in enumerate(llnl.util.lang.dedupe(version_defs)):
                 self.declared_versions[pkg_name].append(
-                    DeclaredVersion(version=ver, idx=idx, origin=Provenance.PACKAGES_YAML)
+                    DeclaredVersion(version=vdef, idx=weight, origin=Provenance.PACKAGES_YAML)
                 )
-                self.possible_versions[pkg_name].add(ver)
+                self.possible_versions[pkg_name].add(vdef)
 
     def add_concrete_versions_from_specs(self, specs, origin):
         """Add concrete versions to possible versions from lists of CLI/dev specs."""
@@ -2296,6 +2313,11 @@ def _get_versioned_specs_from_pkg_requirements():
 
 
 def _specs_from_requires(pkg_name, section):
+    """Collect specs from requirements which define versions (i.e. those that
+    have a concrete version). Requirements can define *new* versions if
+    they are included as part of an equivalence (hash=number) but not
+    otherwise.
+    """
     if isinstance(section, str):
         spec = spack.spec.Spec(section)
         if not spec.name:
@@ -2324,7 +2346,30 @@ def _specs_from_requires(pkg_name, section):
                 spec.name = pkg_name
             extracted_specs.append(spec)
 
-    version_specs = [x for x in extracted_specs if x.versions.concrete]
+    version_specs = []
+    for spec in extracted_specs:
+        if spec.versions.concrete:
+            # Note: this includes git versions
+            version_specs.append(spec)
+            continue
+
+        # Prefer spec's name if it exists, in case the spec is
+        # requiring a specific implementation inside of a virtual section
+        # e.g. packages:mpi:require:openmpi@4.0.1
+        pkg_class = spack.repo.path.get_pkg_class(spec.name or pkg_name)
+        satisfying_versions = list(v for v in pkg_class.versions if v.satisfies(spec.versions))
+        if not satisfying_versions:
+            raise spack.config.ConfigError(
+                "{0} assigns a version that is not defined in"
+                " the associated package.py".format(str(spec))
+            )
+
+        # Version ranges ("@1.3" without the "=", "@1.2:1.4") and lists
+        # will end up here
+        ordered_satisfying_versions = sorted(satisfying_versions, reverse=True)
+        vspecs = list(spack.spec.Spec("@{0}".format(x)) for x in ordered_satisfying_versions)
+        version_specs.extend(vspecs)
+
     for spec in version_specs:
         spec.attach_git_version_lookup()
     return version_specs
diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py
index a52b2f3839..f26e5f7f3d 100644
--- a/lib/spack/spack/test/cmd/env.py
+++ b/lib/spack/spack/test/cmd/env.py
@@ -906,7 +906,7 @@ packages:
   mpileaks:
     version: ["2.2"]
   libelf:
-    version: ["0.8.11"]
+    version: ["0.8.10"]
 """
         )
 
diff --git a/lib/spack/spack/test/concretize_preferences.py b/lib/spack/spack/test/concretize_preferences.py
index 0a8d1c2ce6..76c3680f49 100644
--- a/lib/spack/spack/test/concretize_preferences.py
+++ b/lib/spack/spack/test/concretize_preferences.py
@@ -152,7 +152,9 @@ class TestConcretizePreferences(object):
         assert spec.version == Version("2.2")
 
     def test_preferred_versions_mixed_version_types(self):
-        update_packages("mixedversions", "version", ["2.0"])
+        if spack.config.get("config:concretizer") == "original":
+            pytest.skip("This behavior is not enforced for the old concretizer")
+        update_packages("mixedversions", "version", ["=2.0"])
         spec = concretize("mixedversions")
         assert spec.version == Version("2.0")
 
@@ -228,6 +230,29 @@ mpileaks:
         spec.concretize()
         assert spec.version == Version("3.5.0")
 
+    def test_preferred_undefined_raises(self):
+        """Preference should not specify an undefined version"""
+        if spack.config.get("config:concretizer") == "original":
+            pytest.xfail("This behavior is not enforced for the old concretizer")
+
+        update_packages("python", "version", ["3.5.0.1"])
+        spec = Spec("python")
+        with pytest.raises(spack.config.ConfigError):
+            spec.concretize()
+
+    def test_preferred_truncated(self):
+        """Versions without "=" are treated as version ranges: if there is
+        a satisfying version defined in the package.py, we should use that
+        (don't define a new version).
+        """
+        if spack.config.get("config:concretizer") == "original":
+            pytest.skip("This behavior is not enforced for the old concretizer")
+
+        update_packages("python", "version", ["3.5"])
+        spec = Spec("python")
+        spec.concretize()
+        assert spec.satisfies("@3.5.1")
+
     def test_develop(self):
         """Test concretization with develop-like versions"""
         spec = Spec("develop-test")
diff --git a/lib/spack/spack/test/concretize_requirements.py b/lib/spack/spack/test/concretize_requirements.py
index 52e831ca01..1f09b565f3 100644
--- a/lib/spack/spack/test/concretize_requirements.py
+++ b/lib/spack/spack/test/concretize_requirements.py
@@ -66,6 +66,28 @@ class V(Package):
 )
 
 
+_pkgt = (
+    "t",
+    """\
+class T(Package):
+    version('2.1')
+    version('2.0')
+
+    depends_on('u', when='@2.1:')
+""",
+)
+
+
+_pkgu = (
+    "u",
+    """\
+class U(Package):
+    version('1.1')
+    version('1.0')
+""",
+)
+
+
 @pytest.fixture
 def create_test_repo(tmpdir, mutable_config):
     repo_path = str(tmpdir)
@@ -79,7 +101,7 @@ repo:
         )
 
     packages_dir = tmpdir.join("packages")
-    for pkg_name, pkg_str in [_pkgx, _pkgy, _pkgv]:
+    for pkg_name, pkg_str in [_pkgx, _pkgy, _pkgv, _pkgt, _pkgu]:
         pkg_dir = packages_dir.ensure(pkg_name, dir=True)
         pkg_file = pkg_dir.join("package.py")
         with open(str(pkg_file), "w") as f:
@@ -144,6 +166,45 @@ packages:
         Spec("x@1.1").concretize()
 
 
+def test_require_undefined_version(concretize_scope, test_repo):
+    """If a requirement specifies a numbered version that isn't in
+    the associated package.py and isn't part of a Git hash
+    equivalence (hash=number), then Spack should raise an error
+    (it is assumed this is a typo, and raising the error here
+    avoids a likely error when Spack attempts to fetch the version).
+    """
+    if spack.config.get("config:concretizer") == "original":
+        pytest.skip("Original concretizer does not support configuration requirements")
+
+    conf_str = """\
+packages:
+  x:
+    require: "@1.2"
+"""
+    update_packages_config(conf_str)
+    with pytest.raises(spack.config.ConfigError):
+        Spec("x").concretize()
+
+
+def test_require_truncated(concretize_scope, test_repo):
+    """A requirement specifies a version range, with satisfying
+    versions defined in the package.py. Make sure we choose one
+    of the defined versions (vs. allowing the requirement to
+    define a new version).
+    """
+    if spack.config.get("config:concretizer") == "original":
+        pytest.skip("Original concretizer does not support configuration requirements")
+
+    conf_str = """\
+packages:
+  x:
+    require: "@1"
+"""
+    update_packages_config(conf_str)
+    xspec = Spec("x").concretized()
+    assert xspec.satisfies("@1.1")
+
+
 def test_git_user_supplied_reference_satisfaction(
     concretize_scope, test_repo, mock_git_version_info, monkeypatch
 ):
@@ -220,6 +281,40 @@ packages:
     assert s1.version.ref == a_commit_hash
 
 
+def test_requirement_adds_version_satisfies(
+    concretize_scope, test_repo, mock_git_version_info, monkeypatch
+):
+    """Make sure that new versions added by requirements are factored into
+    conditions. In this case create a new version that satisfies a
+    depends_on condition and make sure it is triggered (i.e. the
+    dependency is added).
+    """
+    if spack.config.get("config:concretizer") == "original":
+        pytest.skip("Original concretizer does not support configuration" " requirements")
+
+    repo_path, filename, commits = mock_git_version_info
+    monkeypatch.setattr(
+        spack.package_base.PackageBase, "git", path_to_file_url(repo_path), raising=False
+    )
+
+    # Sanity check: early version of T does not include U
+    s0 = Spec("t@2.0").concretized()
+    assert not ("u" in s0)
+
+    conf_str = """\
+packages:
+  t:
+    require: "@{0}=2.2"
+""".format(
+        commits[0]
+    )
+    update_packages_config(conf_str)
+
+    s1 = Spec("t").concretized()
+    assert "u" in s1
+    assert s1.satisfies("@2.2")
+
+
 def test_requirement_adds_git_hash_version(
     concretize_scope, test_repo, mock_git_version_info, monkeypatch
 ):
diff --git a/share/spack/gitlab/cloud_pipelines/stacks/data-vis-sdk/spack.yaml b/share/spack/gitlab/cloud_pipelines/stacks/data-vis-sdk/spack.yaml
index 60762b206b..178ada4ee5 100644
--- a/share/spack/gitlab/cloud_pipelines/stacks/data-vis-sdk/spack.yaml
+++ b/share/spack/gitlab/cloud_pipelines/stacks/data-vis-sdk/spack.yaml
@@ -6,9 +6,9 @@ spack:
     mesa:
       require: "+glx +osmesa +opengl ~opengles +llvm"
     libosmesa:
-      require: ^mesa +osmesa
+      require: "mesa +osmesa"
     libglx:
-      require: ^mesa +glx
+      require: "mesa +glx"
     ospray:
       require: "@2.8.0 +denoiser +mpi"
     llvm:
diff --git a/var/spack/repos/builtin.mock/packages/python/package.py b/var/spack/repos/builtin.mock/packages/python/package.py
index 0e440f33cb..4ba957e050 100644
--- a/var/spack/repos/builtin.mock/packages/python/package.py
+++ b/var/spack/repos/builtin.mock/packages/python/package.py
@@ -14,6 +14,7 @@ class Python(Package):
 
     extendable = True
 
+    version("3.7.1", md5="aaabbbcccdddeeefffaaabbbcccddd12")
     version("3.5.1", md5="be78e48cdfc1a7ad90efff146dce6cfe")
     version("3.5.0", md5="a56c0c0b45d75a0ec9c6dee933c41c36")
     version("2.7.11", md5="6b6076ec9e93f05dd63e47eb9c15728b", preferred=True)