From 7bc5b26c520e19e75c3edf2ed3aaaaa135038734 Mon Sep 17 00:00:00 2001 From: Peter Scheibel Date: Tue, 16 May 2023 06:45:11 -0700 Subject: 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. --- lib/spack/spack/solver/asp.py | 57 +++++++++++-- lib/spack/spack/test/cmd/env.py | 2 +- lib/spack/spack/test/concretize_preferences.py | 27 +++++- lib/spack/spack/test/concretize_requirements.py | 97 +++++++++++++++++++++- .../cloud_pipelines/stacks/data-vis-sdk/spack.yaml | 4 +- .../repos/builtin.mock/packages/python/package.py | 1 + 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) -- cgit v1.2.3-60-g2f50