From 6b676b84c13662ba6eed1704834d72d653842a6b Mon Sep 17 00:00:00 2001 From: Massimiliano Culpo Date: Wed, 4 Jan 2023 13:42:35 +0100 Subject: [PATCH 1/2] DependencySpec: add virtuals attribute on edges This works for both the new and the old concretizer. Also, added type hints to involved functions. --- lib/spack/spack/build_systems/python.py | 2 +- lib/spack/spack/cray_manifest.py | 3 +- lib/spack/spack/database.py | 17 +- lib/spack/spack/environment/environment.py | 9 +- lib/spack/spack/graph.py | 1 + lib/spack/spack/installer.py | 4 +- lib/spack/spack/parser.py | 2 +- lib/spack/spack/provider_index.py | 4 +- lib/spack/spack/solver/asp.py | 11 +- lib/spack/spack/solver/concretize.lp | 5 + lib/spack/spack/spec.py | 196 +++++++++++++----- lib/spack/spack/test/concretize.py | 8 + .../test/data/specfiles/hdf5.v020.json.gz | Bin 0 -> 5205 bytes lib/spack/spack/test/spec_dag.py | 30 +-- lib/spack/spack/test/spec_semantics.py | 6 +- lib/spack/spack/test/spec_yaml.py | 68 +++--- lib/spack/spack/test/traverse.py | 2 +- lib/spack/spack/traverse.py | 4 +- 18 files changed, 244 insertions(+), 128 deletions(-) create mode 100644 lib/spack/spack/test/data/specfiles/hdf5.v020.json.gz diff --git a/lib/spack/spack/build_systems/python.py b/lib/spack/spack/build_systems/python.py index 4641f851724788..e6ebf989049019 100644 --- a/lib/spack/spack/build_systems/python.py +++ b/lib/spack/spack/build_systems/python.py @@ -268,7 +268,7 @@ def update_external_dependencies(self, extendee_spec=None): python.external_path = self.spec.external_path python._mark_concrete() - self.spec.add_dependency_edge(python, deptypes=("build", "link", "run")) + self.spec.add_dependency_edge(python, deptypes=("build", "link", "run"), virtuals=()) def get_external_python_for_prefix(self): """ diff --git a/lib/spack/spack/cray_manifest.py b/lib/spack/spack/cray_manifest.py index 6f686aaf577030..24c19de3692e22 100644 --- a/lib/spack/spack/cray_manifest.py +++ b/lib/spack/spack/cray_manifest.py @@ -159,7 +159,8 @@ def entries_to_specs(entries): continue parent_spec = spec_dict[entry["hash"]] dep_spec = spec_dict[dep_hash] - parent_spec._add_dependency(dep_spec, deptypes=deptypes) + # TODO: Check this with Peter + parent_spec._add_dependency(dep_spec, deptypes=deptypes, virtuals=()) return spec_dict diff --git a/lib/spack/spack/database.py b/lib/spack/spack/database.py index af321323c911b3..a0d5400278e9c1 100644 --- a/lib/spack/spack/database.py +++ b/lib/spack/spack/database.py @@ -60,7 +60,7 @@ # DB version. This is stuck in the DB file to track changes in format. # Increment by one when the database format changes. # Versions before 5 were not integers. -_db_version = Version("6") +_db_version = Version("7") # For any version combinations here, skip reindex when upgrading. # Reindexing can take considerable time and is not always necessary. @@ -72,6 +72,7 @@ # version is saved to disk the first time the DB is written. (Version("0.9.3"), Version("5")), (Version("5"), Version("6")), + (Version("6"), Version("7")), ] # Default timeout for spack database locks in seconds or None (no timeout). @@ -105,7 +106,11 @@ def reader(version): - reader_cls = {Version("5"): spack.spec.SpecfileV1, Version("6"): spack.spec.SpecfileV3} + reader_cls = { + Version("5"): spack.spec.SpecfileV1, + Version("6"): spack.spec.SpecfileV3, + Version("7"): spack.spec.SpecfileV4, + } return reader_cls[version] @@ -744,7 +749,9 @@ def _assign_dependencies(self, spec_reader, hash_key, installs, data): spec_node_dict = spec_node_dict[spec.name] if "dependencies" in spec_node_dict: yaml_deps = spec_node_dict["dependencies"] - for dname, dhash, dtypes, _ in spec_reader.read_specfile_dep_specs(yaml_deps): + for dname, dhash, dtypes, _, virtuals in spec_reader.read_specfile_dep_specs( + yaml_deps + ): # It is important that we always check upstream installations # in the same order, and that we always check the local # installation first: if a downstream Spack installs a package @@ -767,7 +774,7 @@ def _assign_dependencies(self, spec_reader, hash_key, installs, data): tty.warn(msg) continue - spec._add_dependency(child, deptypes=dtypes) + spec._add_dependency(child, deptypes=dtypes, virtuals=virtuals) def _read_from_file(self, filename): """Fill database from file, do not maintain old data. @@ -1170,7 +1177,7 @@ def _add( for dep in spec.edges_to_dependencies(deptype=_tracked_deps): dkey = dep.spec.dag_hash() upstream, record = self.query_by_spec_hash(dkey) - new_spec._add_dependency(record.spec, deptypes=dep.deptypes) + new_spec._add_dependency(record.spec, deptypes=dep.deptypes, virtuals=dep.virtuals) if not upstream: record.ref_count += 1 diff --git a/lib/spack/spack/environment/environment.py b/lib/spack/spack/environment/environment.py index 27b83809c3d316..ff606031feb715 100644 --- a/lib/spack/spack/environment/environment.py +++ b/lib/spack/spack/environment/environment.py @@ -121,7 +121,7 @@ def default_manifest_yaml(): valid_environment_name_re = r"^\w[\w-]*$" #: version of the lockfile format. Must increase monotonically. -lockfile_format_version = 4 +lockfile_format_version = 5 READER_CLS = { @@ -129,6 +129,7 @@ def default_manifest_yaml(): 2: spack.spec.SpecfileV1, 3: spack.spec.SpecfileV2, 4: spack.spec.SpecfileV3, + 5: spack.spec.SpecfileV4, } @@ -1479,7 +1480,7 @@ def _concretize_separately(self, tests=False): if test_dependency in current_spec[node.name]: continue current_spec[node.name].add_dependency_edge( - test_dependency.copy(), deptypes="test" + test_dependency.copy(), deptypes="test", virtuals=() ) results = [ @@ -2052,9 +2053,9 @@ def _read_lockfile_dict(self, d): # and add them to the spec for lockfile_key, node_dict in json_specs_by_hash.items(): name, data = reader.name_and_data(node_dict) - for _, dep_hash, deptypes, _ in reader.dependencies_from_node_dict(data): + for _, dep_hash, deptypes, _, virtuals in reader.dependencies_from_node_dict(data): specs_by_hash[lockfile_key]._add_dependency( - specs_by_hash[dep_hash], deptypes=deptypes + specs_by_hash[dep_hash], deptypes=deptypes, virtuals=virtuals ) # Traverse the root specs one at a time in the order they appear. diff --git a/lib/spack/spack/graph.py b/lib/spack/spack/graph.py index e52cc64fdc50bb..73fffaeaa67c41 100644 --- a/lib/spack/spack/graph.py +++ b/lib/spack/spack/graph.py @@ -544,6 +544,7 @@ def _static_edges(specs, deptype): spack.spec.Spec(parent_name), spack.spec.Spec(dependency_name), deptypes=deptype, + virtuals=(), ) diff --git a/lib/spack/spack/installer.py b/lib/spack/spack/installer.py index 39ad78aa18d80a..3b6c994b7cbfcc 100644 --- a/lib/spack/spack/installer.py +++ b/lib/spack/spack/installer.py @@ -230,7 +230,9 @@ def _packages_needed_to_bootstrap_compiler(compiler, architecture, pkgs): dep.concretize() # mark compiler as depended-on by the packages that use it for pkg in pkgs: - dep._dependents.add(spack.spec.DependencySpec(pkg.spec, dep, deptypes=("build",))) + dep._dependents.add( + spack.spec.DependencySpec(pkg.spec, dep, deptypes=("build",), virtuals=()) + ) packages = [(s.package, False) for s in dep.traverse(order="post", root=False)] packages.append((dep.package, True)) diff --git a/lib/spack/spack/parser.py b/lib/spack/spack/parser.py index b4748b259f774c..56da75c7c3e1b3 100644 --- a/lib/spack/spack/parser.py +++ b/lib/spack/spack/parser.py @@ -283,7 +283,7 @@ def next_spec(self, initial_spec: Optional[spack.spec.Spec] = None) -> spack.spe if root_spec.concrete: raise spack.spec.RedundantSpecError(root_spec, "^" + str(dependency)) - root_spec._add_dependency(dependency, deptypes=()) + root_spec._add_dependency(dependency, deptypes=(), virtuals=()) else: break diff --git a/lib/spack/spack/provider_index.py b/lib/spack/spack/provider_index.py index 6de661a02aed7e..33a13eeeeeb22b 100644 --- a/lib/spack/spack/provider_index.py +++ b/lib/spack/spack/provider_index.py @@ -292,8 +292,8 @@ def from_json(stream, repository): index.providers = _transform( providers, lambda vpkg, plist: ( - spack.spec.SpecfileV3.from_node_dict(vpkg), - set(spack.spec.SpecfileV3.from_node_dict(p) for p in plist), + spack.spec.SpecfileV4.from_node_dict(vpkg), + set(spack.spec.SpecfileV4.from_node_dict(p) for p in plist), ), ) return index diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py index abe325cb8610c8..09b6aa0de710e6 100644 --- a/lib/spack/spack/solver/asp.py +++ b/lib/spack/spack/solver/asp.py @@ -2330,10 +2330,15 @@ def depends_on(self, pkg, dep, type): assert len(dependencies) < 2, msg if not dependencies: - self._specs[pkg].add_dependency_edge(self._specs[dep], deptypes=(type,)) + self._specs[pkg].add_dependency_edge(self._specs[dep], deptypes=(type,), virtuals=()) else: # TODO: This assumes that each solve unifies dependencies - dependencies[0].add_type(type) + dependencies[0].update_deptypes(deptypes=(type,)) + + def virtual_on_edge(self, pkg, provider, virtual): + dependencies = self._specs[pkg].edges_to_dependencies(name=provider) + assert len(dependencies) == 1 + dependencies[0].update_virtuals((virtual,)) def reorder_flags(self): """Order compiler flags on specs in predefined order. @@ -2406,6 +2411,8 @@ def sort_fn(function_tuple): return (-2, 0) elif name == "external_spec_selected": return (0, 0) # note out of order so this goes last + elif name == "virtual_on_edge": + return (1, 0) else: return (-1, 0) diff --git a/lib/spack/spack/solver/concretize.lp b/lib/spack/spack/solver/concretize.lp index 60d48b9faed681..c9d5c8cfe4961b 100644 --- a/lib/spack/spack/solver/concretize.lp +++ b/lib/spack/spack/solver/concretize.lp @@ -292,6 +292,11 @@ attr("depends_on", Package, Provider, Type) provider(Provider, Virtual), not external(Package). +attr("virtual_on_edge", Package, Provider, Virtual) + :- dependency_holds(Package, Virtual, Type), + provider(Provider, Virtual), + not external(Package). + % dependencies on virtuals also imply that the virtual is a virtual node attr("virtual_node", Virtual) :- dependency_holds(Package, Virtual, Type), diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py index ced35d5c7d0609..925319fb79dddd 100644 --- a/lib/spack/spack/spec.py +++ b/lib/spack/spack/spec.py @@ -166,7 +166,7 @@ ) #: specfile format version. Must increase monotonically -SPECFILE_FORMAT_VERSION = 3 +SPECFILE_FORMAT_VERSION = 4 def colorize_spec(spec): @@ -703,47 +703,81 @@ class DependencySpec: parent: starting node of the edge spec: ending node of the edge. deptypes: list of strings, representing dependency relationships. + virtuals: virtual packages provided from child to parent node. """ - __slots__ = "parent", "spec", "deptypes" + __slots__ = "parent", "spec", "parameters" - def __init__(self, parent: "Spec", spec: "Spec", *, deptypes: dp.DependencyArgument): + def __init__( + self, + parent: "Spec", + spec: "Spec", + *, + deptypes: dp.DependencyArgument, + virtuals: Tuple[str, ...], + ): self.parent = parent self.spec = spec - self.deptypes = dp.canonical_deptype(deptypes) + self.parameters = { + "deptypes": dp.canonical_deptype(deptypes), + "virtuals": tuple(sorted(set(virtuals))), + } - def update_deptypes(self, deptypes: dp.DependencyArgument) -> bool: - deptypes = set(deptypes) - deptypes.update(self.deptypes) - deptypes = tuple(sorted(deptypes)) - changed = self.deptypes != deptypes + @property + def deptypes(self) -> Tuple[str, ...]: + return self.parameters["deptypes"] - self.deptypes = deptypes - return changed + @property + def virtuals(self) -> Tuple[str, ...]: + return self.parameters["virtuals"] + + def _update_edge_multivalued_property( + self, property_name: str, value: Tuple[str, ...] + ) -> bool: + current = self.parameters[property_name] + update = set(current) | set(value) + update = tuple(sorted(update)) + changed = current != update + + if not changed: + return False - def copy(self) -> "DependencySpec": - return DependencySpec(self.parent, self.spec, deptypes=self.deptypes) + self.parameters[property_name] = update + return True + + def update_deptypes(self, deptypes: Tuple[str, ...]) -> bool: + """Update the current dependency types""" + return self._update_edge_multivalued_property("deptypes", deptypes) - def add_type(self, type: dp.DependencyArgument): - self.deptypes = dp.canonical_deptype(self.deptypes + dp.canonical_deptype(type)) + def update_virtuals(self, virtuals: Tuple[str, ...]) -> bool: + """Update the list of provided virtuals""" + return self._update_edge_multivalued_property("virtuals", virtuals) + + def copy(self) -> "DependencySpec": + """Return a copy of this edge""" + return DependencySpec( + self.parent, self.spec, deptypes=self.deptypes, virtuals=self.virtuals + ) def _cmp_iter(self): yield self.parent.name if self.parent else None yield self.spec.name if self.spec else None yield self.deptypes + yield self.virtuals def __str__(self) -> str: - return "%s %s--> %s" % ( - self.parent.name if self.parent else None, - self.deptypes, - self.spec.name if self.spec else None, - ) + parent = self.parent.name if self.parent else None + child = self.spec.name if self.spec else None + return f"{parent} {self.deptypes}[virtuals={','.join(self.virtuals)}] --> {child}" - def canonical(self) -> Tuple[str, str, Tuple[str, ...]]: - return self.parent.dag_hash(), self.spec.dag_hash(), self.deptypes + def canonical(self) -> Tuple[str, str, Tuple[str, ...], Tuple[str, ...]]: + return self.parent.dag_hash(), self.spec.dag_hash(), self.deptypes, self.virtuals def flip(self) -> "DependencySpec": - return DependencySpec(parent=self.spec, spec=self.parent, deptypes=self.deptypes) + """Flip the dependency, and drop virtual information""" + return DependencySpec( + parent=self.spec, spec=self.parent, deptypes=self.deptypes, virtuals=() + ) class CompilerFlag(str): @@ -1563,10 +1597,12 @@ def _set_compiler(self, compiler): ) self.compiler = compiler - def _add_dependency(self, spec: "Spec", *, deptypes: dp.DependencyArgument): + def _add_dependency( + self, spec: "Spec", *, deptypes: dp.DependencyArgument, virtuals: Tuple[str, ...] + ): """Called by the parser to add another spec as a dependency.""" if spec.name not in self._dependencies: - self.add_dependency_edge(spec, deptypes=deptypes) + self.add_dependency_edge(spec, deptypes=deptypes, virtuals=virtuals) return # Keep the intersection of constraints when a dependency is added @@ -1584,12 +1620,19 @@ def _add_dependency(self, spec: "Spec", *, deptypes: dp.DependencyArgument): "Cannot depend on incompatible specs '%s' and '%s'" % (dspec.spec, spec) ) - def add_dependency_edge(self, dependency_spec: "Spec", *, deptypes: dp.DependencyArgument): + def add_dependency_edge( + self, + dependency_spec: "Spec", + *, + deptypes: dp.DependencyArgument, + virtuals: Tuple[str, ...], + ): """Add a dependency edge to this spec. Args: dependency_spec: spec of the dependency deptypes: dependency types for this edge + virtuals: virtuals provided by this edge """ deptypes = dp.canonical_deptype(deptypes) @@ -1608,10 +1651,10 @@ def add_dependency_edge(self, dependency_spec: "Spec", *, deptypes: dp.Dependenc # If we are here, it means the edge object was previously added to # both the parent and the child. When we update this object they'll # both see the deptype modification. - edge.add_type(deptypes) + edge.update_deptypes(deptypes=deptypes) return - edge = DependencySpec(self, dependency_spec, deptypes=deptypes) + edge = DependencySpec(self, dependency_spec, deptypes=deptypes, virtuals=virtuals) self._dependencies.add(edge) dependency_spec._dependents.add(edge) @@ -1951,8 +1994,14 @@ def to_node_dict(self, hash=ht.dag_hash): name_tuple = ("name", name) for dspec in edges_for_name: hash_tuple = (hash.name, dspec.spec._cached_hash(hash)) - type_tuple = ("type", sorted(str(s) for s in dspec.deptypes)) - deps_list.append(syaml.syaml_dict([name_tuple, hash_tuple, type_tuple])) + parameters_tuple = ( + "parameters", + syaml.syaml_dict( + (key, dspec.parameters[key]) for key in sorted(dspec.parameters) + ), + ) + ordered_entries = [name_tuple, hash_tuple, parameters_tuple] + deps_list.append(syaml.syaml_dict(ordered_entries)) d["dependencies"] = deps_list # Name is included in case this is replacing a virtual. @@ -2276,7 +2325,7 @@ def spec_and_dependency_types(s): dag_node, dependency_types = spec_and_dependency_types(s) dependency_spec = spec_builder({dag_node: s_dependencies}) - spec._add_dependency(dependency_spec, deptypes=dependency_types) + spec._add_dependency(dependency_spec, deptypes=dependency_types, virtuals=()) return spec @@ -2296,7 +2345,9 @@ def from_dict(data): specfile_version = int(data["spec"]["_meta"]["version"]) if specfile_version == 2: return SpecfileV2.load(data) - return SpecfileV3.load(data) + elif specfile_version == 3: + return SpecfileV3.load(data) + return SpecfileV4.load(data) @staticmethod def from_yaml(stream): @@ -2442,6 +2493,7 @@ def _concretize_helper(self, concretizer, presets=None, visited=None): def _replace_with(self, concrete): """Replace this virtual spec with a concrete spec.""" assert self.virtual + virtuals = (self.name,) for dep_spec in itertools.chain.from_iterable(self._dependents.values()): dependent = dep_spec.parent deptypes = dep_spec.deptypes @@ -2452,7 +2504,11 @@ def _replace_with(self, concrete): # add the replacement, unless it is already a dep of dependent. if concrete.name not in dependent._dependencies: - dependent._add_dependency(concrete, deptypes=deptypes) + dependent._add_dependency(concrete, deptypes=deptypes, virtuals=virtuals) + else: + dependent.edges_to_dependencies(name=concrete.name)[0].update_virtuals( + virtuals=virtuals + ) def _expand_virtual_packages(self, concretizer): """Find virtual packages in this spec, replace them with providers, @@ -3066,7 +3122,9 @@ def _merge_dependency(self, dependency, visited, spec_deps, provider_index, test # If it's a virtual dependency, try to find an existing # provider in the spec, and merge that. + virtuals = () if spack.repo.path.is_virtual_safe(dep.name): + virtuals = (dep.name,) visited.add(dep.name) provider = self._find_provider(dep, provider_index) if provider: @@ -3122,7 +3180,7 @@ def _merge_dependency(self, dependency, visited, spec_deps, provider_index, test # Add merged spec to my deps and recurse spec_dependency = spec_deps[dep.name] if dep.name not in self._dependencies: - self._add_dependency(spec_dependency, deptypes=dependency.type) + self._add_dependency(spec_dependency, deptypes=dependency.type, virtuals=virtuals) changed |= spec_dependency._normalize_helper(visited, spec_deps, provider_index, tests) return changed @@ -3454,15 +3512,20 @@ def _constrain_dependencies(self, other): changed |= edges_from_name[0].update_deptypes( other._dependencies[name][0].deptypes ) + changed |= edges_from_name[0].update_virtuals( + other._dependencies[name][0].virtuals + ) # Update with additional constraints from other spec # operate on direct dependencies only, because a concrete dep # represented by hash may have structure that needs to be preserved for name in other.direct_dep_difference(self): dep_spec_copy = other._get_dependency(name) - dep_copy = dep_spec_copy.spec - deptypes = dep_spec_copy.deptypes - self._add_dependency(dep_copy.copy(), deptypes=deptypes) + self._add_dependency( + dep_spec_copy.spec.copy(), + deptypes=dep_spec_copy.deptypes, + virtuals=dep_spec_copy.virtuals, + ) changed = True return changed @@ -3828,7 +3891,7 @@ def spid(spec): new_specs[spid(edge.spec)] = edge.spec.copy(deps=False) new_specs[spid(edge.parent)].add_dependency_edge( - new_specs[spid(edge.spec)], deptypes=edge.deptypes + new_specs[spid(edge.spec)], deptypes=edge.deptypes, virtuals=edge.virtuals ) def copy(self, deps=True, **kwargs): @@ -4743,12 +4806,16 @@ def from_self(name, transitive): if name in self_nodes: for edge in self[name].edges_to_dependencies(): dep_name = deps_to_replace.get(edge.spec, edge.spec).name - nodes[name].add_dependency_edge(nodes[dep_name], deptypes=edge.deptypes) + nodes[name].add_dependency_edge( + nodes[dep_name], deptypes=edge.deptypes, virtuals=edge.virtuals + ) if any(dep not in self_nodes for dep in self[name]._dependencies): nodes[name].build_spec = self[name].build_spec else: for edge in other[name].edges_to_dependencies(): - nodes[name].add_dependency_edge(nodes[edge.spec.name], deptypes=edge.deptypes) + nodes[name].add_dependency_edge( + nodes[edge.spec.name], deptypes=edge.deptypes, virtuals=edge.virtuals + ) if any(dep not in other_nodes for dep in other[name]._dependencies): nodes[name].build_spec = other[name].build_spec @@ -4821,11 +4888,25 @@ def merge_abstract_anonymous_specs(*abstract_specs: Spec): # Update with additional constraints from other spec for name in current_spec_constraint.direct_dep_difference(merged_spec): edge = next(iter(current_spec_constraint.edges_to_dependencies(name))) - merged_spec._add_dependency(edge.spec.copy(), deptypes=edge.deptypes) + merged_spec._add_dependency( + edge.spec.copy(), deptypes=edge.deptypes, virtuals=edge.virtuals + ) return merged_spec +def _reconstruct_virtuals_on_edges(spec): + """Reconstruct virtuals on edges. Used to read from old DB + and reindex. + """ + possible_virtuals = [x for x in spec.package.dependencies if Spec(x).virtual] + for vspec in possible_virtuals: + if vspec in spec: + name = spec[vspec].name + for edge in spec.edges_to_dependencies(name=name): + edge.update_virtuals([vspec]) + + class SpecfileReaderBase: @classmethod def from_node_dict(cls, node): @@ -4907,7 +4988,7 @@ def _load(cls, data): # Pass 0: Determine hash type for node in nodes: - for _, _, _, dhash_type in cls.dependencies_from_node_dict(node): + for _, _, _, dhash_type, _ in cls.dependencies_from_node_dict(node): any_deps = True if dhash_type: hash_type = dhash_type @@ -4938,8 +5019,10 @@ def _load(cls, data): # Pass 2: Finish construction of all DAG edges (including build specs) for node_hash, node in hash_dict.items(): node_spec = node["node_spec"] - for _, dhash, dtypes, _ in cls.dependencies_from_node_dict(node): - node_spec._add_dependency(hash_dict[dhash]["node_spec"], deptypes=dtypes) + for _, dhash, dtypes, _, virtuals in cls.dependencies_from_node_dict(node): + node_spec._add_dependency( + hash_dict[dhash]["node_spec"], deptypes=dtypes, virtuals=virtuals + ) if "build_spec" in node.keys(): _, bhash, _ = cls.build_spec_from_node_dict(node, hash_type=hash_type) node_spec._build_spec = hash_dict[bhash]["node_spec"] @@ -4973,9 +5056,10 @@ def load(cls, data): for node in nodes: # get dependency dict from the node. name, data = cls.name_and_data(node) - for dname, _, dtypes, _ in cls.dependencies_from_node_dict(data): - deps[name]._add_dependency(deps[dname], deptypes=dtypes) + for dname, _, dtypes, _, virtuals in cls.dependencies_from_node_dict(data): + deps[name]._add_dependency(deps[dname], deptypes=dtypes, virtuals=virtuals) + _reconstruct_virtuals_on_edges(result) return result @classmethod @@ -5004,18 +5088,20 @@ def read_specfile_dep_specs(cls, deps, hash_type=ht.dag_hash.name): if h.name in elt: dep_hash, deptypes = elt[h.name], elt["type"] hash_type = h.name + virtuals = [] break else: # We never determined a hash type... raise spack.error.SpecError("Couldn't parse dependency spec.") else: raise spack.error.SpecError("Couldn't parse dependency types in spec.") - yield dep_name, dep_hash, list(deptypes), hash_type + yield dep_name, dep_hash, list(deptypes), hash_type, list(virtuals) class SpecfileV2(SpecfileReaderBase): @classmethod def load(cls, data): result = cls._load(data) + _reconstruct_virtuals_on_edges(result) return result @classmethod @@ -5049,7 +5135,7 @@ def read_specfile_dep_specs(cls, deps, hash_type=ht.dag_hash.name): raise spack.error.SpecError("Couldn't parse dependency spec.") else: raise spack.error.SpecError("Couldn't parse dependency types in spec.") - result.append((dep_name, dep_hash, list(deptypes), hash_type)) + result.append((dep_name, dep_hash, list(deptypes), hash_type, list(virtuals))) return result @classmethod @@ -5069,6 +5155,20 @@ class SpecfileV3(SpecfileV2): pass +class SpecfileV4(SpecfileV2): + @classmethod + def extract_info_from_dep(cls, elt, hash): + dep_hash = elt[hash.name] + deptypes = elt["parameters"]["deptypes"] + hash_type = hash.name + virtuals = elt["parameters"]["virtuals"] + return dep_hash, deptypes, hash_type, virtuals + + @classmethod + def load(cls, data): + return cls._load(data) + + class LazySpecCache(collections.defaultdict): """Cache for Specs that uses a spec_like as key, and computes lazily the corresponding value ``Spec(spec_like``. diff --git a/lib/spack/spack/test/concretize.py b/lib/spack/spack/test/concretize.py index 9754b47f6a65c9..30679f80170778 100644 --- a/lib/spack/spack/test/concretize.py +++ b/lib/spack/spack/test/concretize.py @@ -2151,3 +2151,11 @@ def test_compiler_with_custom_non_numeric_version(self, mock_executable): spack.config.set("compilers", compiler_configuration) s = spack.spec.Spec("a %gcc@foo").concretized() assert s.compiler.version == ver("foo") + + def test_virtuals_are_annotated_on_edges(self, default_mock_concretization): + spec = default_mock_concretization("mpileaks ^mpich") + + edges = spec.edges_to_dependencies(name="mpich") + assert len(edges) == 1 and edges[0].virtuals == ("mpi",) + edges = spec.edges_to_dependencies(name="callpath") + assert len(edges) == 1 and edges[0].virtuals == () diff --git a/lib/spack/spack/test/data/specfiles/hdf5.v020.json.gz b/lib/spack/spack/test/data/specfiles/hdf5.v020.json.gz new file mode 100644 index 0000000000000000000000000000000000000000..3b2fa4d830498b079b3f92b57ba05fe8a1983662 GIT binary patch literal 5205 zcmV-b6sqeViwFP!000021MFQ%Z{#?Z{wociI}M4O6a&m{fI(m9GKWSY!CebQQq-ac9uuJh?Ho)$4Bye3zzuqrt^~D{Kr3jyZO?{o?cR;3|;ALOrzib z`HvgxgzRGG-@n~h+Q^u?DTKl|@zZ_DoAfpXx8VGe23}n1@rL%wh35R(ly;cUU%$Tm z;?8y63~aFd0Fv7Toxk=pXxU%>`zx)IOG9gUC1-^T=a>I2?ND0zFWXCV{-Px9hj3-{ zhKdU%8nfreINv?M-4mza3oCnS^ybCLnsQrUw6@%4z4P4mqP>5|ru#G(_vvjJb)PI% zL+^8Ea+}ouJ1Z$+@GWuyf1b@r0`{QG$%a@Y9M$gmq9n)3%k@pOxB zlbg%Xjb%Dd`K2HmO4FD2=1Mev>Epj#s-nI;7Z6g>!Cb_2E-SutOV`WG@a4@NPuCCq z;xASHk2L>V_Qiim_Ajg4h39#W9~3p6Jok|ceH8QNO<^{f(z{Krq9fg8n+sEY8058Q zLhPga{95`=*}e#pijFF|ed(O(?*D}=L-j?@=YHVc@ALD#%d1_^hzD6yvTN@5i!^yn zwfs`h?mY41$XM`70y90H$z&?6+p!^P1}$%D#iz+FM2EsCu(W;_vbRn9pYiW3KOudb zEp=P2bBPDcSXhg7 zQVVEVR?yxQLpPbQ{+tLR@y{D#qbY;ony$szjs0N7it0gUc&_WV)o!@ivHES8Yybh? z0(_euy6MnOe?Q&yP!nC4Ecv6lNYbllgyS1MVEGU_xzPk}vUm7>_oD4L#ota(Oq#K^ z(=`a3PgORs3Ztf46zyx@BerBZV-@rbr)xbfx@nlB&+Cb=&D|i#>UmAZRyr&T-|HpF zQo5p_C+6I+%=)L{>Y>KyI#XG<8~vh z%_t?e@IcE0E&qm=JiA!(k(hkUjHg5Iu4eJM*-LYtPu4qluIcP-IYR>b%Q)VHya)wOM40chT44`>|qs8f`bF)iTa@x!8)28X)9G{>toLnV94O~`( z+463|Xi~u`2iojaLUdk+owqd%unVT|L^W0Q#0$||}Zxey*_%kAo?zWoN+NO`bA>Q z04U(nFXK3%nP!%7G}q-caxsUBNM7iyXjn0V7X)&c47D_J`6PB`hps0iH1Jsvwg>}k zh~c)Y*t&#F!-BnPWSgy}2Rnxl^sowz(7s2A#i32DcYSZBwXR@YziwPz3)XCjD<=vx zEL7GL(47`puUjeVtkqbF*4tMl{;uZUvj*+Hm-ZuN>wD+X`9_CkI5fkDi=$n3-P}R_ z1HaqTo)>qc4v^%OD3s>;Da~+36(W>FmY?WJAt;Ds_>`YiMxo#mij*sUIw?jm%0L3y zMU)J{RB?_3=1L`4Vh$f5gVC+8x(2rx!xtFIqEu?=OS9YjLA5ECx~ENrnx%!4=S3HZj;>KSQ%?PGIPmkI zv4nT{v*UBOPwhaU1A&f3&L2gUYD9YFXj~doMGt5;!FoDOGx2Mm)zb>S9{_JAU$(3J0bes2S*TgLPhJNs%&GI*$t}v$vwbVdJiz>Ss>eC%$Do@ z`YSmw=D?T_hB04Z;j2k=aj{|x_@zB>hd|{-M<8Z65mrJ1OQ_ocS|LE7RcB--nO-e3 zD-A0{qzrUI?L%MYS!{VUEVi5D;Ml%-%<;pH_|aKxO6g}qd|~*^v}#H=Q&q^RFAJk7 zMKdgF2Qyx`CcdBRpv4R9i1Ug>oPE19OokC0YRh6(y)2DdaTqd4k@vN1qJ{FRapY$s z<~v~7vb*Wi{5a%!V9STYmR+EfsZ*v>$x%X2;3NfQn9@8wopPQ^AQgfbLBjDV%P=~n zL=uW~k}D8ru2R`wZgRXbs}Ir7qB5vu<^8fu%vW{k8`-W)%bIiNLj$-Z9A$VWOU9dW z$tZ1q&0{&A{|wSTy56_9s&3Q6t~#*q17P3zjI4@;ZY~=s`m!lkc^?M828o~8S@CvO zm=G5acQ5LR5{(U`ocM*qI+cISw z&%sjjshG^}aPosoFO1ZbFNO{_+BjibLmV)D*j{hZ$R?KI^+jr5U%C+K4AzGT2wUbu zl!Lnmv2a0$uLu{b5ress+ccpotsV>wWxs6K%&uXjZ~r~^_vtB6?`8NDqK#I(2V_=R zfkL2eZCfP5*iM5ZB|`Op1{BPy4W}FsL|aj#C62+Buoswa*FM{zYGLAl8Ppoln4u8^ z&QX=k1=M3`bfB-h>&y-f<^67le?8eb^Rouds%7y=5WLtCS~pY{x}p0E&T|)A`ZJMu z>QK#V)n)Bi(01)-OKHL9wh4y+$mJWq!l8zDIxV_mp(*E`sJ*3fGNb59*P^I}Hr-TV zF+=a>XlO{6zbxxyAv2+R747(J)wi#jpGRUe7pr=gVe(f%_ev zH3lDc*lyHpKqEP}CLG%9ShoCeoG&n?EeteRYZ}i+hzRl-oV~B@IuATl85BLpGT*p$ zsS!2f%W(eOmMz3cyZWpf$7$->!V$&=3_55)Eo!O^hmYcnocCycir!~%JA`keQX{IP zpulL(AXGD5s1uoO$Bgx4dX*ie&#KXfu==%=b`Al$%??OCAoYOMAB5D*EOcDTDf;2X zl=W)FqYfnmw1)_5AvG)Kva4$X<=g{>!@9lzQX8~|6H-(w7hrN@De2mIxY}~z6s&7o zU^2eW2K8qldj}_126u~}=r%naP;x-Y2j2K`5*#Zg5`j5O6PQt!N+J@;d4?!VgaTR2uu@@;S)V&iU2`5#o6_Apb|>aj04G%Gy$o=IXW>&P8r|{Igt}27*UY}!cz(o zh9QP1Nlp|=p3(etxALGnsmC`zA^5Yr$NGL+hL7z?=~~THTTo1ms%0?&pSgN86>XYwfnO{r{m)1V>%zx`NuB+qrKK69?BpL=tHwMa;lwO#cA2R>lp0#JgOHZwU?DcYTOyy%Le>{I}=?iDyWf@gcSV-TsBLj@#zNbD= zQqyHAA;-v1K0v%GS;>hxv_jI54X`^4FS$*RV+qH!|Hu(}BvX;)oH3STAql{Y3Xo?B zmKn-(OcEHU4ONt#03}2yC14II6q!sC&QF&iPJrS$&6&yw!IUI2SBRgWWK3X)3Cg&P z-%xtWRTisKk|UnvnC3aaJV`EFl9YlOIY+PY>BJ*Bm1HQvY0gfmmtH#`#hp+xo&d>0j@DbUEN<|dnHgfh`jo^#8FP}E+x%UI}U4y$x< z5)H4>3rz6y@Ox4kua(ZfT_w(DZM(e=p;l`S{$m^8or@bwt>;nV3_4rf94sT(H!PkqIf;E%j)bpue#D@Sc$Ntef)G z8S33IO8xO`V+WvpG(gi`NB#e`ceTrrqEPgSHZ=jnrBfGrj(_s}s<^o^yeF&k=j*KX5ZJ+XBZtH>B8v)7i6mz_wC^zw}ag7jokLrgPU>grWx|w&#}a`;(^92FxJ_$1%v_- zj#fyc+AWo@)4cY>71?_uoYChv6N0UXVD+@L#${7`LZosC@r4f>34D{(eZSs=f+wD) z4V!^r6BY{~gKR>{4Pd#BnR4EVS|Bz3_O$Z-SCW^lQ`Bp8Y+YIo4desakA0YrD|;e| zK_A1CY$NCjf$~!G1=XZQpo)?7gy^ATGpaejZ%d* zN0B)sqE~)h<~Rnq+TKF8%@>ln4EW&Dd*ji<49_xHZ2jED$ugUy zW|k{Gvxbf8@BDe3{m=m*4Pp4zZsy3t2&u ztzr>4JV1A9oWNvc7@Hg1`fXwTiUC*WyNwF!+9}YG0c(jeG)m7ZV%m43Z9wG6e{jg} zzrvGm{s^0@)}w9gAdWk;v-v!tu#V8g3KuX7Lr{HFfgXd}Whr_#Xxyrb)WbZrH8TSV ztMuXuqS*D=^CB|8iFuo3v^w_24^X)~P$^@Uz*0B%XSj$j0tZnLTFKS9{&K}z)4T;Q1?E7 z;W#w=uu_=Nb{s<<%mR@m0d>a9%w3g6Tb^qQBd5*O%#{u>W2aoYM&0``Ft}V!_29SkA07|PJTUX_ z60?^|>K!(hTJBBOM2iN{MuoPMZLDMBlXGb`!8c2JZDD58iV7>>@x3mGQj` z1 -> z2 -> z3 - root.add_dependency_edge(z1, deptypes="link") - z1.add_dependency_edge(z2, deptypes="link") - z2.add_dependency_edge(z3_flavor_2, deptypes="link") + root.add_dependency_edge(z1, deptypes="link", virtuals=()) + z1.add_dependency_edge(z2, deptypes="link", virtuals=()) + z2.add_dependency_edge(z3_flavor_2, deptypes="link", virtuals=()) # Indexing should prefer the link-type dep. assert "through_z1" in root["z3"].variants diff --git a/lib/spack/spack/test/spec_semantics.py b/lib/spack/spack/test/spec_semantics.py index 63e2253b6886ed..87bdd129e31a01 100644 --- a/lib/spack/spack/test/spec_semantics.py +++ b/lib/spack/spack/test/spec_semantics.py @@ -1003,7 +1003,7 @@ def test_error_message_unknown_variant(self): def test_satisfies_dependencies_ordered(self): d = Spec("zmpi ^fake") s = Spec("mpileaks") - s._add_dependency(d, deptypes=()) + s._add_dependency(d, deptypes=(), virtuals=()) assert s.satisfies("mpileaks ^zmpi ^fake") @pytest.mark.parametrize("transitive", [True, False]) @@ -1050,6 +1050,7 @@ def test_is_extension_after_round_trip_to_dict(config, mock_packages, spec_str): def test_malformed_spec_dict(): + # FIXME: This test was really testing the specific implementation with an ad-hoc test with pytest.raises(SpecError, match="malformed"): Spec.from_dict( {"spec": {"_meta": {"version": 2}, "nodes": [{"dependencies": {"name": "foo"}}]}} @@ -1057,6 +1058,7 @@ def test_malformed_spec_dict(): def test_spec_dict_hashless_dep(): + # FIXME: This test was really testing the specific implementation with an ad-hoc test with pytest.raises(SpecError, match="Couldn't parse"): Spec.from_dict( { @@ -1150,7 +1152,7 @@ def test_concretize_partial_old_dag_hash_spec(mock_packages, config): # add it to an abstract spec as a dependency top = Spec("dt-diamond") - top.add_dependency_edge(bottom, deptypes=()) + top.add_dependency_edge(bottom, deptypes=(), virtuals=()) # concretize with the already-concrete dependency top.concretize() diff --git a/lib/spack/spack/test/spec_yaml.py b/lib/spack/spack/test/spec_yaml.py index ff614c0d68dc14..7875107c9e59c9 100644 --- a/lib/spack/spack/test/spec_yaml.py +++ b/lib/spack/spack/test/spec_yaml.py @@ -43,12 +43,6 @@ def check_json_round_trip(spec): assert spec.eq_dag(spec_from_json) -def test_simple_spec(): - spec = Spec("mpileaks") - check_yaml_round_trip(spec) - check_json_round_trip(spec) - - def test_read_spec_from_signed_json(): spec_dir = os.path.join(spack.paths.test_path, "data", "mirrors", "signed_json") file_name = ( @@ -70,13 +64,6 @@ def check_spec(spec_to_check): check_spec(s) -def test_normal_spec(mock_packages): - spec = Spec("mpileaks+debug~opt") - spec.normalize() - check_yaml_round_trip(spec) - check_json_round_trip(spec) - - @pytest.mark.parametrize( "invalid_yaml", ["playing_playlist: {{ action }} playlist {{ playlist_name }}"] ) @@ -97,37 +84,28 @@ def test_invalid_json_spec(invalid_json, error_message): assert error_message in exc_msg -def test_external_spec(config, mock_packages): - spec = Spec("externaltool") - spec.concretize() - check_yaml_round_trip(spec) - check_json_round_trip(spec) - - spec = Spec("externaltest") - spec.concretize() - check_yaml_round_trip(spec) - check_json_round_trip(spec) - - -def test_ambiguous_version_spec(mock_packages): - spec = Spec("mpileaks@1.0:5.0,6.1,7.3+debug~opt") - spec.normalize() - check_yaml_round_trip(spec) - check_json_round_trip(spec) - - -def test_concrete_spec(config, mock_packages): - spec = Spec("mpileaks+debug~opt") - spec.concretize() - check_yaml_round_trip(spec) - check_json_round_trip(spec) - - -def test_yaml_multivalue(config, mock_packages): - spec = Spec('multivalue-variant foo="bar,baz"') - spec.concretize() - check_yaml_round_trip(spec) - check_json_round_trip(spec) +@pytest.mark.parametrize( + "abstract_spec", + [ + # Externals + "externaltool", + "externaltest", + # Ambiguous version spec + "mpileaks@1.0:5.0,6.1,7.3+debug~opt", + # Variants + "mpileaks+debug~opt", + 'multivalue-variant foo="bar,baz"', + # Virtuals on edges + "callpath", + "mpileaks", + ], +) +def test_roundtrip_concrete_specs(abstract_spec, default_mock_concretization): + check_yaml_round_trip(Spec(abstract_spec)) + check_json_round_trip(Spec(abstract_spec)) + concrete_spec = default_mock_concretization(abstract_spec) + check_yaml_round_trip(concrete_spec) + check_json_round_trip(concrete_spec) def test_yaml_subdag(config, mock_packages): @@ -508,6 +486,8 @@ def test_legacy_yaml(tmpdir, install_mockery, mock_packages): ("specfiles/hdf5.v017.json.gz", "xqh5iyjjtrp2jw632cchacn3l7vqzf3m", spack.spec.SpecfileV2), # Use "full hash" everywhere, see https://github.com/spack/spack/pull/28504 ("specfiles/hdf5.v019.json.gz", "iulacrbz7o5v5sbj7njbkyank3juh6d3", spack.spec.SpecfileV3), + # Add properties on edges, see https://github.com/spack/spack/pull/34821 + ("specfiles/hdf5.v020.json.gz", "vlirlcgazhvsvtundz4kug75xkkqqgou", spack.spec.SpecfileV4), ], ) def test_load_json_specfiles(specfile, expected_hash, reader_cls): diff --git a/lib/spack/spack/test/traverse.py b/lib/spack/spack/test/traverse.py index dae944ffc36892..2d9679d6ce7e03 100644 --- a/lib/spack/spack/test/traverse.py +++ b/lib/spack/spack/test/traverse.py @@ -19,7 +19,7 @@ def create_dag(nodes, edges): """ specs = {name: Spec(name) for name in nodes} for parent, child, deptypes in edges: - specs[parent].add_dependency_edge(specs[child], deptypes=deptypes) + specs[parent].add_dependency_edge(specs[child], deptypes=deptypes, virtuals=()) return specs diff --git a/lib/spack/spack/traverse.py b/lib/spack/spack/traverse.py index 848643847229b4..c0fa981ccfeb45 100644 --- a/lib/spack/spack/traverse.py +++ b/lib/spack/spack/traverse.py @@ -211,7 +211,9 @@ def get_visitor_from_args(cover, direction, deptype, key=id, visited=None, visit def with_artificial_edges(specs): """Initialize a list of edges from an imaginary root node to the root specs.""" return [ - EdgeAndDepth(edge=spack.spec.DependencySpec(parent=None, spec=s, deptypes=()), depth=0) + EdgeAndDepth( + edge=spack.spec.DependencySpec(parent=None, spec=s, deptypes=(), virtuals=()), depth=0 + ) for s in specs ] From 83e5c45dbf131a5799089cbf20f7d39e11ddef5f Mon Sep 17 00:00:00 2001 From: Massimiliano Culpo Date: Thu, 2 Mar 2023 14:04:00 +0100 Subject: [PATCH 2/2] Improve virtual reconstruction from old format --- lib/spack/spack/spec.py | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py index 925319fb79dddd..2cf2042b01f45c 100644 --- a/lib/spack/spack/spec.py +++ b/lib/spack/spack/spec.py @@ -4899,12 +4899,25 @@ def _reconstruct_virtuals_on_edges(spec): """Reconstruct virtuals on edges. Used to read from old DB and reindex. """ - possible_virtuals = [x for x in spec.package.dependencies if Spec(x).virtual] + # Collect all possible virtuals + possible_virtuals = set() + for node in spec.traverse(): + try: + possible_virtuals.update({x for x in node.package.dependencies if Spec(x).virtual}) + except Exception as e: + warnings.warn(f"cannot reconstruct virtual dependencies on package {node.name}: {e}") + continue + + # Assume all incoming edges to provider are marked with virtuals= for vspec in possible_virtuals: - if vspec in spec: - name = spec[vspec].name - for edge in spec.edges_to_dependencies(name=name): - edge.update_virtuals([vspec]) + try: + provider = spec[vspec] + except KeyError: + # Virtual not in the DAG + continue + + for edge in provider.edges_from_dependents(): + edge.update_virtuals([vspec]) class SpecfileReaderBase: