ci: fixes for compiler bootstrapping (#17563)

This PR addresses a number of issues related to compiler bootstrapping.

Specifically:
1. Collect compilers to be bootstrapped while queueing in installer
Compiler tasks currently have an incomplete list in their task.dependents,
making those packages fail to install as they think they have not all their
dependencies installed. This PR collects the dependents and sets them on
compiler tasks.

2. allow boostrapped compilers to back off target
Bootstrapped compilers may be built with a compiler that doesn't support
the target used by the rest of the spec.  Allow them to build with less
aggressive target optimization settings.

3. Support for target ranges
Backing off the target necessitates computing target ranges, so make Spack
handle those properly.  Notably, this adds an intersection method for target
ranges and fixes the way ranges are satisfied and constrained on Spec objects.

This PR also:
- adds testing
- improves concretizer handling of target ranges

Co-authored-by: Harmen Stoppels <harmenstoppels@gmail.com>
Co-authored-by: Gregory Becker <becker33@llnl.gov>
Co-authored-by: Massimiliano Culpo <massimiliano.culpo@gmail.com>

lib/spack/spack/concretize.py

@@ -253,8 +253,7 @@ def concretize_architecture(self, spec):
         if spec.architecture is None:
             spec.architecture = spack.spec.ArchSpec()
 
-        if spec.architecture.platform and \
+        if spec.architecture.concrete:
-                (spec.architecture.os and spec.architecture.target):
             return False
 
         # Get platform of nearest spec with a platform, including spec
@@ -294,15 +293,28 @@ def concretize_architecture(self, spec):
 
         # Get the nearest spec with relevant platform and a target
         # Generally, same algorithm as finding os
+        curr_target = None
         if spec.architecture.target:
+            curr_target = spec.architecture.target
+        if spec.architecture.target and spec.architecture.target_concrete:
             new_target = spec.architecture.target
         else:
             new_target_spec = find_spec(
                 spec, lambda x: (x.architecture and
                                  x.architecture.platform == str(new_plat) and
-                                 x.architecture.target)
+                                 x.architecture.target and
+                                 x.architecture.target != curr_target)
             )
             if new_target_spec:
+                if curr_target:
+                    # constrain one target by the other
+                    new_target_arch = spack.spec.ArchSpec(
+                        (None, None, new_target_spec.architecture.target))
+                    curr_target_arch = spack.spec.ArchSpec(
+                        (None, None, curr_target))
+                    curr_target_arch.constrain(new_target_arch)
+                    new_target = curr_target_arch.target
+                else:
                     new_target = new_target_spec.architecture.target
             else:
                 # To get default platform, consider package prefs
@@ -310,6 +322,29 @@ def concretize_architecture(self, spec):
                     new_target = self.target_from_package_preferences(spec)
                 else:
                     new_target = new_plat.target('default_target')
+                if curr_target:
+                    # convert to ArchSpec to compare satisfaction
+                    new_target_arch = spack.spec.ArchSpec(
+                        (None, None, str(new_target)))
+                    curr_target_arch = spack.spec.ArchSpec(
+                        (None, None, str(curr_target)))
+
+                    if not new_target_arch.satisfies(curr_target_arch):
+                        # new_target is an incorrect guess based on preferences
+                        # and/or default
+                        valid_target_ranges = str(curr_target).split(',')
+                        for target_range in valid_target_ranges:
+                            t_min, t_sep, t_max = target_range.partition(':')
+                            if not t_sep:
+                                new_target = t_min
+                                break
+                            elif t_max:
+                                new_target = t_max
+                                break
+                            elif t_min:
+                                # TODO: something better than picking first
+                                new_target = t_min
+                                break
 
         # Construct new architecture, compute whether spec changed
         arch_spec = (str(new_plat), str(new_os), str(new_target))
@@ -384,7 +419,7 @@ def concretize_compiler(self, spec):
         """
         # Pass on concretizing the compiler if the target or operating system
         # is not yet determined
-        if not (spec.architecture.os and spec.architecture.target):
+        if not spec.architecture.concrete:
             # We haven't changed, but other changes need to happen before we
             # continue. `return True` here to force concretization to keep
             # running.
@@ -482,7 +517,7 @@ def concretize_compiler_flags(self, spec):
         """
         # Pass on concretizing the compiler flags if the target or operating
         # system is not set.
-        if not (spec.architecture.os and spec.architecture.target):
+        if not spec.architecture.concrete:
             # We haven't changed, but other changes need to happen before we
             # continue. `return True` here to force concretization to keep
             # running.

lib/spack/spack/installer.py

@@ -182,7 +182,7 @@ def _do_fake_install(pkg):
     dump_packages(pkg.spec, packages_dir)
 
 
-def _packages_needed_to_bootstrap_compiler(pkg):
+def _packages_needed_to_bootstrap_compiler(compiler, architecture, pkgs):
     """
     Return a list of packages required to bootstrap `pkg`s compiler
 
@@ -190,7 +190,11 @@ def _packages_needed_to_bootstrap_compiler(pkg):
     matches the package spec.
 
     Args:
-        pkg (Package): the package that may need its compiler installed
+        compiler (CompilerSpec): the compiler to bootstrap
+        architecture (ArchSpec): the architecture for which to boostrap the
+            compiler
+        pkgs (list of PackageBase): the packages that may need their compiler
+            installed
 
     Return:
         (list) list of tuples, (PackageBase, bool), for concretized compiler-
@@ -199,19 +203,25 @@ def _packages_needed_to_bootstrap_compiler(pkg):
             (``True``) or one of its dependencies (``False``).  The list
             will be empty if there are no compilers.
     """
-    tty.debug('Bootstrapping {0} compiler for {1}'
+    tty.debug('Bootstrapping {0} compiler'.format(compiler))
-              .format(pkg.spec.compiler, package_id(pkg)))
     compilers = spack.compilers.compilers_for_spec(
-        pkg.spec.compiler, arch_spec=pkg.spec.architecture)
+        compiler, arch_spec=architecture)
     if compilers:
         return []
 
-    dep = spack.compilers.pkg_spec_for_compiler(pkg.spec.compiler)
+    dep = spack.compilers.pkg_spec_for_compiler(compiler)
-    dep.architecture = pkg.spec.architecture
+
+    # Set the architecture for the compiler package in a way that allows the
+    # concretizer to back off if needed for the older bootstrapping compiler
+    dep.constrain('platform=%s' % str(architecture.platform))
+    dep.constrain('os=%s' % str(architecture.os))
+    dep.constrain('target=%s:' %
+                  architecture.target.microarchitecture.family.name)
     # concrete CompilerSpec has less info than concrete Spec
     # concretize as Spec to add that information
     dep.concretize()
-    # mark compiler as depended-on by the package that uses it
+    # mark compiler as depended-on by the packages that use it
+    for pkg in pkgs:
         dep._dependents[pkg.name] = spack.spec.DependencySpec(
             pkg.spec, dep, ('build',))
     packages = [(s.package, False) for
@@ -647,17 +657,21 @@ def __str__(self):
         return '{0}: {1}; {2}; {3}; {4}'.format(
             self.pid, requests, tasks, installed, failed)
 
-    def _add_bootstrap_compilers(self, pkg, request, all_deps):
+    def _add_bootstrap_compilers(
+            self, compiler, architecture, pkgs, request, all_deps):
         """
         Add bootstrap compilers and dependencies to the build queue.
 
         Args:
-            pkg (PackageBase): the package with possible compiler dependencies
+            compiler: the compiler to boostrap
+            architecture: the architecture for which to bootstrap the compiler
+            pkgs (PackageBase): the package with possible compiler dependencies
             request (BuildRequest): the associated install request
             all_deps (defaultdict(set)): dictionary of all dependencies and
                 associated dependents
         """
-        packages = _packages_needed_to_bootstrap_compiler(pkg)
+        packages = _packages_needed_to_bootstrap_compiler(
+            compiler, architecture, pkgs)
         for (comp_pkg, is_compiler) in packages:
             if package_id(comp_pkg) not in self.build_tasks:
                 self._add_init_task(comp_pkg, request, is_compiler, all_deps)
@@ -997,14 +1011,42 @@ def _add_tasks(self, request, all_deps):
             'config:install_missing_compilers', False)
 
         install_deps = request.install_args.get('install_deps')
+        # Bootstrap compilers first
+        if install_deps and install_compilers:
+            packages_per_compiler = {}
+
+            for dep in request.traverse_dependencies():
+                dep_pkg = dep.package
+                compiler = dep_pkg.spec.compiler
+                arch = dep_pkg.spec.architecture
+                if compiler not in packages_per_compiler:
+                    packages_per_compiler[compiler] = {}
+
+                if arch not in packages_per_compiler[compiler]:
+                    packages_per_compiler[compiler][arch] = []
+
+                packages_per_compiler[compiler][arch].append(dep_pkg)
+
+            compiler = request.pkg.spec.compiler
+            arch = request.pkg.spec.architecture
+
+            if compiler not in packages_per_compiler:
+                packages_per_compiler[compiler] = {}
+
+            if arch not in packages_per_compiler[compiler]:
+                packages_per_compiler[compiler][arch] = []
+
+            packages_per_compiler[compiler][arch].append(request.pkg)
+
+            for compiler, archs in packages_per_compiler.items():
+                for arch, packages in archs.items():
+                    self._add_bootstrap_compilers(
+                        compiler, arch, packages, request, all_deps)
+
         if install_deps:
             for dep in request.traverse_dependencies():
                 dep_pkg = dep.package
 
-                # First push any missing compilers (if requested)
-                if install_compilers:
-                    self._add_bootstrap_compilers(dep_pkg, request, all_deps)
-
                 dep_id = package_id(dep_pkg)
                 if dep_id not in self.build_tasks:
                     self._add_init_task(dep_pkg, request, False, all_deps)
@@ -1014,13 +1056,9 @@ def _add_tasks(self, request, all_deps):
                 # of the spec.
                 spack.store.db.clear_failure(dep, force=False)
 
-            # Push any missing compilers (if requested) as part of the
-            # package dependencies.
-            if install_compilers:
-                self._add_bootstrap_compilers(request.pkg, request, all_deps)
-
         install_package = request.install_args.get('install_package')
         if install_package and request.pkg_id not in self.build_tasks:
+
             # Be sure to clear any previous failure
             spack.store.db.clear_failure(request.spec, force=True)
 
@@ -1752,6 +1790,11 @@ def __init__(self, pkg, request, compiler, start, attempts, status,
         # to support tracking of parallel, multi-spec, environment installs.
         self.dependents = set(get_dependent_ids(self.pkg.spec))
 
+        tty.debug(
+            'Pkg id {0} has the following dependents:'.format(self.pkg_id))
+        for dep_id in self.dependents:
+            tty.debug('- {0}'.format(dep_id))
+
         # Set of dependencies
         #
         # Be consistent wrt use of dependents and dependencies.  That is,
@@ -1772,7 +1815,10 @@ def __init__(self, pkg, request, compiler, start, attempts, status,
                                                   arch_spec=arch_spec):
             # The compiler is in the queue, identify it as dependency
             dep = spack.compilers.pkg_spec_for_compiler(compiler_spec)
-            dep.architecture = arch_spec
+            dep.constrain('platform=%s' % str(arch_spec.platform))
+            dep.constrain('os=%s' % str(arch_spec.os))
+            dep.constrain('target=%s:' %
+                          arch_spec.target.microarchitecture.family.name)
             dep.concretize()
             dep_id = package_id(dep.package)
             self.dependencies.add(dep_id)

lib/spack/spack/solver/asp.py

@@ -1112,6 +1112,12 @@ def target_defaults(self, specs):
         self.gen.h2('Target compatibility')
 
         compatible_targets = [uarch] + uarch.ancestors
+        additional_targets_in_family = sorted([
+            t for t in archspec.cpu.TARGETS.values()
+            if (t.family.name == uarch.family.name and
+                t not in compatible_targets)
+        ], key=lambda x: len(x.ancestors), reverse=True)
+        compatible_targets += additional_targets_in_family
         compilers = self.possible_compilers
 
         # this loop can be used to limit the number of targets
@@ -1155,7 +1161,9 @@ def target_defaults(self, specs):
             print("TTYPE:", type(platform.target(spec.target.name)))
             target = archspec.cpu.TARGETS.get(spec.target.name)
             if not target:
-                raise ValueError("Invalid target: ", spec.target.name)
+                self.target_ranges(spec, None)
+                continue
+
             if target not in compatible_targets:
                 compatible_targets.append(target)
 
@@ -1290,6 +1298,10 @@ def define_target_constraints(self):
 
         def _all_targets_satisfiying(single_constraint):
             allowed_targets = []
+
+            if ':' not in single_constraint:
+                return [single_constraint]
+
             t_min, _, t_max = single_constraint.partition(':')
             for test_target in archspec.cpu.TARGETS.values():
                 # Check lower bound

lib/spack/spack/solver/concretize.lp

@@ -349,16 +349,21 @@ node_target_weight(Package, Weight)
     target_weight(Target, Package, Weight).
 
 % compatibility rules for targets among nodes
-node_target_match_pref(Package, Target) :- node_target_set(Package, Target).
 node_target_match_pref(Dependency, Target)
-  :- depends_on(Package, Dependency), node_target_match_pref(Package, Target),
+  :- depends_on(Package, Dependency),
+     node_target_match_pref(Package, Target),
+     not node_target_set(Dependency, _).
+
+node_target_match_pref(Dependency, Target)
+  :- depends_on(Package, Dependency),
+     node_target_set(Package, Target),
+     not node_target_match_pref(Package, Target),
      not node_target_set(Dependency, _).
 
 node_target_match_pref(Dependency, Target)
   :- depends_on(Package, Dependency),
      root(Package), node_target(Package, Target),
-     not node_target_match_pref(Package, _),
+     not node_target_match_pref(Package, _).
-     not node_target_set(Dependency, _).
 
 node_target_match(Package, 1)
   :- node_target(Package, Target), node_target_match_pref(Package, Target).

lib/spack/spack/spec.py

@@ -359,13 +359,11 @@ def satisfies(self, other, strict=False):
                     return False
 
         # Check target
-        return self._satisfies_target(other.target, strict=strict)
+        return self.target_satisfies(other, strict=strict)
 
-    def _satisfies_target(self, other_target, strict):
+    def target_satisfies(self, other, strict):
-        self_target = self.target
+        need_to_check = bool(other.target) if strict or self.concrete \
-
+            else bool(other.target and self.target)
-        need_to_check = bool(other_target) if strict or self.concrete \
-            else bool(other_target and self_target)
 
         # If there's no need to check we are fine
         if not need_to_check:
@@ -375,24 +373,68 @@ def _satisfies_target(self, other_target, strict):
         if self.target is None:
             return False
 
-        for target_range in str(other_target).split(','):
+        return bool(self.target_intersection(other))
-            t_min, sep, t_max = target_range.partition(':')
 
-            # Checking against a single specific target
+    def target_constrain(self, other):
-            if not sep and self_target == t_min:
+        if not other.target_satisfies(self, strict=False):
+            raise UnsatisfiableArchitectureSpecError(self, other)
+
+        if self.target_concrete:
+            return False
+        elif other.target_concrete:
+            self.target = other.target
             return True
 
-            if not sep and self_target != t_min:
+        # Compute the intersection of every combination of ranges in the lists
-                return False
+        results = self.target_intersection(other)
+        # Do we need to dedupe here?
+        self.target = ','.join(results)
 
-            # Check against a range
+    def target_intersection(self, other):
-            min_ok = self_target.microarchitecture >= t_min if t_min else True
+        results = []
-            max_ok = self_target.microarchitecture <= t_max if t_max else True
 
-            if min_ok and max_ok:
+        if not self.target or not other.target:
-                return True
+            return results
 
-        return False
+        for s_target_range in str(self.target).split(','):
+            s_min, s_sep, s_max = s_target_range.partition(':')
+            for o_target_range in str(other.target).split(','):
+                o_min, o_sep, o_max = o_target_range.partition(':')
+
+                if not s_sep:
+                    # s_target_range is a concrete target
+                    # get a microarchitecture reference for at least one side
+                    # of each comparison so we can use archspec comparators
+                    s_comp = spack.architecture.Target(s_min).microarchitecture
+                    if not o_sep:
+                        if s_min == o_min:
+                            results.append(s_min)
+                    elif (not o_min or s_comp >= o_min) and (
+                            not o_max or s_comp <= o_max):
+                        results.append(s_min)
+                elif not o_sep:
+                    # "cast" to microarchitecture
+                    o_comp = spack.architecture.Target(o_min).microarchitecture
+                    if (not s_min or o_comp >= s_min) and (
+                            not s_max or o_comp <= s_max):
+                        results.append(o_min)
+                else:
+                    # Take intersection of two ranges
+                    # Lots of comparisons needed
+                    _s_min = spack.architecture.Target(s_min).microarchitecture
+                    _s_max = spack.architecture.Target(s_max).microarchitecture
+                    _o_min = spack.architecture.Target(o_min).microarchitecture
+                    _o_max = spack.architecture.Target(o_max).microarchitecture
+
+                    n_min = s_min if _s_min >= _o_min else o_min
+                    n_max = s_max if _s_max <= _o_max else o_max
+                    _n_min = spack.architecture.Target(n_min).microarchitecture
+                    _n_max = spack.architecture.Target(n_max).microarchitecture
+                    if _n_min == _n_max:
+                        results.append(n_min)
+                    elif not n_min or not n_max or _n_min < _n_max:
+                        results.append('%s:%s' % (n_min, n_max))
+        return results
 
     def constrain(self, other):
         """Projects all architecture fields that are specified in the given
@@ -409,16 +451,18 @@ def constrain(self, other):
         """
         other = self._autospec(other)
 
-        if not self.satisfies(other):
+        if not other.satisfies(self):
-            raise UnsatisfiableArchitectureSpecError(self, other)
+            raise UnsatisfiableArchitectureSpecError(other, self)
 
         constrained = False
-        for attr in ('platform', 'os', 'target'):
+        for attr in ('platform', 'os'):
             svalue, ovalue = getattr(self, attr), getattr(other, attr)
             if svalue is None and ovalue is not None:
                 setattr(self, attr, ovalue)
                 constrained = True
 
+        self.target_constrain(other)
+
         return constrained
 
     def copy(self):
@@ -431,7 +475,13 @@ def copy(self):
     def concrete(self):
         """True if the spec is concrete, False otherwise"""
         # return all(v for k, v in six.iteritems(self.to_cmp_dict()))
-        return self.platform and self.os and self.target
+        return (self.platform and self.os and self.target and
+                self.target_concrete)
+
+    @property
+    def target_concrete(self):
+        """True if the target is not a range or list."""
+        return ':' not in str(self.target) and ',' not in str(self.target)
 
     def to_dict(self):
         d = syaml.syaml_dict([

lib/spack/spack/test/architecture.py

@@ -12,6 +12,7 @@
 import pytest
 
 import spack.architecture
+import spack.concretize
 from spack.spec import Spec
 from spack.platforms.cray import Cray
 from spack.platforms.linux import Linux
@@ -223,3 +224,29 @@ def test_satisfy_strict_constraint_when_not_concrete(
     architecture = spack.spec.ArchSpec(architecture_tuple)
     constraint = spack.spec.ArchSpec(constraint_tuple)
     assert not architecture.satisfies(constraint, strict=True)
+
+
+@pytest.mark.parametrize('root_target_range,dep_target_range,result', [
+    (('x86_64:nocona', 'x86_64:core2', 'nocona')),  # pref not in intersection
+    (('x86_64:core2', 'x86_64:nocona', 'nocona')),
+    (('x86_64:haswell', 'x86_64:mic_knl', 'core2')),  # pref in intersection
+    (('ivybridge', 'nocona:skylake', 'ivybridge')),  # one side concrete
+    (('haswell:icelake', 'broadwell', 'broadwell')),
+    # multiple ranges in lists with multiple overlaps
+    (('x86_64:nocona,haswell:broadwell', 'nocona:haswell,skylake:',
+      'nocona')),
+    # lists with concrete targets, lists compared to ranges
+    (('x86_64,haswell', 'core2:broadwell', 'haswell'))
+])
+@pytest.mark.usefixtures('mock_packages', 'config')
+def test_concretize_target_ranges(
+        root_target_range, dep_target_range, result
+):
+    # use foobar=bar to make the problem simpler for the old concretizer
+    # the new concretizer should not need that help
+    spec = Spec('a %%gcc@10 foobar=bar target=%s ^b target=%s' %
+                (root_target_range, dep_target_range))
+    with spack.concretize.disable_compiler_existence_check():
+        spec.concretize()
+
+    assert str(spec).count('arch=test-debian6-%s' % result) == 2

lib/spack/spack/test/installer.py

@@ -450,7 +450,8 @@ def test_packages_needed_to_bootstrap_compiler_none(install_mockery):
     spec.concretize()
     assert spec.concrete
 
-    packages = inst._packages_needed_to_bootstrap_compiler(spec.package)
+    packages = inst._packages_needed_to_bootstrap_compiler(
+        spec.compiler, spec.architecture, [spec.package])
     assert not packages
 
 
@@ -468,7 +469,8 @@ def _conc_spec(compiler):
     monkeypatch.setattr(spack.compilers, 'pkg_spec_for_compiler', _conc_spec)
     monkeypatch.setattr(spack.spec.Spec, 'concretize', _noop)
 
-    packages = inst._packages_needed_to_bootstrap_compiler(spec.package)
+    packages = inst._packages_needed_to_bootstrap_compiler(
+        spec.compiler, spec.architecture, [spec.package])
     assert packages
 
 
@@ -626,7 +628,7 @@ def test_check_deps_status_upstream(install_mockery, monkeypatch):
 def test_add_bootstrap_compilers(install_mockery, monkeypatch):
     from collections import defaultdict
 
-    def _pkgs(pkg):
+    def _pkgs(compiler, architecture, pkgs):
         spec = spack.spec.Spec('mpi').concretized()
         return [(spec.package, True)]
 
@@ -636,7 +638,8 @@ def _pkgs(pkg):
     all_deps = defaultdict(set)
 
     monkeypatch.setattr(inst, '_packages_needed_to_bootstrap_compiler', _pkgs)
-    installer._add_bootstrap_compilers(request.pkg, request, all_deps)
+    installer._add_bootstrap_compilers(
+        'fake', 'fake', [request.pkg], request, all_deps)
 
     ids = list(installer.build_tasks)
     assert len(ids) == 1