From 74a5cd2bb04e819c9fd2920ff81bb2f2fbd28756 Mon Sep 17 00:00:00 2001 From: Greg Becker Date: Mon, 15 May 2023 22:08:34 -0700 Subject: [PATCH] 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(-) diff --git a/lib/spack/spack/environment/environment.py b/lib/spack/spack/environment/environment.py index bee1d629a60..a4312df6dc7 100644 --- a/lib/spack/spack/environment/environment.py +++ b/lib/spack/spack/environment/environment.py @@ -1402,6 +1402,10 @@ def _concretize_together_where_possible( 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 @@ def _concretize_together_where_possible( 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 @@ def _concretize_together( 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 @@ def _concretize_together( ) 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