From 7912a8e90b296ad648f2d86651931746fc1826f6 Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Mon, 21 Feb 2022 11:46:37 -0800 Subject: 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 --- lib/spack/spack/solver/asp.py | 32 +++++++++++++++++++----------- lib/spack/spack/test/cmd/install.py | 9 +++++---- lib/spack/spack/test/concretize.py | 13 ++++++++++++ lib/spack/spack/test/data/web/4.html | 1 + lib/spack/spack/test/url_fetch.py | 38 +++++++++++++++++++++++++++--------- 5 files changed, 69 insertions(+), 24 deletions(-) (limited to 'lib') 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 @@ foo-4.5.tar.gz. foo-4.1-rc5.tar.gz. + foo-4.5.0.tar.gz. 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): -- cgit v1.2.3-60-g2f50