From 0d0ff44e3e0e61b20a8e53bba75df5c7d2fd11ea Mon Sep 17 00:00:00 2001
From: Todd Gamblin <tgamblin@llnl.gov>
Date: Sat, 14 Dec 2024 06:24:22 -0800
Subject: [PATCH] Spec: Remove `_normal` attribute and unused constructor
arguments (#48119)
The `_normal` attribute on specs is no longer used and has no meaning.
It's left over from part of the original concretizer.
The `concrete` constructor argument is also not used by any part of core.
- [x] remove `_normal` attribute from `Spec`
- [x] remove `concrete` argument from `Spec.__init__`
- [x] remove unused `check_diamond_normalized_dag` function in tests
- [x] simplify `Spec` constructor and docstrings
I tried to add typing to `Spec` here, but it creates a huge number of type issues
because *most* things on `Spec` are optional. We probably need separate `Spec` and
`ConcreteSpec` classes before attempting that.
Signed-off-by: Todd Gamblin <tgamblin@llnl.gov>
---
lib/spack/spack/spec.py | 51 ++++++++++----------------------
lib/spack/spack/test/spec_dag.py | 14 ---------
2 files changed, 16 insertions(+), 49 deletions(-)
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index a9e0b088770..14614969c72 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -1433,10 +1433,6 @@ def tree(
@lang.lazy_lexicographic_ordering(set_hash=False)
class Spec:
- #: Cache for spec's prefix, computed lazily in the corresponding property
- _prefix = None
- abstract_hash = None
-
@staticmethod
def default_arch():
"""Return an anonymous spec for the default architecture"""
@@ -1444,27 +1440,17 @@ def default_arch():
s.architecture = ArchSpec.default_arch()
return s
- def __init__(
- self,
- spec_like=None,
- normal=False,
- concrete=False,
- external_path=None,
- external_modules=None,
- ):
+ def __init__(self, spec_like=None, *, external_path=None, external_modules=None):
"""Create a new Spec.
Arguments:
- spec_like (optional string): if not provided, we initialize
- an anonymous Spec that matches any Spec object; if
- provided we parse this as a Spec string.
+ spec_like: if not provided, we initialize an anonymous Spec that matches any Spec;
+ if provided we parse this as a Spec string, or we copy the provided Spec.
Keyword arguments:
- # assign special fields from constructor
- self._normal = normal
- self._concrete = concrete
- self.external_path = external_path
- self.external_module = external_module
+ external_path: prefix, if this is a spec for an external package
+ external_modules: list of external modules, if this is an external package
+ using modules.
"""
# Copy if spec_like is a Spec.
if isinstance(spec_like, Spec):
@@ -1481,26 +1467,26 @@ def __init__(
self._dependents = _EdgeMap(store_by_child=False)
self._dependencies = _EdgeMap(store_by_child=True)
self.namespace = None
+ self.abstract_hash = None
# initial values for all spec hash types
for h in ht.hashes:
setattr(self, h.attr, None)
+ # cache for spec's prefix, computed lazily by prefix property
+ self._prefix = None
+
# Python __hash__ is handled separately from the cached spec hashes
self._dunder_hash = None
# cache of package for this spec
self._package = None
- # Most of these are internal implementation details that can be
- # set by internal Spack calls in the constructor.
- #
- # For example, Specs are by default not assumed to be normal, but
- # in some cases we've read them from a file want to assume
- # normal. This allows us to manipulate specs that Spack doesn't
- # have package.py files for.
- self._normal = normal
- self._concrete = concrete
+ # whether the spec is concrete or not; set at the end of concretization
+ self._concrete = False
+
+ # External detection details that can be set by internal Spack calls
+ # in the constructor.
self._external_path = external_path
self.external_modules = Spec._format_module_list(external_modules)
@@ -2876,7 +2862,6 @@ def _mark_root_concrete(self, value=True):
"""Mark just this spec (not dependencies) concrete."""
if (not value) and self.concrete and self.installed:
return
- self._normal = value
self._concrete = value
self._validate_version()
@@ -3538,7 +3523,6 @@ def _dup(self, other, deps: Union[bool, dt.DepTypes, dt.DepFlag] = True, clearde
and self.architecture != other.architecture
and self.compiler != other.compiler
and self.variants != other.variants
- and self._normal != other._normal
and self.concrete != other.concrete
and self.external_path != other.external_path
and self.external_modules != other.external_modules
@@ -3584,20 +3568,17 @@ def _dup(self, other, deps: Union[bool, dt.DepTypes, dt.DepFlag] = True, clearde
depflag = dt.canonicalize(deps)
self._dup_deps(other, depflag)
+ self._prefix = other._prefix
self._concrete = other._concrete
self.abstract_hash = other.abstract_hash
if self._concrete:
self._dunder_hash = other._dunder_hash
- self._normal = other._normal
for h in ht.hashes:
setattr(self, h.attr, getattr(other, h.attr, None))
else:
self._dunder_hash = None
- # Note, we could use other._normal if we are copying all deps, but
- # always set it False here to avoid the complexity of checking
- self._normal = False
for h in ht.hashes:
setattr(self, h.attr, None)
diff --git a/lib/spack/spack/test/spec_dag.py b/lib/spack/spack/test/spec_dag.py
index 05cc15f3dab..d127f307458 100644
--- a/lib/spack/spack/test/spec_dag.py
+++ b/lib/spack/spack/test/spec_dag.py
@@ -391,7 +391,6 @@ def test_copy_simple(self):
assert orig == copy
assert orig.eq_dag(copy)
- assert orig._normal == copy._normal
assert orig._concrete == copy._concrete
# ensure no shared nodes bt/w orig and copy.
@@ -408,7 +407,6 @@ def test_copy_concretized(self):
assert orig == copy
assert orig.eq_dag(copy)
- assert orig._normal == copy._normal
assert orig._concrete == copy._concrete
# ensure no shared nodes bt/w orig and copy.
@@ -621,18 +619,6 @@ def check_diamond_deptypes(self, spec):
== dt.BUILD | dt.LINK | dt.RUN
)
- def check_diamond_normalized_dag(self, spec):
- dag = Spec.from_literal(
- {
- "dt-diamond": {
- "dt-diamond-left:build,link": {"dt-diamond-bottom:build": None},
- "dt-diamond-right:build,link": {"dt-diamond-bottom:build,link,run": None},
- }
- }
- )
-
- assert spec.eq_dag(dag)
-
def test_concretize_deptypes(self):
"""Ensure that dependency types are preserved after concretization."""
s = Spec("dt-diamond")
--
GitLab