bugfix: Not all concrete versions on the CLI should be considered real (#28620)

Some "concrete" versions on the command line, e.g. `qt@5` are really
meant to satisfy some actual concrete version from a package. We should
only assume the user is introducing a new, unknown version on the CLI
if we, well, don't know of any version that satisfies the user's
request.  So, if we know about `5.11.1` and `5.11.3` and they ask for
`5.11.2`, we'd ask the solver to consider `5.11.2` as a solution.  If
they just ask for `5`, though, `5.11.1` or `5.11.3` are fine solutions,
as they satisfy `@5`, so use them.

Co-authored-by: Harmen Stoppels <harmenstoppels@gmail.com>

5 files changed, 69 insertions, 24 deletions

diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py
index dfd2111453..bc26619d6c 100644
--- a/lib/spack/spack/solver/asp.py
+++ b/lib/spack/spack/solver/asp.py
@@ -24,7 +24,7 @@ try:
     # There may be a better way to detect this
     clingo_cffi = hasattr(clingo.Symbol, '_rep')
 except ImportError:
-    clingo = None
+    clingo = None  # type: ignore
     clingo_cffi = False
 
 import llnl.util.lang
@@ -1316,16 +1316,26 @@ class SpackSolverSetup(object):
 
         for spec in specs:
             for dep in spec.traverse():
-                if dep.versions.concrete:
-                    # Concrete versions used in abstract specs from cli. They
-                    # all have idx equal to 0, which is the best possible. In
-                    # any case they will be used due to being set from the cli.
-                    self.declared_versions[dep.name].append(DeclaredVersion(
-                        version=dep.version,
-                        idx=0,
-                        origin=version_provenance.spec
-                    ))
-                    self.possible_versions[dep.name].add(dep.version)
+                if not dep.versions.concrete:
+                    continue
+
+                known_versions = self.possible_versions[dep.name]
+                if (not dep.version.is_commit and
+                    any(v.satisfies(dep.version) for v in known_versions)):
+                    # some version we know about satisfies this constraint, so we
+                    # should use that one. e.g, if the user asks for qt@5 and we
+                    # know about qt@5.5.
+                    continue
+
+                # if there is a concrete version on the CLI *that we know nothing
+                # about*, add it to the known versions. Use idx=0, which is the
+                # best possible, so they're guaranteed to be used preferentially.
+                self.declared_versions[dep.name].append(DeclaredVersion(
+                    version=dep.version,
+                    idx=0,
+                    origin=version_provenance.spec
+                ))
+                self.possible_versions[dep.name].add(dep.version)
 
     def _supported_targets(self, compiler_name, compiler_version, targets):
         """Get a list of which targets are supported by the compiler.
diff --git a/lib/spack/spack/test/cmd/install.py b/lib/spack/spack/test/cmd/install.py
index 006cace185..81be6273b3 100644
--- a/lib/spack/spack/test/cmd/install.py
+++ b/lib/spack/spack/test/cmd/install.py
@@ -248,11 +248,11 @@ def test_install_overwrite_not_installed(
 
 def test_install_commit(
         mock_git_version_info, install_mockery, mock_packages, monkeypatch):
-    """
-    Test installing a git package from a commit.
+    """Test installing a git package from a commit.
+
+    This ensures Spack associates commit versions with their packages in time to do
+    version lookups. Details of version lookup tested elsewhere.
 
-    This ensures Spack appropriately associates commit versions with their
-    packages in time to do version lookups. Details of version lookup tested elsewhere
     """
     repo_path, filename, commits = mock_git_version_info
     monkeypatch.setattr(spack.package.PackageBase,
@@ -262,6 +262,7 @@ def test_install_commit(
     commit = commits[-1]
     spec = spack.spec.Spec('git-test-commit@%s' % commit)
     spec.concretize()
+    print(spec)
     spec.package.do_install()
 
     # Ensure first commit file contents were written
diff --git a/lib/spack/spack/test/concretize.py b/lib/spack/spack/test/concretize.py
index e08ef6b6aa..fc24a1d972 100644
--- a/lib/spack/spack/test/concretize.py
+++ b/lib/spack/spack/test/concretize.py
@@ -1376,3 +1376,16 @@ class TestConcretize(object):
 
         s = spack.spec.Spec('sticky-variant %clang').concretized()
         assert s.satisfies('%clang') and s.satisfies('~allow-gcc')
+
+    def test_do_not_invent_new_concrete_versions_unless_necessary(self):
+        if spack.config.get('config:concretizer') == 'original':
+            pytest.skip(
+                "Original concretizer doesn't resolve concrete versions to known ones"
+            )
+
+        # ensure we select a known satisfying version rather than creating
+        # a new '2.7' version.
+        assert ver("2.7.11") == Spec("python@2.7").concretized().version
+
+        # Here there is no known satisfying version - use the one on the spec.
+        assert ver("2.7.21") == Spec("python@2.7.21").concretized().version
diff --git a/lib/spack/spack/test/data/web/4.html b/lib/spack/spack/test/data/web/4.html
index b5fe850f4d..dfe51df053 100644
--- a/lib/spack/spack/test/data/web/4.html
+++ b/lib/spack/spack/test/data/web/4.html
@@ -7,5 +7,6 @@
 
     <a href="foo-4.5.tar.gz">foo-4.5.tar.gz.</a>
     <a href="foo-4.5-rc5.tar.gz">foo-4.1-rc5.tar.gz.</a>
+    <a href="foo-4.5.0.tar.gz">foo-4.5.0.tar.gz.</a>
   </body>
 </html>
diff --git a/lib/spack/spack/test/url_fetch.py b/lib/spack/spack/test/url_fetch.py
index b2ccb9cb50..6e00a3f481 100644
--- a/lib/spack/spack/test/url_fetch.py
+++ b/lib/spack/spack/test/url_fetch.py
@@ -195,22 +195,42 @@ def test_from_list_url(mock_packages, config, spec, url, digest, _fetch_method):
         assert fetch_strategy.extra_options == {'timeout': 60}
 
 
-@pytest.mark.parametrize('_fetch_method', ['curl', 'urllib'])
-def test_from_list_url_unspecified(mock_packages, config, _fetch_method):
+@pytest.mark.parametrize("_fetch_method", ["curl", "urllib"])
+@pytest.mark.parametrize("requested_version,tarball,digest", [
+    # This version is in the web data path (test/data/web/4.html), but not in the
+    # url-list-test package. We expect Spack to generate a URL with the new version.
+    ("4.5.0", "foo-4.5.0.tar.gz", None),
+    # This version is in web data path and not in the package file, BUT the 2.0.0b2
+    # version in the package file satisfies 2.0.0, so Spack will use the known version.
+    # TODO: this is *probably* not what the user wants, but it's here as an example
+    # TODO: for that reason. We can't express "exactly 2.0.0" right now, and we don't
+    # TODO: have special cases that would make 2.0.0b2 less than 2.0.0. We should
+    # TODO: probably revisit this in our versioning scheme.
+    ("2.0.0", "foo-2.0.0b2.tar.gz", "000000000000000000000000000200b2"),
+])
+def test_new_version_from_list_url(
+        mock_packages, config, _fetch_method, requested_version, tarball, digest
+):
+    if spack.config.get('config:concretizer') == 'original':
+        pytest.skip(
+            "Original concretizer doesn't resolve concrete versions to known ones"
+        )
+
     """Test non-specific URLs from the url-list-test package."""
-    with spack.config.override('config:url_fetch_method', _fetch_method):
-        pkg = spack.repo.get('url-list-test')
+    with spack.config.override("config:url_fetch_method", _fetch_method):
+        pkg = spack.repo.get("url-list-test")
 
-        spec = Spec('url-list-test @2.0.0').concretized()
+        spec = Spec("url-list-test @%s" % requested_version).concretized()
         pkg = spack.repo.get(spec)
         fetch_strategy = fs.from_list_url(pkg)
+
         assert isinstance(fetch_strategy, fs.URLFetchStrategy)
-        assert os.path.basename(fetch_strategy.url) == 'foo-2.0.0.tar.gz'
-        assert fetch_strategy.digest is None
+        assert os.path.basename(fetch_strategy.url) == tarball
+        assert fetch_strategy.digest == digest
         assert fetch_strategy.extra_options == {}
-        pkg.fetch_options = {'timeout': 60}
+        pkg.fetch_options = {"timeout": 60}
         fetch_strategy = fs.from_list_url(pkg)
-        assert fetch_strategy.extra_options == {'timeout': 60}
+        assert fetch_strategy.extra_options == {"timeout": 60}
 
 
 def test_nosource_from_list_url(mock_packages, config):