From 7d534f38d6606e4bbe2e52c64ec9a2c7d0d06863 Mon Sep 17 00:00:00 2001 From: psakievich Date: Tue, 12 Apr 2022 11:37:24 -0600 Subject: [PATCH] Don't allow replacement of root develop specs with --reuse (#28605) * Fix to concretize.lp do not allow dev specs to be reused Co-authored-by: Gregory Becker --- lib/spack/spack/solver/asp.py | 6 ++-- lib/spack/spack/solver/concretize.lp | 3 ++ lib/spack/spack/test/concretize.py | 43 ++++++++++++++++++++++++++-- 3 files changed, 48 insertions(+), 4 deletions(-) diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py index 6a79f37acd..cb2b697282 100644 --- a/lib/spack/spack/solver/asp.py +++ b/lib/spack/spack/solver/asp.py @@ -1676,13 +1676,15 @@ def define_installed_packages(self, specs, possible): # Specs from local store with spack.store.db.read_transaction(): for spec in spack.store.db.query(installed=True): - self._facts_from_concrete_spec(spec, possible) + if not spec.satisfies('dev_path=*'): + self._facts_from_concrete_spec(spec, possible) # Specs from configured buildcaches try: index = spack.binary_distribution.update_cache_and_get_specs() for spec in index: - self._facts_from_concrete_spec(spec, possible) + if not spec.satisfies('dev_path=*'): + self._facts_from_concrete_spec(spec, possible) except (spack.binary_distribution.FetchCacheError, IndexError): # this is raised when no mirrors had indices. # TODO: update mirror configuration so it can indicate that the source cache diff --git a/lib/spack/spack/solver/concretize.lp b/lib/spack/spack/solver/concretize.lp index 36426bd836..3206ed60a4 100644 --- a/lib/spack/spack/solver/concretize.lp +++ b/lib/spack/spack/solver/concretize.lp @@ -853,6 +853,9 @@ no_flags(Package, FlagType) { hash(Package, Hash) : installed_hash(Package, Hash) } 1 :- node(Package), error("Internal error: package must resolve to at most one hash"). +% you can't choose an installed hash for a dev spec +:- hash(Package, Hash), variant_value(Package, "dev_path", _). + % if a hash is selected, we impose all the constraints that implies impose(Hash) :- hash(Package, Hash). diff --git a/lib/spack/spack/test/concretize.py b/lib/spack/spack/test/concretize.py index 4a348828eb..adb8d05160 100644 --- a/lib/spack/spack/test/concretize.py +++ b/lib/spack/spack/test/concretize.py @@ -339,7 +339,6 @@ def concretize_difficult_packages(self, a, b): assert s[a].version == ver(b) def test_concretize_two_virtuals(self): - """Test a package with multiple virtual dependencies.""" Spec('hypre').concretize() @@ -1165,6 +1164,46 @@ def test_external_package_versions(self, spec_str, is_external, expected): assert s.external == is_external assert s.satisfies(expected) + @pytest.mark.parametrize('dev_first', [True, False]) + @pytest.mark.parametrize('spec', [ + 'dev-build-test-install', 'dev-build-test-dependent ^dev-build-test-install']) + @pytest.mark.parametrize('mock_db', [True, False]) + def test_reuse_does_not_overwrite_dev_specs( + self, dev_first, spec, mock_db, tmpdir, monkeypatch): + """Test that reuse does not mix dev specs with non-dev specs. + + Tests for either order (dev specs are not reused for non-dev, and + non-dev specs are not reused for dev specs) + Tests for a spec in which the root is developed and a spec in + which a dep is developed. + Tests for both reuse from database and reuse from buildcache""" + # dev and non-dev specs that are otherwise identical + spec = Spec(spec) + dev_spec = spec.copy() + dev_constraint = 'dev_path=%s' % tmpdir.strpath + dev_spec['dev-build-test-install'].constrain(dev_constraint) + + # run the test in both orders + first_spec = dev_spec if dev_first else spec + second_spec = spec if dev_first else dev_spec + + # concretize and setup spack to reuse in the appropriate manner + first_spec.concretize() + + def mock_fn(*args, **kwargs): + return [first_spec] + + if mock_db: + monkeypatch.setattr(spack.store.db, 'query', mock_fn) + else: + monkeypatch.setattr( + spack.binary_distribution, 'update_cache_and_get_specs', mock_fn) + + # concretize and ensure we did not reuse + with spack.config.override("concretizer:reuse", True): + second_spec.concretize() + assert first_spec.dag_hash() != second_spec.dag_hash() + @pytest.mark.regression('20292') @pytest.mark.parametrize('context', [ {'add_variant': True, 'delete_variant': False}, @@ -1172,7 +1211,7 @@ def test_external_package_versions(self, spec_str, is_external, expected): {'add_variant': True, 'delete_variant': True} ]) @pytest.mark.xfail() - def test_reuse_installed_packages( + def test_reuse_installed_packages_when_package_def_changes( self, context, mutable_database, repo_with_changing_recipe ): # Install a spec