Fix non-parallel make under depfile jobserver (#32862)

When a package asks for non-parallel make, we need to force `make -j1` because just doing `make` will run in parallel under jobserver (e.g. `spack env depfile`).

We now always add `-j1` when asked for a non-parallel execution (even if there is no jobserver).

And each `MakeExecutable` can now ask for jobserver support or not. For example: the default `ninja` does not support jobserver so spack applies the default `-j`, but `ninja@kitware` or `ninja-fortran` does, so spack doesn't add `-j`.

Tips: you can run `SPACK_INSTALL_FLAGS=-j1 make -f spack-env-depfile.make -j8` to avoid massive job-spawning because of build tools that don't support jobserver (ninja).

lib/spack/spack/build_environment.py

@@ -121,18 +121,18 @@
 stat_suffix = "lib" if sys.platform == "win32" else "a"
 
 
-def should_set_parallel_jobs(jobserver_support=False):
+def jobserver_enabled():
-    """Returns true in general, except when:
+    """Returns true if a posix jobserver (make) is detected."""
-    - The env variable SPACK_NO_PARALLEL_MAKE=1 is set
+    return "MAKEFLAGS" in os.environ and "--jobserver" in os.environ["MAKEFLAGS"]
-    - jobserver_support is enabled, and a jobserver was found.
+
-    """
+
-    if (
+def get_effective_jobs(jobs, parallel=True, supports_jobserver=False):
-        jobserver_support
+    """Return the number of jobs, or None if supports_jobserver and a jobserver is detected."""
-        and "MAKEFLAGS" in os.environ
+    if not parallel or jobs <= 1 or env_flag(SPACK_NO_PARALLEL_MAKE):
-        and "--jobserver" in os.environ["MAKEFLAGS"]
+        return 1
-    ):
+    if supports_jobserver and jobserver_enabled():
-        return False
+        return None
-    return not env_flag(SPACK_NO_PARALLEL_MAKE)
+    return jobs
 
 
 class MakeExecutable(Executable):
@@ -147,26 +147,33 @@ class MakeExecutable(Executable):
     """
 
     def __init__(self, name, jobs, **kwargs):
+        supports_jobserver = kwargs.pop("supports_jobserver", True)
         super(MakeExecutable, self).__init__(name, **kwargs)
+        self.supports_jobserver = supports_jobserver
         self.jobs = jobs
 
     def __call__(self, *args, **kwargs):
         """parallel, and jobs_env from kwargs are swallowed and used here;
         remaining arguments are passed through to the superclass.
         """
-        # TODO: figure out how to check if we are using a jobserver-supporting ninja,
+        parallel = kwargs.pop("parallel", True)
-        # the two split ninja packages make this very difficult right now
-        parallel = should_set_parallel_jobs(jobserver_support=True) and kwargs.pop(
-            "parallel", self.jobs > 1
-        )
-
-        if parallel:
-            args = ("-j{0}".format(self.jobs),) + args
         jobs_env = kwargs.pop("jobs_env", None)
+        jobs_env_supports_jobserver = kwargs.pop("jobs_env_supports_jobserver", False)
+
+        jobs = get_effective_jobs(
+            self.jobs, parallel=parallel, supports_jobserver=self.supports_jobserver
+        )
+        if jobs is not None:
+            args = ("-j{0}".format(jobs),) + args
+
         if jobs_env:
             # Caller wants us to set an environment variable to
             # control the parallelism.
-                kwargs["extra_env"] = {jobs_env: str(self.jobs)}
+            jobs_env_jobs = get_effective_jobs(
+                self.jobs, parallel=parallel, supports_jobserver=jobs_env_supports_jobserver
+            )
+            if jobs_env_jobs is not None:
+                kwargs["extra_env"] = {jobs_env: str(jobs_env_jobs)}
 
         return super(MakeExecutable, self).__call__(*args, **kwargs)
 
@@ -547,7 +554,7 @@ def _set_variables_for_single_module(pkg, module):
     # TODO: make these build deps that can be installed if not found.
     m.make = MakeExecutable("make", jobs)
     m.gmake = MakeExecutable("gmake", jobs)
-    m.ninja = MakeExecutable("ninja", jobs)
+    m.ninja = MakeExecutable("ninja", jobs, supports_jobserver=False)
 
     # easy shortcut to os.environ
     m.env = os.environ

lib/spack/spack/test/make_executable.py

@@ -53,24 +53,24 @@ def test_make_explicit(self):
 
     def test_make_one_job(self):
         make = MakeExecutable("make", 1)
-        self.assertEqual(make(output=str).strip(), "")
+        self.assertEqual(make(output=str).strip(), "-j1")
-        self.assertEqual(make("install", output=str).strip(), "install")
+        self.assertEqual(make("install", output=str).strip(), "-j1 install")
 
     def test_make_parallel_false(self):
         make = MakeExecutable("make", 8)
-        self.assertEqual(make(parallel=False, output=str).strip(), "")
+        self.assertEqual(make(parallel=False, output=str).strip(), "-j1")
-        self.assertEqual(make("install", parallel=False, output=str).strip(), "install")
+        self.assertEqual(make("install", parallel=False, output=str).strip(), "-j1 install")
 
     def test_make_parallel_disabled(self):
         make = MakeExecutable("make", 8)
 
         os.environ["SPACK_NO_PARALLEL_MAKE"] = "true"
-        self.assertEqual(make(output=str).strip(), "")
+        self.assertEqual(make(output=str).strip(), "-j1")
-        self.assertEqual(make("install", output=str).strip(), "install")
+        self.assertEqual(make("install", output=str).strip(), "-j1 install")
 
         os.environ["SPACK_NO_PARALLEL_MAKE"] = "1"
-        self.assertEqual(make(output=str).strip(), "")
+        self.assertEqual(make(output=str).strip(), "-j1")
-        self.assertEqual(make("install", output=str).strip(), "install")
+        self.assertEqual(make("install", output=str).strip(), "-j1 install")
 
         # These don't disable (false and random string)
         os.environ["SPACK_NO_PARALLEL_MAKE"] = "false"
@@ -88,12 +88,12 @@ def test_make_parallel_precedence(self):
 
         # These should work
         os.environ["SPACK_NO_PARALLEL_MAKE"] = "true"
-        self.assertEqual(make(parallel=True, output=str).strip(), "")
+        self.assertEqual(make(parallel=True, output=str).strip(), "-j1")
-        self.assertEqual(make("install", parallel=True, output=str).strip(), "install")
+        self.assertEqual(make("install", parallel=True, output=str).strip(), "-j1 install")
 
         os.environ["SPACK_NO_PARALLEL_MAKE"] = "1"
-        self.assertEqual(make(parallel=True, output=str).strip(), "")
+        self.assertEqual(make(parallel=True, output=str).strip(), "-j1")
-        self.assertEqual(make("install", parallel=True, output=str).strip(), "install")
+        self.assertEqual(make("install", parallel=True, output=str).strip(), "-j1 install")
 
         # These don't disable (false and random string)
         os.environ["SPACK_NO_PARALLEL_MAKE"] = "false"
@@ -113,3 +113,17 @@ def test_make_jobs_env(self):
             make(output=str, jobs_env="MAKE_PARALLELISM", _dump_env=dump_env).strip(), "-j8"
         )
         self.assertEqual(dump_env["MAKE_PARALLELISM"], "8")
+
+    def test_make_jobserver(self):
+        make = MakeExecutable("make", 8)
+        os.environ["MAKEFLAGS"] = "--jobserver-auth=X,Y"
+        self.assertEqual(make(output=str).strip(), "")
+        self.assertEqual(make(parallel=False, output=str).strip(), "-j1")
+        del os.environ["MAKEFLAGS"]
+
+    def test_make_jobserver_not_supported(self):
+        make = MakeExecutable("make", 8, supports_jobserver=False)
+        os.environ["MAKEFLAGS"] = "--jobserver-auth=X,Y"
+        # Currently fallback on default job count, Maybe it should force -j1 ?
+        self.assertEqual(make(output=str).strip(), "-j8")
+        del os.environ["MAKEFLAGS"]

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

@@ -307,11 +307,20 @@ def bootstrap_args(self):
         args = []
         self.generator = make
 
+        if self.spec.satisfies("platform=windows"):
+            args.append("-GNinja")
+            self.generator = ninja
+
         if not sys.platform == "win32":
             args.append("--prefix={0}".format(self.prefix))
 
-            if spack.build_environment.should_set_parallel_jobs(jobserver_support=True):
+            jobs = spack.build_environment.get_effective_jobs(
-                args.append("--parallel={0}".format(make_jobs))
+                make_jobs,
+                parallel=self.parallel,
+                supports_jobserver=self.generator.supports_jobserver,
+            )
+            if jobs is not None:
+                args.append("--parallel={0}".format(jobs))
 
             if "+ownlibs" in spec:
                 # Build and link to the CMake-provided third-party libraries
@@ -338,9 +347,7 @@ def bootstrap_args(self):
             args.append("--")
         else:
             args.append("-DCMAKE_INSTALL_PREFIX=%s" % self.prefix)
-        if self.spec.satisfies("platform=windows"):
+
-            args.append("-GNinja")
-            self.generator = ninja
         args.append("-DCMAKE_BUILD_TYPE={0}".format(self.spec.variants["build_type"].value))
 
         # Install CMake correctly, even if `spack install` runs

var/spack/repos/builtin/packages/ninja-fortran/package.py

@@ -86,3 +86,12 @@ def install(self, spec, prefix):
         # instead of 'ninja'. Install both for uniformity.
         with working_dir(prefix.bin):
             symlink("ninja", "ninja-build")
+
+    def setup_dependent_package(self, module, dspec):
+        name = "ninja"
+
+        module.ninja = MakeExecutable(
+            which_string(name, path=[self.spec.prefix.bin], required=True),
+            determine_number_of_jobs(parallel=self.parallel),
+            supports_jobserver=True,  # This fork supports jobserver
+        )

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

@@ -79,4 +79,5 @@ def setup_dependent_package(self, module, dspec):
         module.ninja = MakeExecutable(
             which_string(name, path=[self.spec.prefix.bin], required=True),
             determine_number_of_jobs(parallel=self.parallel),
+            supports_jobserver=self.spec.version == ver("kitware"),
         )