From b1f896e6c75560cdb4f315cd8e68a653d085db63 Mon Sep 17 00:00:00 2001 From: Jordan Galby <67924449+Jordan474@users.noreply.github.com> Date: Fri, 4 Nov 2022 10:05:22 +0100 Subject: 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 | 57 ++++++++++++++++++--------------- lib/spack/spack/test/make_executable.py | 38 +++++++++++++++------- 2 files changed, 58 insertions(+), 37 deletions(-) (limited to 'lib') diff --git a/lib/spack/spack/build_environment.py b/lib/spack/spack/build_environment.py index 1496e9359f..c69dd566e6 100644 --- a/lib/spack/spack/build_environment.py +++ b/lib/spack/spack/build_environment.py @@ -121,18 +121,18 @@ else: stat_suffix = "lib" if sys.platform == "win32" else "a" -def should_set_parallel_jobs(jobserver_support=False): - """Returns true in general, except when: - - The env variable SPACK_NO_PARALLEL_MAKE=1 is set - - jobserver_support is enabled, and a jobserver was found. - """ - if ( - jobserver_support - and "MAKEFLAGS" in os.environ - and "--jobserver" in os.environ["MAKEFLAGS"] - ): - return False - return not env_flag(SPACK_NO_PARALLEL_MAKE) +def jobserver_enabled(): + """Returns true if a posix jobserver (make) is detected.""" + return "MAKEFLAGS" in os.environ and "--jobserver" in os.environ["MAKEFLAGS"] + + +def get_effective_jobs(jobs, parallel=True, supports_jobserver=False): + """Return the number of jobs, or None if supports_jobserver and a jobserver is detected.""" + if not parallel or jobs <= 1 or env_flag(SPACK_NO_PARALLEL_MAKE): + return 1 + if supports_jobserver and jobserver_enabled(): + return None + 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, - # 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 - ) + parallel = kwargs.pop("parallel", True) + jobs_env = kwargs.pop("jobs_env", None) + jobs_env_supports_jobserver = kwargs.pop("jobs_env_supports_jobserver", False) - if parallel: - args = ("-j{0}".format(self.jobs),) + args - jobs_env = kwargs.pop("jobs_env", None) - if jobs_env: - # Caller wants us to set an environment variable to - # control the parallelism. - kwargs["extra_env"] = {jobs_env: str(self.jobs)} + 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. + 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 diff --git a/lib/spack/spack/test/make_executable.py b/lib/spack/spack/test/make_executable.py index b7063e5e10..efaff04de5 100644 --- a/lib/spack/spack/test/make_executable.py +++ b/lib/spack/spack/test/make_executable.py @@ -53,24 +53,24 @@ class MakeExecutableTest(unittest.TestCase): def test_make_one_job(self): make = MakeExecutable("make", 1) - self.assertEqual(make(output=str).strip(), "") - self.assertEqual(make("install", output=str).strip(), "install") + self.assertEqual(make(output=str).strip(), "-j1") + 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("install", parallel=False, output=str).strip(), "install") + self.assertEqual(make(parallel=False, output=str).strip(), "-j1") + 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("install", output=str).strip(), "install") + self.assertEqual(make(output=str).strip(), "-j1") + 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("install", output=str).strip(), "install") + self.assertEqual(make(output=str).strip(), "-j1") + 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 @@ class MakeExecutableTest(unittest.TestCase): # These should work os.environ["SPACK_NO_PARALLEL_MAKE"] = "true" - self.assertEqual(make(parallel=True, output=str).strip(), "") - self.assertEqual(make("install", parallel=True, output=str).strip(), "install") + self.assertEqual(make(parallel=True, output=str).strip(), "-j1") + 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("install", parallel=True, output=str).strip(), "install") + self.assertEqual(make(parallel=True, output=str).strip(), "-j1") + 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 @@ class MakeExecutableTest(unittest.TestCase): 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"] -- cgit v1.2.3-70-g09d2