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>

7 files changed, 241 insertions, 63 deletions

diff --git a/lib/spack/spack/concretize.py b/lib/spack/spack/concretize.py
index 186cf8244f..3c3801bf5b 100644
--- a/lib/spack/spack/concretize.py
+++ b/lib/spack/spack/concretize.py
@@ -253,8 +253,7 @@ class Concretizer(object):
         if spec.architecture is None:
             spec.architecture = spack.spec.ArchSpec()
 
-        if spec.architecture.platform and \
-                (spec.architecture.os and spec.architecture.target):
+        if spec.architecture.concrete:
             return False
 
         # Get platform of nearest spec with a platform, including spec
@@ -294,22 +293,58 @@ class Concretizer(object):
 
         # 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:
-                new_target = new_target_spec.architecture.target
+                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
                 if PackagePrefs.has_preferred_targets(spec.name):
                     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 @@ class Concretizer(object):
         """
         # 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 @@ class Concretizer(object):
         """
         # 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.
diff --git a/lib/spack/spack/installer.py b/lib/spack/spack/installer.py
index bc23b4896f..c19d9018b2 100644
--- a/lib/spack/spack/installer.py
+++ b/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,21 +203,27 @@ 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}'
-              .format(pkg.spec.compiler, package_id(pkg)))
+    tty.debug('Bootstrapping {0} compiler'.format(compiler))
     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.architecture = pkg.spec.architecture
+    dep = spack.compilers.pkg_spec_for_compiler(compiler)
+
+    # 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
-    dep._dependents[pkg.name] = spack.spec.DependencySpec(
-        pkg.spec, dep, ('build',))
+    # 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
                 s in dep.traverse(order='post', root=False)]
     packages.append((dep.package, True))
@@ -647,17 +657,21 @@ class PackageInstaller(object):
         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,13 +1011,41 @@ class PackageInstaller(object):
             'config:install_missing_compilers', False)
 
         install_deps = request.install_args.get('install_deps')
-        if 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] = []
 
-                # First push any missing compilers (if requested)
-                if install_compilers:
-                    self._add_bootstrap_compilers(dep_pkg, request, all_deps)
+            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
 
                 dep_id = package_id(dep_pkg)
                 if dep_id not in self.build_tasks:
@@ -1014,13 +1056,9 @@ class PackageInstaller(object):
                 # 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)
 
@@ -1750,6 +1788,11 @@ class BuildTask(object):
         # 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,
@@ -1770,7 +1813,10 @@ class BuildTask(object):
                                                   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)
diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py
index 713f9a50ae..c5dbcb0151 100644
--- a/lib/spack/spack/solver/asp.py
+++ b/lib/spack/spack/solver/asp.py
@@ -1112,6 +1112,12 @@ class SpackSolverSetup(object):
         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
@@ -1154,7 +1160,9 @@ class SpackSolverSetup(object):
 
             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)
 
@@ -1289,6 +1297,10 @@ class SpackSolverSetup(object):
 
         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
diff --git a/lib/spack/spack/solver/concretize.lp b/lib/spack/spack/solver/concretize.lp
index e991e66ff2..7c94b7a8a4 100644
--- a/lib/spack/spack/solver/concretize.lp
+++ b/lib/spack/spack/solver/concretize.lp
@@ -349,17 +349,22 @@ 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),
-     root(Package), node_target(Package, Target),
-     not node_target_match_pref(Package, _),
+     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, _).
+
 node_target_match(Package, 1)
   :- node_target(Package, Target), node_target_match_pref(Package, Target).
 
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index a424dc60c9..f0ae6a1431 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -359,13 +359,11 @@ class ArchSpec(object):
                     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):
-        self_target = self.target
-
-        need_to_check = bool(other_target) if strict or self.concrete \
-            else bool(other_target and self_target)
+    def target_satisfies(self, other, strict):
+        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 @@ class ArchSpec(object):
         if self.target is None:
             return False
 
-        for target_range in str(other_target).split(','):
-            t_min, sep, t_max = target_range.partition(':')
-
-            # Checking against a single specific target
-            if not sep and self_target == t_min:
-                return True
-
-            if not sep and self_target != t_min:
-                return False
+        return bool(self.target_intersection(other))
 
-            # Check against a range
-            min_ok = self_target.microarchitecture >= t_min if t_min else True
-            max_ok = self_target.microarchitecture <= t_max if t_max else True
+    def target_constrain(self, other):
+        if not other.target_satisfies(self, strict=False):
+            raise UnsatisfiableArchitectureSpecError(self, other)
 
-            if min_ok and max_ok:
-                return True
+        if self.target_concrete:
+            return False
+        elif other.target_concrete:
+            self.target = other.target
+            return True
 
-        return False
+        # Compute the intersection of every combination of ranges in the lists
+        results = self.target_intersection(other)
+        # Do we need to dedupe here?
+        self.target = ','.join(results)
+
+    def target_intersection(self, other):
+        results = []
+
+        if not self.target or not other.target:
+            return results
+
+        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 @@ class ArchSpec(object):
         """
         other = self._autospec(other)
 
-        if not self.satisfies(other):
-            raise UnsatisfiableArchitectureSpecError(self, other)
+        if not other.satisfies(self):
+            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 @@ class ArchSpec(object):
     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([
diff --git a/lib/spack/spack/test/architecture.py b/lib/spack/spack/test/architecture.py
index 7af5a8d150..66c87240a1 100644
--- a/lib/spack/spack/test/architecture.py
+++ b/lib/spack/spack/test/architecture.py
@@ -12,6 +12,7 @@ import platform as py_platform
 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
diff --git a/lib/spack/spack/test/installer.py b/lib/spack/spack/test/installer.py
index cec3721d0d..3bf818544e 100644
--- a/lib/spack/spack/test/installer.py
+++ b/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 test_packages_needed_to_bootstrap_compiler_packages(install_mockery,
     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 test_add_bootstrap_compilers(install_mockery, monkeypatch):
     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