From 3765a5f7f847ecec69fc88a8b4dcaa4994217e94 Mon Sep 17 00:00:00 2001 From: Greg Becker Date: Mon, 15 May 2023 22:08:34 -0700 Subject: unify: when_possible and unify: true -- Bugfix for error in 37438 (#37681) Two bugs came in from #37438 1. `unify: when_possible` was broken, because of an incorrect assertion. abstract/concrete spec pairs were compared against the results that were in the process of being computed, rather than against the previous results. 2. `unify: true` had an ordering bug that could mix the association between abstract and concrete specs - [x] 1 is resolved by creating a lookup from old concrete specs to old abstract specs, and we use that to associate the "new" concrete specs that happen to be the old ones with their abstract specs (since those are stripped out for concretization - [x] 2 is resolved by combining the new and old abstract as lists instead of combining them as sets. This is important because `set() | set()` does not make any ordering promises, even though set ordering is otherwise guaranteed in `python@3.7:` --- lib/spack/spack/environment/environment.py | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) (limited to 'lib') diff --git a/lib/spack/spack/environment/environment.py b/lib/spack/spack/environment/environment.py index bee1d629a6..a4312df6dc 100644 --- a/lib/spack/spack/environment/environment.py +++ b/lib/spack/spack/environment/environment.py @@ -1402,6 +1402,10 @@ class Environment: if not new_user_specs: return [] + old_concrete_to_abstract = { + concrete: abstract for (abstract, concrete) in self.concretized_specs() + } + self.concretized_user_specs = [] self.concretized_order = [] self.specs_by_hash = {} @@ -1413,11 +1417,13 @@ class Environment: result = [] for abstract, concrete in sorted(result_by_user_spec.items()): + # If the "abstract" spec is a concrete spec from the previous concretization + # translate it back to an abstract spec. Otherwise, keep the abstract spec + abstract = old_concrete_to_abstract.get(abstract, abstract) if abstract in new_user_specs: result.append((abstract, concrete)) - else: - assert (abstract, concrete) in result self._add_concrete_spec(abstract, concrete) + return result def _concretize_together( @@ -1436,7 +1442,7 @@ class Environment: self.specs_by_hash = {} try: - concrete_specs = spack.concretize.concretize_specs_together( + concrete_specs: List[spack.spec.Spec] = spack.concretize.concretize_specs_together( *specs_to_concretize, tests=tests ) except spack.error.UnsatisfiableSpecError as e: @@ -1455,11 +1461,14 @@ class Environment: ) raise - # zip truncates the longer list, which is exactly what we want here - concretized_specs = [x for x in zip(new_user_specs | kept_user_specs, concrete_specs)] + # set() | set() does not preserve ordering, even though sets are ordered + ordered_user_specs = list(new_user_specs) + list(kept_user_specs) + concretized_specs = [x for x in zip(ordered_user_specs, concrete_specs)] for abstract, concrete in concretized_specs: self._add_concrete_spec(abstract, concrete) - return concretized_specs + + # zip truncates the longer list, which is exactly what we want here + return list(zip(new_user_specs, concrete_specs)) def _concretize_separately(self, tests=False): """Concretization strategy that concretizes separately one -- cgit v1.2.3-60-g2f50