From 5bd7ad2b225bf9229ca96ba63b90f8094a2becde Mon Sep 17 00:00:00 2001
From: Paul Smith <psmith@gnu.org>
Date: Mon, 4 Apr 2016 01:23:04 -0400
Subject: [PATCH] Preserve the real value of -jN in MAKEFLAGS using jobserver.
Previously if the jobserver was active, MAKEFLAGS would contain only
the -j option but not the number (not -j5 or whatever) so users
could not discover that value. Allow that value to be provided in
MAKEFLAGS without error but still give warnings if -jN is provided
on the command line if the jobserver is already activated.
* NEWS: Discuss the new behavior.
* os.h, posixos.c, w32/w32os.c: Return success/failure from
jobserver_setup() and jobserver_parse_auth().
* main.c (main): Separate the command line storage of job slots (now
in arg_job_slots) from the control storage (in job_slots). Make a
distinction between -jN flags read from MAKEFLAGS and those seen
on the command line: for the latter if the jobserver is enabled then
warn and disable it, as before.
* tests/scripts/features/jobserver: Add new testing.
---
NEWS | 4 +
doc/make.texi | 6 +-
main.c | 167 ++++++++++++++++-------------
os.h | 10 +-
posixos.c | 26 +++--
tests/scripts/features/jobserver | 46 +++++++-
tests/scripts/features/output-sync | 8 +-
tests/scripts/variables/MAKEFLAGS | 4 +
w32/w32os.c | 16 +--
9 files changed, 179 insertions(+), 108 deletions(-)
diff --git a/NEWS b/NEWS
index c2475e3c..4a38e4fe 100644
--- a/NEWS
+++ b/NEWS
@@ -30,6 +30,10 @@ http://sv.gnu.org/bugs/index.php?group=make&report_id=111&fix_release_id=106&set
WARNING: Backward-incompatibility! The internal-only command line option
--jobserver-fds has been renamed for publishing, to --jobserver-auth.
+* The amount of parallelism can be determined by querying MAKEFLAGS, even when
+ the job server is enabled (previously MAKEFLAGS would always contain only
+ "-j" when job server was enabled).
+
* VMS-specific changes:
* Perl test harness now works.
diff --git a/doc/make.texi b/doc/make.texi
index b4ce44ae..2dc907a3 100644
--- a/doc/make.texi
+++ b/doc/make.texi
@@ -4721,9 +4721,9 @@ doesn't count against the total jobs (otherwise we could get @samp{N}
sub-@code{make}s running and have no slots left over for any real work!)
If your operating system doesn't support the above communication, then
-@samp{-j 1} is always put into @code{MAKEFLAGS} instead of the value you
-specified. This is because if the @w{@samp{-j}} option were passed down
-to sub-@code{make}s, you would get many more jobs running in parallel
+no @samp{-j} is added to @code{MAKEFLAGS}, so that sub-@code{make}s
+run in non-parallel mode. If the @w{@samp{-j}} option were passed down
+to sub-@code{make}s you would get many more jobs running in parallel
than you asked for. If you give @samp{-j} with no numeric argument,
meaning to run as many jobs as possible in parallel, this is passed
down, since multiple infinities are no more than one.@refill
diff --git a/main.c b/main.c
index bf8fb0dc..ee08720f 100644
--- a/main.c
+++ b/main.c
@@ -259,19 +259,23 @@ struct rlimit stack_limit;
#endif
-/* Number of job slots (commands that can be run at once). */
+/* Number of job slots for parallelism. */
-unsigned int job_slots = 1;
-unsigned int default_job_slots = 1;
+unsigned int job_slots;
+
+#define INVALID_JOB_SLOTS (-1)
static unsigned int master_job_slots = 0;
+static int arg_job_slots = INVALID_JOB_SLOTS;
+
+static const int default_job_slots = INVALID_JOB_SLOTS;
/* Value of job_slots that means no limit. */
-static unsigned int inf_jobs = 0;
+static const int inf_jobs = 0;
/* Authorization for the jobserver. */
-char *jobserver_auth = NULL;
+static char *jobserver_auth = NULL;
/* Handle for the mutex used on Windows to synchronize output of our
children under -O. */
@@ -456,7 +460,7 @@ static const struct command_switch switches[] =
{ 'f', filename, &makefiles, 0, 0, 0, 0, 0, "file" },
{ 'I', filename, &include_directories, 1, 1, 0, 0, 0,
"include-dir" },
- { 'j', positive_int, &job_slots, 1, 1, 0, &inf_jobs, &default_job_slots,
+ { 'j', positive_int, &arg_job_slots, 1, 1, 0, &inf_jobs, &default_job_slots,
"jobs" },
#ifndef NO_FLOAT
{ 'l', floating, &max_load_average, 1, 1, 0, &default_load_average,
@@ -1057,8 +1061,13 @@ msdos_return_to_initial_directory (void)
}
#endif /* __MSDOS__ */
-
-
+static void
+reset_jobserver ()
+{
+ jobserver_clear ();
+ free (jobserver_auth);
+ jobserver_auth = NULL;
+}
#ifdef _AMIGA
int
@@ -1074,6 +1083,7 @@ main (int argc, char **argv, char **envp)
PATH_VAR (current_directory);
unsigned int restarts = 0;
unsigned int syncing = 0;
+ int argv_slots;
#ifdef WINDOWS32
const char *unix_path = NULL;
const char *windows32_path = NULL;
@@ -1483,24 +1493,34 @@ main (int argc, char **argv, char **envp)
|| output_sync == OUTPUT_SYNC_TARGET);
OUTPUT_SET (&make_sync);
- decode_switches (argc, (const char **)argv, 0);
+ /* Remember the job slots set through the environment vs. command line. */
+ {
+ int env_slots = arg_job_slots;
+ arg_job_slots = INVALID_JOB_SLOTS;
- /* Set a variable specifying whether stdout/stdin is hooked to a TTY. */
+ decode_switches (argc, (const char **)argv, 0);
+ argv_slots = arg_job_slots;
+
+ if (arg_job_slots == INVALID_JOB_SLOTS)
+ arg_job_slots = env_slots;
+ }
+
+ /* Set a variable specifying whether stdout/stdin is hooked to a TTY. */
#ifdef HAVE_ISATTY
- if (isatty (fileno (stdout)))
- if (! lookup_variable (STRING_SIZE_TUPLE ("MAKE_TERMOUT")))
- {
- const char *tty = TTYNAME (fileno (stdout));
- define_variable_cname ("MAKE_TERMOUT", tty ? tty : DEFAULT_TTYNAME,
- o_default, 0)->export = v_export;
- }
- if (isatty (fileno (stderr)))
- if (! lookup_variable (STRING_SIZE_TUPLE ("MAKE_TERMERR")))
- {
- const char *tty = TTYNAME (fileno (stderr));
- define_variable_cname ("MAKE_TERMERR", tty ? tty : DEFAULT_TTYNAME,
- o_default, 0)->export = v_export;
- }
+ if (isatty (fileno (stdout)))
+ if (! lookup_variable (STRING_SIZE_TUPLE ("MAKE_TERMOUT")))
+ {
+ const char *tty = TTYNAME (fileno (stdout));
+ define_variable_cname ("MAKE_TERMOUT", tty ? tty : DEFAULT_TTYNAME,
+ o_default, 0)->export = v_export;
+ }
+ if (isatty (fileno (stderr)))
+ if (! lookup_variable (STRING_SIZE_TUPLE ("MAKE_TERMERR")))
+ {
+ const char *tty = TTYNAME (fileno (stderr));
+ define_variable_cname ("MAKE_TERMERR", tty ? tty : DEFAULT_TTYNAME,
+ o_default, 0)->export = v_export;
+ }
#endif
/* Reset in case the switches changed our minds. */
@@ -1600,37 +1620,43 @@ main (int argc, char **argv, char **envp)
/* We may move, but until we do, here we are. */
starting_directory = current_directory;
-#ifdef MAKE_JOBSERVER
- /* If the jobserver_auth option is seen, make sure that -j is reasonable.
- This can't be usefully set in the makefile, and we want to verify the
- authorization is valid before any other aspect of make has a chance to
- start using it for something else. */
+ /* Set up the job_slots value and the jobserver. This can't be usefully set
+ in the makefile, and we want to verify the authorization is valid before
+ make has a chance to start using it for something else. */
if (jobserver_auth)
{
- /* The combination of jobserver_auth and !job_slots means we're using
- the jobserver. If !job_slots and no jobserver_auth, we can start
- infinite jobs. If we see both jobserver_auth and job_slots >0 that
- means the user set -j explicitly. This is broken; in this case obey
- the user (ignore the jobserver for this make) but print a message.
- If we've restarted, we already printed this the first time. */
+ if (argv_slots == INVALID_JOB_SLOTS)
+ {
+ if (jobserver_parse_auth (jobserver_auth))
+ {
+ /* Success! Use the jobserver. */
+ job_slots = 0;
+ goto job_setup_complete;
+ }
- if (!job_slots)
- jobserver_parse_auth (jobserver_auth);
+ O (error, NILF, _("warning: jobserver unavailable: using -j1. Add '+' to parent make rule."));
+ arg_job_slots = 1;
+ }
- else if (! restarts)
+ /* The user provided a -j setting on the command line: use it. */
+ else if (!restarts)
+ /* If restarts is >0 we already printed this message. */
O (error, NILF,
_("warning: -jN forced in submake: disabling jobserver mode."));
- if (job_slots > 0)
- {
- /* If job_slots is set now then we're not using jobserver */
- jobserver_clear ();
- free (jobserver_auth);
- jobserver_auth = NULL;
- }
+ /* We failed to use our parent's jobserver. */
+ reset_jobserver ();
+ job_slots = (unsigned int)arg_job_slots;
}
-#endif
+ else if (arg_job_slots == INVALID_JOB_SLOTS)
+ /* The default is one job at a time. */
+ job_slots = 1;
+ else
+ /* Use whatever was provided. */
+ job_slots = (unsigned int)arg_job_slots;
+
+ job_setup_complete:
/* The extra indirection through $(MAKE_COMMAND) is done
for hysterical raisins. */
@@ -2024,7 +2050,7 @@ main (int argc, char **argv, char **envp)
}
#if defined (__MSDOS__) || defined (__EMX__) || defined (VMS)
- if (job_slots != 1
+ if (arg_job_slots != 1
# ifdef __EMX__
&& _osmode != OS2_MODE /* turn off -j if we are in DOS mode */
# endif
@@ -2033,30 +2059,29 @@ main (int argc, char **argv, char **envp)
O (error, NILF,
_("Parallel jobs (-j) are not supported on this platform."));
O (error, NILF, _("Resetting to single job (-j1) mode."));
- job_slots = 1;
+ arg_job_slots = job_slots = 1;
}
#endif
-#ifdef MAKE_JOBSERVER
- if (job_slots > 1)
+ /* If we have >1 slot at this point, then we're a top-level make.
+ Set up the jobserver.
+
+ Every make assumes that it always has one job it can run. For the
+ submakes it's the token they were given by their parent. For the top
+ make, we just subtract one from the number the user wants. */
+
+ if (job_slots > 1 && jobserver_setup (job_slots - 1))
{
- /* If we have >1 slot at this point, then we're a top-level make.
- Set up the jobserver.
-
- Every make assumes that it always has one job it can run. For the
- submakes it's the token they were given by their parent. For the
- top make, we just subtract one from the number the user wants. */
-
- jobserver_setup (job_slots - 1);
-
- /* We're using the jobserver so set job_slots to 0. */
- master_job_slots = job_slots;
- job_slots = 0;
-
/* Fill in the jobserver_auth for our children. */
jobserver_auth = jobserver_get_auth ();
+
+ if (jobserver_auth)
+ {
+ /* We're using the jobserver so set job_slots to 0. */
+ master_job_slots = job_slots;
+ job_slots = 0;
+ }
}
-#endif
/* If we're not using parallel jobs, then we don't need output sync.
This is so people can enable output sync in GNUMAKEFLAGS or similar, but
@@ -2365,11 +2390,6 @@ main (int argc, char **argv, char **envp)
++restarts;
- /* If we're re-exec'ing the first make, put back the number of
- job slots so define_makefiles() will get it right. */
- if (master_job_slots)
- job_slots = master_job_slots;
-
if (ISDB (DB_BASIC))
{
const char **p;
@@ -3400,14 +3420,7 @@ clean_jobserver (int status)
"INTERNAL: Exiting with %u jobserver tokens available; should be %u!",
tokens, master_job_slots);
- jobserver_clear ();
-
- /* Clean out jobserver_auth so we don't pass this information to any
- sub-makes. Also reset job_slots since it will be put on the command
- line, not in MAKEFLAGS. */
- job_slots = default_job_slots;
- free (jobserver_auth);
- jobserver_auth = NULL;
+ reset_jobserver ();
}
}
diff --git a/os.h b/os.h
index f8a5a233..ac5350b1 100644
--- a/os.h
+++ b/os.h
@@ -23,10 +23,10 @@ this program. If not, see <http://www.gnu.org/licenses/>. */
unsigned int jobserver_enabled ();
/* Called in the master instance to set up the jobserver initially. */
-void jobserver_setup (int job_slots);
+unsigned int jobserver_setup (int job_slots);
/* Called in a child instance to connect to the jobserver. */
-void jobserver_parse_auth (const char* auth);
+unsigned int jobserver_parse_auth (const char* auth);
/* Returns an allocated buffer used to pass to child instances. */
char *jobserver_get_auth ();
@@ -57,13 +57,13 @@ void jobserver_pre_acquire ();
in this case we won't wait forever, so we can check the load.
Returns 1 if we got a token, or 0 if we stopped waiting due to a child
exiting or a timeout. */
-int jobserver_acquire (int timeout);
+unsigned int jobserver_acquire (int timeout);
#else
#define jobserver_enabled() (0)
-#define jobserver_setup(_slots) (void)(0)
-#define jobserver_parse_auth(_auth) (void)(0)
+#define jobserver_setup(_slots) (0)
+#define jobserver_parse_auth(_auth) (0)
#define jobserver_get_auth() (NULL)
#define jobserver_clear() (void)(0)
#define jobserver_release(_fatal) (void)(0)
diff --git a/posixos.c b/posixos.c
index 94083059..8c270010 100644
--- a/posixos.c
+++ b/posixos.c
@@ -59,7 +59,7 @@ make_job_rfd ()
#endif
}
-void
+unsigned int
jobserver_setup (int slots)
{
int r;
@@ -77,9 +77,11 @@ jobserver_setup (int slots)
if (r != 1)
pfatal_with_name (_("init jobserver pipe"));
}
+
+ return 1;
}
-void
+unsigned int
jobserver_parse_auth (const char *auth)
{
/* Given the command-line parameter, parse it. */
@@ -106,12 +108,12 @@ jobserver_parse_auth (const char *auth)
if (errno != EBADF)
pfatal_with_name (_("jobserver pipeline"));
- O (error, NILF,
- _("warning: jobserver unavailable: using -j1. Add '+' to parent make rule."));
-
- job_slots = 1;
job_fds[0] = job_fds[1] = -1;
+
+ return 0;
}
+
+ return 1;
}
char *
@@ -175,7 +177,8 @@ jobserver_acquire_all ()
}
/* Prepare the jobserver to start a child process. */
-void jobserver_pre_child (int recursive)
+void
+jobserver_pre_child (int recursive)
{
/* If it's not a recursive make, avoid polutting the jobserver pipes. */
if (!recursive && job_fds[0] >= 0)
@@ -185,7 +188,8 @@ void jobserver_pre_child (int recursive)
}
}
-void jobserver_post_child (int recursive)
+void
+jobserver_post_child (int recursive)
{
#if defined(F_GETFD) && defined(F_SETFD)
if (!recursive && job_fds[0] >= 0)
@@ -229,7 +233,7 @@ jobserver_pre_acquire ()
and only unblocked (atomically) within the pselect() call, so we can
never miss a SIGCHLD.
*/
-int
+unsigned int
jobserver_acquire (int timeout)
{
sigset_t empty;
@@ -273,7 +277,7 @@ jobserver_acquire (int timeout)
/* What does it mean if read() returns 0? It shouldn't happen because only
the master make can reap all the tokens and close the write side...?? */
- return r;
+ return r > 0;
}
#else
@@ -358,7 +362,7 @@ set_child_handler_action_flags (int set_handler, int set_alarm)
#endif
}
-int
+unsigned int
jobserver_acquire (int timeout)
{
char intake;
diff --git a/tests/scripts/features/jobserver b/tests/scripts/features/jobserver
index b5549cc9..7da4a654 100644
--- a/tests/scripts/features/jobserver
+++ b/tests/scripts/features/jobserver
@@ -12,6 +12,48 @@ if (!$parallel_jobs) {
return -1;
}
+# Shorthand
+my $np = '--no-print-directory';
+
+# Simple test of MAKEFLAGS settings
+run_make_test(q!
+SHOW = $(patsubst --jobserver-auth=%,--jobserver-auth=<auth>,$(MAKEFLAGS))
+recurse: ; @echo $@: "/$(SHOW)/"; $(MAKE) -f #MAKEFILE# all
+all:;@echo $@: "/$(SHOW)/"
+!,
+ "-j2 $np", "recurse: /-j2 --jobserver-auth=<auth> $np/\nall: /-j2 --jobserver-auth=<auth> $np/\n");
+
+# Setting parallelism with the environment
+# Command line should take precedence over the environment
+$extraENV{MAKEFLAGS} = "-j2 $np";
+run_make_test(q!
+SHOW = $(patsubst --jobserver-auth=%,--jobserver-auth=<auth>,$(MAKEFLAGS))
+recurse: ; @echo $@: "/$(SHOW)/"; $(MAKE) -f #MAKEFILE# all
+all:;@echo $@: "/$(SHOW)/"
+!,
+ '', "recurse: /-j2 --jobserver-auth=<auth> $np/\nall: /-j2 --jobserver-auth=<auth> $np/\n");
+delete $extraENV{MAKEFLAGS};
+
+# Test override of -jN
+$extraENV{MAKEFLAGS} = "-j9 $np";
+run_make_test(q!
+SHOW = $(patsubst --jobserver-auth=%,--jobserver-auth=<auth>,$(MAKEFLAGS))
+recurse: ; @echo $@: "/$(SHOW)/"; $(MAKE) -j3 -f #MAKEFILE# recurse2
+recurse2: ; @echo $@: "/$(SHOW)/"; $(MAKE) -f #MAKEFILE# all
+all:;@echo $@: "/$(SHOW)/"
+!,
+ "-j2 $np", "recurse: /-j2 --jobserver-auth=<auth> $np/\n#MAKE#[1]: warning: -jN forced in submake: disabling jobserver mode.\nrecurse2: /-j3 --jobserver-auth=<auth> $np/\nall: /-j3 --jobserver-auth=<auth> $np/\n");
+delete $extraENV{MAKEFLAGS};
+
+# Test override of -jN with -j
+run_make_test(q!
+SHOW = $(patsubst --jobserver-auth=%,--jobserver-auth=<auth>,$(MAKEFLAGS))
+recurse: ; @echo $@: "/$(SHOW)/"; $(MAKE) -j -f #MAKEFILE# recurse2
+recurse2: ; @echo $@: "/$(SHOW)/"; $(MAKE) -f #MAKEFILE# all
+all:;@echo $@: "/$(SHOW)/"
+!,
+ "-j2 $np", "recurse: /-j2 --jobserver-auth=<auth> $np/\n#MAKE#[1]: warning: -jN forced in submake: disabling jobserver mode.\nrecurse2: /-j $np/\nall: /-j $np/\n");
+
# Don't put --jobserver-auth into a re-exec'd MAKEFLAGS.
# We can't test this directly because there's no way a makefile can
# show the value of MAKEFLAGS we were re-exec'd with. We can intuit it
@@ -34,7 +76,7 @@ inc.mk:
# @echo 'MAKEFLAGS = $(MAKEFLAGS)'
@echo 'FOO = bar' > $@
!,
- '--no-print-directory -j2', "#MAKE#[1]: warning: -jN forced in submake: disabling jobserver mode.\nall\n");
+ "$np -j2", "#MAKE#[1]: warning: -jN forced in submake: disabling jobserver mode.\nall\n");
unlink('inc.mk');
@@ -52,7 +94,7 @@ close(MAKEFILE);
run_make_test(q!
default: ; @ #MAKEPATH# -f Makefile2
!,
- '-j2 --no-print-directory',
+ "-j2 $np",
"#MAKE#[1]: warning: jobserver unavailable: using -j1. Add '+' to parent make rule.
#MAKE#[1]: Nothing to be done for 'foo'.");
diff --git a/tests/scripts/features/output-sync b/tests/scripts/features/output-sync
index ee3e5ad8..64d6fe08 100644
--- a/tests/scripts/features/output-sync
+++ b/tests/scripts/features/output-sync
@@ -140,7 +140,7 @@ bar: start
bar: end
baz: start
baz: end
-#MAKE#[1]: Leaving directory '#PWD#/bar'\n", 0, 6);
+#MAKE#[1]: Leaving directory '#PWD#/bar'\n", 0, 10);
# Test per-target synchronization.
# Note we have to sleep again here after starting the foo makefile before
@@ -171,7 +171,7 @@ foo: end
#MAKE#[1]: Entering directory '#PWD#/bar'
baz: start
baz: end
-#MAKE#[1]: Leaving directory '#PWD#/bar'\n", 0, 6);
+#MAKE#[1]: Leaving directory '#PWD#/bar'\n", 0, 10);
# Rerun but this time suppress the directory tracking
unlink(@syncfiles);
@@ -183,7 +183,7 @@ bar: end
foo: start
foo: end
baz: start
-baz: end\n", 0, 6);
+baz: end\n", 0, 10);
# Test that messages from make itself are enclosed with
# "Entering/Leaving directory" messages.
@@ -236,7 +236,7 @@ bar: end
#MAKE#[1]: Leaving directory '#PWD#/bar'
#MAKE#[1]: Entering directory '#PWD#/foo'
foo: end
-#MAKE#[1]: Leaving directory '#PWD#/foo'\n", 0, 6);
+#MAKE#[1]: Leaving directory '#PWD#/foo'\n", 0, 10);
# Remove temporary directories and contents.
diff --git a/tests/scripts/variables/MAKEFLAGS b/tests/scripts/variables/MAKEFLAGS
index 8a5d0f6a..0fac74a3 100644
--- a/tests/scripts/variables/MAKEFLAGS
+++ b/tests/scripts/variables/MAKEFLAGS
@@ -39,3 +39,7 @@ jump Works: MAKEFLAGS=e --no-print-directory
print Works: MAKEFLAGS=e --no-print-directory');
1;
+
+### Local Variables:
+### eval: (setq whitespace-action (delq 'auto-cleanup whitespace-action))
+### End:
diff --git a/w32/w32os.c b/w32/w32os.c
index 170c6d4c..533b9100 100644
--- a/w32/w32os.c
+++ b/w32/w32os.c
@@ -33,7 +33,7 @@ this program. If not, see <http://www.gnu.org/licenses/>. */
static char jobserver_semaphore_name[MAX_PATH + 1];
static HANDLE jobserver_semaphore = NULL;
-void
+unsigned int
jobserver_setup (int slots)
{
/* sub_proc.c cannot wait for more than MAXIMUM_WAIT_OBJECTS objects
@@ -61,15 +61,17 @@ jobserver_setup (int slots)
ONS (fatal, NILF,
_("creating jobserver semaphore: (Error %ld: %s)"), err, estr);
}
+
+ return 1;
}
-void
+unsigned int
jobserver_parse_auth (const char *auth)
{
jobserver_semaphore = OpenSemaphore (
- SEMAPHORE_ALL_ACCESS, /* Semaphore access setting */
- FALSE, /* Child processes DON'T inherit */
- auth); /* Semaphore name */
+ SEMAPHORE_ALL_ACCESS, /* Semaphore access setting */
+ FALSE, /* Child processes DON'T inherit */
+ auth); /* Semaphore name */
if (jobserver_semaphore == NULL)
{
@@ -80,6 +82,8 @@ jobserver_parse_auth (const char *auth)
auth, err, estr);
}
DB (DB_JOBS, (_("Jobserver client (semaphore %s)\n"), auth));
+
+ return 1;
}
char *
@@ -161,7 +165,7 @@ jobserver_pre_acquire ()
/* Returns 1 if we got a token, or 0 if a child has completed.
The Windows implementation doesn't support load detection. */
-int
+unsigned int
jobserver_acquire (int timeout)
{
HANDLE handles[MAXIMUM_WAIT_OBJECTS];