summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorHarmen Stoppels <me@harmenstoppels.nl>2023-12-20 20:21:15 +0100
committerGitHub <noreply@github.com>2023-12-20 19:21:15 +0000
commit1aaab97a162f2c3c6d1dca77e2ac3d75ab3e2930 (patch)
tree3f768e0274441fb58bdb05603bd4cb1c7c66ed4a
parent3053e701c02fd57b0a3aa169abd57b144edf827b (diff)
downloadspack-1aaab97a162f2c3c6d1dca77e2ac3d75ab3e2930.tar.gz
spack-1aaab97a162f2c3c6d1dca77e2ac3d75ab3e2930.tar.bz2
spack-1aaab97a162f2c3c6d1dca77e2ac3d75ab3e2930.tar.xz
spack-1aaab97a162f2c3c6d1dca77e2ac3d75ab3e2930.zip
Only reuse externals when configured (#41707)
Users expect that changes to the externals sections in packages.yaml config apply immediately, but reuse concretization caused this not to be the case. With this commit, the concretizer is only allowed to reuse externals previously imported from config if identical config exists.
-rw-r--r--lib/spack/spack/solver/asp.py54
-rw-r--r--lib/spack/spack/test/concretize.py36
-rw-r--r--lib/spack/spack/test/cray_manifest.py21
3 files changed, 78 insertions, 33 deletions
diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py
index 13fe2ef156..b00982b4fb 100644
--- a/lib/spack/spack/solver/asp.py
+++ b/lib/spack/spack/solver/asp.py
@@ -3134,14 +3134,38 @@ def _develop_specs_from_env(spec, env):
spec.constrain(dev_info["spec"])
-def _is_reusable_external(packages, spec: spack.spec.Spec) -> bool:
- """Returns true iff spec is an external that can be reused.
+def _is_reusable(spec: spack.spec.Spec, packages, local: bool) -> bool:
+ """A spec is reusable if it's not a dev spec, it's imported from the cray manifest, it's not
+ external, or it's external with matching packages.yaml entry. The latter prevents two issues:
+
+ 1. Externals in build caches: avoid installing an external on the build machine not
+ available on the target machine
+ 2. Local externals: avoid reusing an external if the local config changes. This helps in
+ particular when a user removes an external from packages.yaml, and expects that that
+ takes effect immediately.
Arguments:
- packages: the packages configuration
spec: the spec to check
+ packages: the packages configuration
"""
- for name in {spec.name, *(p.name for p in spec.package.provided)}:
+ if "dev_path" in spec.variants:
+ return False
+
+ if not spec.external:
+ return True
+
+ # Cray external manifest externals are always reusable
+ if local:
+ _, record = spack.store.STORE.db.query_by_spec_hash(spec.dag_hash())
+ if record and record.origin == "external-db":
+ return True
+
+ try:
+ provided = [p.name for p in spec.package.provided]
+ except spack.repo.RepoError:
+ provided = []
+
+ for name in {spec.name, *provided}:
for entry in packages.get(name, {}).get("externals", []):
if (
spec.satisfies(entry["spec"])
@@ -3188,29 +3212,21 @@ class Solver:
def _reusable_specs(self, specs):
reusable_specs = []
if self.reuse:
+ packages = spack.config.get("packages")
# Specs from the local Database
with spack.store.STORE.db.read_transaction():
reusable_specs.extend(
- [
- s
- for s in spack.store.STORE.db.query(installed=True)
- if not s.satisfies("dev_path=*")
- ]
+ s
+ for s in spack.store.STORE.db.query(installed=True)
+ if _is_reusable(s, packages, local=True)
)
# Specs from buildcaches
try:
- # Specs in a build cache that depend on externals are reusable as long as local
- # config has matching externals. This should guard against picking up binaries
- # linked against externals not available locally, while still supporting the use
- # case of distributing binaries across machines with similar externals.
- packages = spack.config.get("packages")
reusable_specs.extend(
- [
- s
- for s in spack.binary_distribution.update_cache_and_get_specs()
- if not s.external or _is_reusable_external(packages, s)
- ]
+ s
+ for s in spack.binary_distribution.update_cache_and_get_specs()
+ if _is_reusable(s, packages, local=False)
)
except (spack.binary_distribution.FetchCacheError, IndexError):
# this is raised when no mirrors had indices.
diff --git a/lib/spack/spack/test/concretize.py b/lib/spack/spack/test/concretize.py
index 2eb75edb6c..fd5bd7d9c5 100644
--- a/lib/spack/spack/test/concretize.py
+++ b/lib/spack/spack/test/concretize.py
@@ -1817,12 +1817,14 @@ class TestConcretize:
@pytest.mark.regression("31484")
@pytest.mark.only_clingo("Use case not supported by the original concretizer")
- def test_installed_externals_are_reused(self, mutable_database, repo_with_changing_recipe):
+ def test_installed_externals_are_reused(
+ self, mutable_database, repo_with_changing_recipe, tmp_path
+ ):
"""Test that external specs that are in the DB can be reused."""
external_conf = {
"changing": {
"buildable": False,
- "externals": [{"spec": "changing@1.0", "prefix": "/usr"}],
+ "externals": [{"spec": "changing@1.0", "prefix": str(tmp_path)}],
}
}
spack.config.set("packages", external_conf)
@@ -1847,12 +1849,12 @@ class TestConcretize:
@pytest.mark.regression("31484")
@pytest.mark.only_clingo("Use case not supported by the original concretizer")
- def test_user_can_select_externals_with_require(self, mutable_database):
+ def test_user_can_select_externals_with_require(self, mutable_database, tmp_path):
"""Test that users have means to select an external even in presence of reusable specs."""
external_conf = {
"mpi": {"buildable": False},
"multi-provider-mpi": {
- "externals": [{"spec": "multi-provider-mpi@2.0.0", "prefix": "/usr"}]
+ "externals": [{"spec": "multi-provider-mpi@2.0.0", "prefix": str(tmp_path)}]
},
}
spack.config.set("packages", external_conf)
@@ -2434,7 +2436,8 @@ def test_reusable_externals_match(mock_packages, tmpdir):
spec.external_path = tmpdir.strpath
spec.external_modules = ["mpich/4.1"]
spec._mark_concrete()
- assert spack.solver.asp._is_reusable_external(
+ assert spack.solver.asp._is_reusable(
+ spec,
{
"mpich": {
"externals": [
@@ -2442,7 +2445,7 @@ def test_reusable_externals_match(mock_packages, tmpdir):
]
}
},
- spec,
+ local=False,
)
@@ -2451,7 +2454,8 @@ def test_reusable_externals_match_virtual(mock_packages, tmpdir):
spec.external_path = tmpdir.strpath
spec.external_modules = ["mpich/4.1"]
spec._mark_concrete()
- assert spack.solver.asp._is_reusable_external(
+ assert spack.solver.asp._is_reusable(
+ spec,
{
"mpi": {
"externals": [
@@ -2459,7 +2463,7 @@ def test_reusable_externals_match_virtual(mock_packages, tmpdir):
]
}
},
- spec,
+ local=False,
)
@@ -2468,7 +2472,8 @@ def test_reusable_externals_different_prefix(mock_packages, tmpdir):
spec.external_path = "/other/path"
spec.external_modules = ["mpich/4.1"]
spec._mark_concrete()
- assert not spack.solver.asp._is_reusable_external(
+ assert not spack.solver.asp._is_reusable(
+ spec,
{
"mpich": {
"externals": [
@@ -2476,7 +2481,7 @@ def test_reusable_externals_different_prefix(mock_packages, tmpdir):
]
}
},
- spec,
+ local=False,
)
@@ -2486,7 +2491,8 @@ def test_reusable_externals_different_modules(mock_packages, tmpdir, modules):
spec.external_path = tmpdir.strpath
spec.external_modules = modules
spec._mark_concrete()
- assert not spack.solver.asp._is_reusable_external(
+ assert not spack.solver.asp._is_reusable(
+ spec,
{
"mpich": {
"externals": [
@@ -2494,7 +2500,7 @@ def test_reusable_externals_different_modules(mock_packages, tmpdir, modules):
]
}
},
- spec,
+ local=False,
)
@@ -2502,6 +2508,8 @@ def test_reusable_externals_different_spec(mock_packages, tmpdir):
spec = Spec("mpich@4.1%gcc@13.1.0~debug build_system=generic arch=linux-ubuntu23.04-zen2")
spec.external_path = tmpdir.strpath
spec._mark_concrete()
- assert not spack.solver.asp._is_reusable_external(
- {"mpich": {"externals": [{"spec": "mpich@4.1 +debug", "prefix": tmpdir.strpath}]}}, spec
+ assert not spack.solver.asp._is_reusable(
+ spec,
+ {"mpich": {"externals": [{"spec": "mpich@4.1 +debug", "prefix": tmpdir.strpath}]}},
+ local=False,
)
diff --git a/lib/spack/spack/test/cray_manifest.py b/lib/spack/spack/test/cray_manifest.py
index 123e2ac3f1..dd19919ebc 100644
--- a/lib/spack/spack/test/cray_manifest.py
+++ b/lib/spack/spack/test/cray_manifest.py
@@ -19,6 +19,7 @@ import spack.cmd
import spack.compilers
import spack.config
import spack.cray_manifest as cray_manifest
+import spack.solver.asp
import spack.spec
import spack.store
from spack.cray_manifest import compiler_from_entry, entries_to_specs
@@ -488,3 +489,23 @@ def test_find_external_nonempty_default_manifest_dir(
spack.cmd.external._collect_and_consume_cray_manifest_files(ignore_default_dir=False)
specs = spack.store.STORE.db.query("hwloc")
assert any(x.dag_hash() == "hwlocfakehashaaa" for x in specs)
+
+
+def test_reusable_externals_cray_manifest(
+ tmpdir, mutable_config, mock_packages, temporary_store, manifest_content
+):
+ """The concretizer should be able to reuse specs imported from a manifest without a
+ externals config entry in packages.yaml"""
+ with tmpdir.as_cwd():
+ with open("external-db.json", "w") as f:
+ json.dump(manifest_content, f)
+ cray_manifest.read(path="external-db.json", apply_updates=True)
+
+ # Get any imported spec
+ spec = temporary_store.db.query_local()[0]
+
+ # Reusable if imported locally
+ assert spack.solver.asp._is_reusable(spec, packages={}, local=True)
+
+ # If cray manifest entries end up in a build cache somehow, they are not reusable
+ assert not spack.solver.asp._is_reusable(spec, packages={}, local=False)