Make "minimal" the default duplicate strategy (#39621)

* Allow branching out of the "generic build" unification set

For cases like the one in https://github.com/spack/spack/pull/39661
we need to relax rules on unification sets.

The issue is that, right now, nodes in the "generic build" unification
set are unified together with their build dependencies. This was done
out of caution to avoid the risk of circular dependencies, which would
ultimately cause a very slow solve.

For build-tools like Cython, however, the build dependencies is masked
by a long chain of "build, run" dependencies that belong in the
"generic build" unification space.

To allow splitting on cases like this, we relax the rule disallowing
branching out of the "generic build" unification set.

* Fix issue with pure build virtual dependencies

Pure build virtual dependencies were not accounted properly in the
list of possible virtuals. This caused some facts connecting virtuals
to the corresponding providers to not be emitted, and in the end
lead to unsat problems.

* Fixed a few issues in packages

py-gevent: restore dependency on py-cython@3
jsoncpp: fix typo in build dependency
ecp-data-vis-sdk: update spack.yaml and cmake recipe
py-statsmodels: add v0.13.5

* Make dependency on "blt" of type "build"

etc/spack/defaults/concretizer.yaml

@@ -41,4 +41,4 @@ concretizer:
     # "none": allows a single node for any package in the DAG.
     # "minimal": allows the duplication of 'build-tools' nodes only (e.g. py-setuptools, cmake etc.)
     # "full" (experimental): allows separation of the entire build-tool stack (e.g. the entire "cmake" subDAG)
-    strategy: none
+    strategy: minimal

lib/spack/docs/build_settings.rst

@@ -3,6 +3,103 @@
 
    SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
+
+.. _concretizer-options:
+
+==========================================
+Concretization Settings (concretizer.yaml)
+==========================================
+
+The ``concretizer.yaml`` configuration file allows to customize aspects of the
+algorithm used to select the dependencies you install. The default configuration
+is the following:
+
+.. literalinclude:: _spack_root/etc/spack/defaults/concretizer.yaml
+   :language: yaml
+
+--------------------------------
+Reuse already installed packages
+--------------------------------
+
+The ``reuse`` attribute controls whether Spack will prefer to use installed packages (``true``), or
+whether it will do a "fresh" installation and prefer the latest settings from
+``package.py`` files and ``packages.yaml`` (``false``).
+You can use:
+
+.. code-block:: console
+
+   % spack install --reuse <spec>
+
+to enable reuse for a single installation, and you can use:
+
+.. code-block:: console
+
+   spack install --fresh <spec>
+
+to do a fresh install if ``reuse`` is enabled by default.
+``reuse: true`` is the default.
+
+------------------------------------------
+Selection of the target microarchitectures
+------------------------------------------
+
+The options under the ``targets`` attribute control which targets are considered during a solve.
+Currently the options in this section are only configurable from the ``concretizer.yaml`` file
+and there are no corresponding command line arguments to enable them for a single solve.
+
+The ``granularity`` option can take two possible values: ``microarchitectures`` and ``generic``.
+If set to:
+
+.. code-block:: yaml
+
+   concretizer:
+     targets:
+       granularity: microarchitectures
+
+Spack will consider all the microarchitectures known to ``archspec`` to label nodes for
+compatibility. If instead the option is set to:
+
+.. code-block:: yaml
+
+   concretizer:
+     targets:
+       granularity: generic
+
+Spack will consider only generic microarchitectures. For instance, when running on an
+Haswell node, Spack will consider ``haswell`` as the best target in the former case and
+``x86_64_v3`` as the best target in the latter case.
+
+The ``host_compatible`` option is a Boolean option that determines whether or not the
+microarchitectures considered during the solve are constrained to be compatible with the
+host Spack is currently running on. For instance, if this option is set to ``true``, a
+user cannot concretize for ``target=icelake`` while running on an Haswell node.
+
+---------------
+Duplicate nodes
+---------------
+
+The ``duplicates`` attribute controls whether the DAG can contain multiple configurations of
+the same package. This is mainly relevant for build dependencies, which may have their version
+pinned by some nodes, and thus be required at different versions by different nodes in the same
+DAG.
+
+The ``strategy`` option controls how the solver deals with duplicates. If the value is ``none``,
+then a single configuration per package is allowed in the DAG. This means, for instance, that only
+a single ``cmake`` or a single ``py-setuptools`` version is allowed. The result would be a slightly
+faster concretization, at the expense of making a few specs unsolvable.
+
+If the value is ``minimal`` Spack will allow packages tagged as ``build-tools`` to have duplicates.
+This allows, for instance, to concretize specs whose nodes require different, and incompatible, ranges
+of some build tool. For instance, in the figure below the latest `py-shapely` requires a newer `py-setuptools`,
+while `py-numpy` still needs an older version:
+
+.. figure::  images/shapely_duplicates.svg
+   :scale: 70 %
+   :align: center
+
+Up to Spack v0.20 ``duplicates:strategy:none`` was the default (and only) behavior. From Spack v0.21 the
+default behavior is ``duplicates:strategy:minimal``.
+
 .. _build-settings:
 
 ================================
@@ -232,76 +329,6 @@ Specific limitations include:
   then Spack will not add a new external entry (``spack config blame packages``
   can help locate all external entries).
 
-.. _concretizer-options:
-
-----------------------
-Concretizer options
-----------------------
-
-``packages.yaml`` gives the concretizer preferences for specific packages,
-but you can also use ``concretizer.yaml`` to customize aspects of the
-algorithm it uses to select the dependencies you install:
-
-.. literalinclude:: _spack_root/etc/spack/defaults/concretizer.yaml
-   :language: yaml
-
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-Reuse already installed packages
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-
-The ``reuse`` attribute controls whether Spack will prefer to use installed packages (``true``), or
-whether it will do a "fresh" installation and prefer the latest settings from
-``package.py`` files and ``packages.yaml`` (``false``).
-You can use:
-
-.. code-block:: console
-
-   % spack install --reuse <spec>
-
-to enable reuse for a single installation, and you can use:
-
-.. code-block:: console
-
-   spack install --fresh <spec>
-
-to do a fresh install if ``reuse`` is enabled by default.
-``reuse: true`` is the default.
-
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-Selection of the target microarchitectures
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-
-The options under the ``targets`` attribute control which targets are considered during a solve.
-Currently the options in this section are only configurable from the ``concretizer.yaml`` file
-and there are no corresponding command line arguments to enable them for a single solve.
-
-The ``granularity`` option can take two possible values: ``microarchitectures`` and ``generic``.
-If set to:
-
-.. code-block:: yaml
-
-   concretizer:
-     targets:
-       granularity: microarchitectures
-
-Spack will consider all the microarchitectures known to ``archspec`` to label nodes for
-compatibility. If instead the option is set to:
-
-.. code-block:: yaml
-
-   concretizer:
-     targets:
-       granularity: generic
-
-Spack will consider only generic microarchitectures. For instance, when running on an
-Haswell node, Spack will consider ``haswell`` as the best target in the former case and
-``x86_64_v3`` as the best target in the latter case.
-
-The ``host_compatible`` option is a Boolean option that determines whether or not the
-microarchitectures considered during the solve are constrained to be compatible with the
-host Spack is currently running on. For instance, if this option is set to ``true``, a
-user cannot concretize for ``target=icelake`` while running on an Haswell node.
-
 .. _package-requirements:
 
 --------------------

lib/spack/spack/solver/asp.py

@@ -2595,6 +2595,7 @@ class SpecBuilder:
                 r"^node_compiler$",
                 r"^package_hash$",
                 r"^root$",
+                r"^variant_default_value_from_cli$",
                 r"^virtual_node$",
                 r"^virtual_root$",
             ]

lib/spack/spack/solver/concretize.lp

@@ -20,7 +20,7 @@
 
 % Integrity constraints on DAG nodes
 :- attr("root", PackageNode), not attr("node", PackageNode).
-:- attr("version", PackageNode), not attr("node", PackageNode).
+:- attr("version", PackageNode, _), not attr("node", PackageNode), not attr("virtual_node", PackageNode).
 :- attr("node_version_satisfies", PackageNode), not attr("node", PackageNode).
 :- attr("hash", PackageNode, _), not attr("node", PackageNode).
 :- attr("node_platform", PackageNode, _), not attr("node", PackageNode).
@@ -58,7 +58,6 @@ unification_set(SetID, ChildNode) :- attr("depends_on", ParentNode, ChildNode, T
 unification_set(("build", node(X, Child)), node(X, Child))
   :- attr("depends_on", ParentNode, node(X, Child), Type),
      Type == "build",
-     SetID != "generic_build",
      multiple_unification_sets(Child),
      unification_set(SetID, ParentNode).
 
@@ -68,18 +67,18 @@ unification_set("generic_build", node(X, Child))
      not multiple_unification_sets(Child),
      unification_set(_, ParentNode).
 
-% Any dependency of type "build" in a unification set that is in the leaf unification set,
-% stays in that unification set
-unification_set(SetID, ChildNode)
-  :- attr("depends_on", ParentNode, ChildNode, Type),
-     Type == "build",
-     SetID == "generic_build",
-     unification_set(SetID, ParentNode).
-
 unification_set(SetID, VirtualNode)
   :- provider(PackageNode, VirtualNode),
      unification_set(SetID, PackageNode).
 
+% Do not allow split dependencies, for now. This ensures that we don't construct graphs where e.g.
+% a python extension depends on setuptools@63.4 as a run dependency, but uses e.g. setuptools@68
+% as a build dependency.
+%
+% We'll need to relax the rule before we get to actual cross-compilation
+:- depends_on(ParentNode, node(X, Dependency)), depends_on(ParentNode, node(Y, Dependency)), X < Y.
+
+
 #defined multiple_unification_sets/1.
 
 %----

lib/spack/spack/solver/counter.py

@@ -5,6 +5,8 @@
 import collections
 from typing import List, Set
 
+from llnl.util import lang
+
 import spack.deptypes as dt
 import spack.package_base
 import spack.repo
@@ -95,8 +97,17 @@ def _compute_cache_values(self):
         )
         self._link_run_virtuals.update(self._possible_virtuals)
         for x in self._link_run:
-            current = spack.repo.PATH.get_pkg_class(x).dependencies_of_type(dt.BUILD)
-            self._direct_build.update(current)
+            build_dependencies = spack.repo.PATH.get_pkg_class(x).dependencies_of_type(dt.BUILD)
+            virtuals, reals = lang.stable_partition(
+                build_dependencies, spack.repo.PATH.is_virtual_safe
+            )
+
+            self._possible_virtuals.update(virtuals)
+            for virtual_dep in virtuals:
+                providers = spack.repo.PATH.providers_for(virtual_dep)
+                self._direct_build.update(str(x) for x in providers)
+
+            self._direct_build.update(reals)
 
         self._total_build = set(
             spack.package_base.possible_dependencies(

lib/spack/spack/spec.py

@@ -1604,13 +1604,20 @@ def _add_dependency(self, spec: "Spec", *, depflag: dt.DepFlag, virtuals: Tuple[
         try:
             dspec = next(dspec for dspec in orig if depflag == dspec.depflag)
         except StopIteration:
-            raise DuplicateDependencyError("Cannot depend on '%s' twice" % spec)
+            current_deps = ", ".join(
+                dt.flag_to_chars(x.depflag) + " " + x.spec.short_spec for x in orig
+            )
+            raise DuplicateDependencyError(
+                f"{self.short_spec} cannot depend on '{spec.short_spec}' multiple times.\n"
+                f"\tRequired: {dt.flag_to_chars(depflag)}\n"
+                f"\tDependency: {current_deps}"
+            )
 
         try:
             dspec.spec.constrain(spec)
         except spack.error.UnsatisfiableSpecError:
             raise DuplicateDependencyError(
-                "Cannot depend on incompatible specs '%s' and '%s'" % (dspec.spec, spec)
+                f"Cannot depend on incompatible specs '{dspec.spec}' and '{spec}'"
             )
 
     def add_dependency_edge(

lib/spack/spack/test/concretize.py

@@ -2121,12 +2121,9 @@ def duplicates_test_repository():
 
 
 @pytest.mark.usefixtures("mutable_config", "duplicates_test_repository")
+@pytest.mark.only_clingo("Not supported by the original concretizer")
 class TestConcretizeSeparately:
     @pytest.mark.parametrize("strategy", ["minimal", "full"])
-    @pytest.mark.skipif(
-        os.environ.get("SPACK_TEST_SOLVER") == "original",
-        reason="Not supported by the original concretizer",
-    )
     def test_two_gmake(self, strategy):
         """Tests that we can concretize a spec with nodes using the same build
         dependency pinned at different versions.
@@ -2151,10 +2148,6 @@ def test_two_gmake(self, strategy):
         assert len(pinned_gmake) == 1 and pinned_gmake[0].satisfies("@=3.0")
 
     @pytest.mark.parametrize("strategy", ["minimal", "full"])
-    @pytest.mark.skipif(
-        os.environ.get("SPACK_TEST_SOLVER") == "original",
-        reason="Not supported by the original concretizer",
-    )
     def test_two_setuptools(self, strategy):
         """Tests that we can concretize separate build dependencies, when we are dealing
         with extensions.
@@ -2191,10 +2184,6 @@ def test_two_setuptools(self, strategy):
         gmake = s["python"].dependencies(name="gmake", deptype="build")
         assert len(gmake) == 1 and gmake[0].satisfies("@=3.0")
 
-    @pytest.mark.skipif(
-        os.environ.get("SPACK_TEST_SOLVER") == "original",
-        reason="Not supported by the original concretizer",
-    )
     def test_solution_without_cycles(self):
         """Tests that when we concretize a spec with cycles, a fallback kicks in to recompute
         a solution without cycles.
@@ -2207,6 +2196,21 @@ def test_solution_without_cycles(self):
         assert s["cycle-a"].satisfies("~cycle")
         assert s["cycle-b"].satisfies("+cycle")
 
+    @pytest.mark.parametrize("strategy", ["minimal", "full"])
+    def test_pure_build_virtual_dependency(self, strategy):
+        """Tests that we can concretize a pure build virtual dependency, and ensures that
+        pure build virtual dependencies are accounted in the list of possible virtual
+        dependencies.
+
+        virtual-build@1.0
+        | [type=build, virtual=pkgconfig]
+        pkg-config@1.0
+        """
+        spack.config.CONFIG.set("concretizer:duplicates:strategy", strategy)
+
+        s = Spec("virtual-build").concretized()
+        assert s["pkgconfig"].name == "pkg-config"
+
 
 @pytest.mark.parametrize(
     "v_str,v_opts,checksummed",

lib/spack/spack/test/data/config/concretizer.yaml

@@ -4,4 +4,4 @@ concretizer:
     granularity: microarchitectures
     host_compatible: false
   duplicates:
-    strategy: none
+    strategy: minimal

share/spack/gitlab/cloud_pipelines/stacks/data-vis-sdk/spack.yaml

@@ -4,22 +4,16 @@ spack:
     cmake:
       variants: ~ownlibs
     ecp-data-vis-sdk:
-      require:
-      - one_of:
-        - +ascent +adios2 +cinema +darshan +faodel +hdf5 +pnetcdf +sensei +sz +unifyfs
-          +veloc +vtkm +zfp
-      - one_of:
-        - +paraview ~visit
-        - ~paraview +visit
+      require: "+ascent +adios2 +cinema +darshan +faodel +hdf5 +pnetcdf +sensei +sz +unifyfs +veloc +vtkm +zfp"
     hdf5:
       require:
       - one_of: ['@1.14', '@1.12']
     mesa:
-      require: +glx +osmesa +opengl ~opengles +llvm
+      require: "+glx +osmesa +opengl ~opengles +llvm"
     libosmesa:
-      require: mesa +osmesa
+      require: "mesa +osmesa"
     libglx:
-      require: mesa +glx
+      require: "mesa +glx"
     ospray:
       require: '@2.8.0 +denoiser +mpi'
     llvm:
@@ -57,9 +51,11 @@ spack:
     # Test ParaView and VisIt builds with different GL backends
   - matrix:
     - [$sdk_base_spec]
+    - ["+paraview ~visit"]
     - [$^paraview_specs]
   - matrix:
     - [$sdk_base_spec]
+    - ["~paraview +visit"]
     - [$^visit_specs]
 
   mirrors: {mirror: s3://spack-binaries/develop/data-vis-sdk}

var/spack/repos/builtin/packages/camp/package.py

@@ -53,7 +53,7 @@ class Camp(CMakePackage, CudaPackage, ROCmPackage):
 
     depends_on("cub", when="+cuda")
 
-    depends_on("blt")
+    depends_on("blt", type="build")
 
     conflicts("^blt@:0.3.6", when="+rocm")
 

var/spack/repos/builtin/packages/cmake/package.py

@@ -239,7 +239,7 @@ class Cmake(Package):
         depends_on("libuv@1.10.0:1.10", when="@3.11.0:3.11")
         depends_on("libuv@1.10.0:", when="@3.12.0:")
         depends_on("rhash", when="@3.8.0:")
-        depends_on("jsoncpp", when="@3.2:")
+        depends_on("jsoncpp build_system=meson", when="@3.2:")
 
     depends_on("ncurses", when="+ncurses")
 

var/spack/repos/builtin/packages/jsoncpp/package.py

@@ -47,7 +47,7 @@ class Jsoncpp(CMakePackage, MesonPackage):
 
     with when("build_system=cmake"):
         depends_on("cmake@3.1:", type="build")
-        depends_on("cmake@1.9:", when="@1.9:", type="build")
+        depends_on("cmake@3.9:", when="@1.9:", type="build")
 
     with when("build_system=meson"):
         depends_on("meson@0.49.0:", type="build")

var/spack/repos/builtin/packages/mgard/package.py

@@ -50,7 +50,7 @@ class Mgard(CMakePackage, CudaPackage):
     depends_on("libarchive", when="@2021-11-12:")
     depends_on("tclap", when="@2021-11-12")
     depends_on("yaml-cpp", when="@2021-11-12:")
-    depends_on("cmake@3.19:")
+    depends_on("cmake@3.19:", type="build")
     depends_on("nvcomp@2.2.0:", when="@2022-11-18:+cuda")
     depends_on("nvcomp@2.0.2", when="@:2021-11-12+cuda")
     conflicts("cuda_arch=none", when="+cuda")

var/spack/repos/builtin/packages/py-gevent/package.py

@@ -24,8 +24,7 @@ class PyGevent(PythonPackage):
     depends_on("py-setuptools@40.8:", when="@20.5.1:", type=("build", "run"))
     depends_on("py-setuptools@40.8:", when="@1.5:", type="build")
     depends_on("py-setuptools@24.2:", when="@:1.4", type="build")
-    # TODO: relax this until we support separate concretization of build deps by default
-    # depends_on("py-cython@3:", when="@20.5.1:", type="build")
+    depends_on("py-cython@3:", when="@20.5.1:", type="build")
     depends_on("py-cython@0.29.14:", when="@1.5:", type="build")
     depends_on("py-cffi@1.12.3:", type=("build", "run"))
     depends_on("py-greenlet@3:", when="@23.7: ^python@3.12:", type=("build", "run"))

var/spack/repos/builtin/packages/py-statsmodels/package.py

@@ -14,6 +14,7 @@ class PyStatsmodels(PythonPackage):
     homepage = "https://www.statsmodels.org"
     pypi = "statsmodels/statsmodels-0.8.0.tar.gz"
 
+    version("0.13.5", sha256="593526acae1c0fda0ea6c48439f67c3943094c542fe769f8b90fe9e6c6cc4871")
     version("0.13.2", sha256="77dc292c9939c036a476f1770f9d08976b05437daa229928da73231147cde7d4")
     version("0.13.1", sha256="006ec8d896d238873af8178d5475203844f2c391194ed8d42ddac37f5ff77a69")
     version("0.13.0", sha256="f2efc02011b7240a9e851acd76ab81150a07d35c97021cb0517887539a328f8a")
@@ -25,12 +26,15 @@ class PyStatsmodels(PythonPackage):
     depends_on("python@2.7:2.8,3.4:", when="@0.10.1:", type=("build", "link", "run"))
     depends_on("python@3.6:", when="@0.12.1:", type=("build", "link", "run"))
 
-    # according to https://www.statsmodels.org/dev/install.html earlier versions
-    # might work.
-    depends_on("py-setuptools@0.6c5:", type="build")
-    depends_on("py-cython@0.29:", type="build")
+    # according to https://www.statsmodels.org/dev/install.html earlier versions might work.
+    depends_on("py-setuptools", type="build")
+    depends_on("py-setuptools@59.2.0:", type="build", when="@0.13.5:")
+
+    # https://github.com/statsmodels/statsmodels/blob/01b19d7d111b29c183f620ff0a949ef6391ff8ee/pyproject.toml
+    depends_on("py-cython@0", type="build")
     depends_on("py-cython@0.29.14:", type="build", when="@0.12.0:")
     depends_on("py-cython@0.29.22:", type="build", when="@0.13.0:")
+    depends_on("py-cython@0.29.32:", type="build", when="@0.13.5:")
 
     # patsy@0.5.1 works around a Python change
     #    https://github.com/statsmodels/statsmodels/issues/5343 and

var/spack/repos/builtin/packages/raja/package.py

@@ -137,7 +137,7 @@ class Raja(CachedCMakePackage, CudaPackage, ROCmPackage):
 
     depends_on("cmake@:3.20", when="@:2022.03+rocm", type="build")
     depends_on("cmake@3.23:", when="@2022.10:+rocm", type="build")
-    depends_on("cmake@3.14:", when="@2022.03.0:")
+    depends_on("cmake@3.14:", when="@2022.03.0:", type="build")
 
     depends_on("llvm-openmp", when="+openmp %apple-clang")
 

var/spack/repos/builtin/packages/umpire/package.py

@@ -168,7 +168,7 @@ class Umpire(CachedCMakePackage, CudaPackage, ROCmPackage):
 
     depends_on("cmake@3.8:", type="build")
     depends_on("cmake@3.9:", when="+cuda", type="build")
-    depends_on("cmake@3.14:", when="@2022.03.0:")
+    depends_on("cmake@3.14:", when="@2022.03.0:", type="build")
 
     depends_on("blt@0.5.2:", type="build", when="@2022.10.0:")
     depends_on("blt@0.5.0:", type="build", when="@2022.03.0:")

var/spack/repos/duplicates.test/packages/pkg-config/package.py

@@ -0,0 +1,16 @@
+# Copyright 2013-2023 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+from spack.package import *
+
+
+class PkgConfig(Package):
+    """A package providing a virtual, which is frequently used as a pure build dependency."""
+
+    homepage = "http://www.example.com"
+    url = "http://www.example.com/tdep-1.0.tar.gz"
+
+    version("1.0.0", md5="0123456789abcdef0123456789abcdef")
+
+    provides("pkgconfig")

var/spack/repos/duplicates.test/packages/virtual-build/package.py

@@ -0,0 +1,16 @@
+# Copyright 2013-2023 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+from spack.package import *
+
+
+class VirtualBuild(Package):
+    """A package that has a pure build virtual dependency"""
+
+    homepage = "http://www.example.com"
+    url = "http://www.example.com/tdep-1.0.tar.gz"
+
+    version("1.0.0", md5="0123456789abcdef0123456789abcdef")
+
+    depends_on("pkgconfig", type="build")