Skip to content
68 changes: 66 additions & 2 deletions build.c
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,12 @@ struct job {
bool failed;
};

struct buildoptions buildopts = {.maxfail = 1};
struct buildoptions buildopts = {.maxfail = 1, .gmakepipe = {-1, -1} };
static struct edge *work;
static size_t nstarted, nfinished, ntotal;
static bool consoleused;
static struct timespec starttime;
static char gmaketokens[512], *gmakelatest = gmaketokens;

void
buildreset(void)
Expand Down Expand Up @@ -322,6 +323,15 @@ jobstart(struct job *j, struct edge *e)
warn("posix_spawn_file_actions_addclose:");
goto err3;
}
if (buildopts.gmakepipe[0] >= 0) {
/* do not allow children to steal GNU/tokens */
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @trws asked, do we need to prevent children from interacting with the job server?

If we do, I think it'd be better to set FD_CLOEXEC on the fds than closing them here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember why I thought of that in the first place (nowhere in the documentation is this recommended).

After testing for a bit, it seems GNU Make uses the same --jobserver-auth parameters for every client, so it's fine for samu to share the jobserver with its own clients.

for (i = 0; i < 2; ++i) {
if ((errno = posix_spawn_file_actions_addclose(&actions, buildopts.gmakepipe[i]))) {
warn("posix_spawn_file_actions_addclose:");
goto err3;
}
}
}
if (e->pool != &consolepool) {
if ((errno = posix_spawn_file_actions_addopen(&actions, 0, "/dev/null", O_RDONLY, 0))) {
warn("posix_spawn_file_actions_addopen:");
Expand Down Expand Up @@ -541,24 +551,71 @@ queryload(void)
#endif
}

/* returns remaining GNU/tokens */
static void
returngmake(void)
{
if (buildopts.gmakepipe[1] >= 0) {
if (write(buildopts.gmakepipe[1], gmaketokens, gmakelatest - gmaketokens) < 0) {
warn("last write to jobserver:");
Comment thread
capezotte marked this conversation as resolved.
Outdated
} else {
gmakelatest = gmaketokens;
}
}
}

static void
termsignal(int signum)
{
write(2, "samu: terminating due to signal\n", 32);
returngmake();
_exit(128 + signum);
}

void
build(void)
{
struct sigaction sact = { 0 };
struct job *jobs = NULL;
struct pollfd *fds = NULL;
struct pollfd *fds = NULL, tokenin = { .fd = -1, .events = POLLIN };
size_t i, next = 0, jobslen = 0, maxjobs = buildopts.maxjobs, numjobs = 0, numfail = 0;
ssize_t gmakeread;
struct edge *e;

if (ntotal == 0) {
warn("nothing to do");
return;
}

if (buildopts.gmakepipe[0] >= 0) {
if (atexit(returngmake) == 0) {
maxjobs = 1; /* will change dynamically as tokens are exchanged */
tokenin.fd = buildopts.gmakepipe[0];
sact.sa_handler = termsignal;
sigaction(SIGTERM, &sact, NULL);
sigaction(SIGINT, &sact, NULL);
sigaction(SIGPIPE, &sact, NULL);
} else {
warn("unable to register an atexit() function");
}
}

clock_gettime(CLOCK_MONOTONIC, &starttime);
formatstatus(NULL, 0);

nstarted = 0;
for (;;) {
/* see if GNU/make has authorized more jobs */
if (poll(&tokenin, 1, 0) > 0) {
Comment thread
michaelforney marked this conversation as resolved.
Outdated
gmakeread = gmakelatest - gmaketokens;
gmakeread = read(buildopts.gmakepipe[0], gmakelatest, 512 - (size_t)gmakeread > ntotal - nstarted ? ntotal - nstarted : 512 - gmakeread);
if (gmakeread < 0) {
warn("read from jobserver:");
gmakeread = 0;
}
gmakelatest += gmakeread;
maxjobs += gmakeread;
}
/* limit number of of jobs based on load */
if (buildopts.maxload)
maxjobs = queryload() > buildopts.maxload ? 1 : buildopts.maxjobs;
Expand Down Expand Up @@ -613,6 +670,13 @@ build(void)
next = i;
if (jobs[i].failed)
++numfail;
/* must return the GNU/tokens once a job is done */
if (buildopts.gmakepipe[1] > 0 && gmakelatest != gmaketokens) {
if (write(buildopts.gmakepipe[1], --gmakelatest, 1) < 0) {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if it matters, but this might not be the same job token that we acquired to start this job. Maybe we should track the job token in the job struct?

Copy link
Copy Markdown
Contributor Author

@capezotte capezotte Apr 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is necessary, it's not like make knows how exactly the job slots are used by the client. In fact, despite documentation saying "don’t assume that all tokens are the same character", in my testing they usually are.

warn("write to jobserver:");
}
--maxjobs;
}
}
}
for (i = 0; i < jobslen; ++i)
Expand Down
1 change: 1 addition & 0 deletions build.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ struct buildoptions {
_Bool verbose, explain, keepdepfile, keeprsp, dryrun;
const char *statusfmt;
double maxload;
int gmakepipe[2];
};

extern struct buildoptions buildopts;
Expand Down
59 changes: 58 additions & 1 deletion samu.c
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include <stdlib.h>
#include <string.h>
#include <unistd.h> /* for chdir */
#include <poll.h> /* for poll */
#include "arg.h"
#include "build.h"
#include "deps.h"
Expand Down Expand Up @@ -90,6 +91,54 @@ jobsflag(const char *flag)
buildopts.maxjobs = num > 0 ? num : -1;
}

static void
jobserverflags(const char *flag)
{
int read_end, write_end;
Comment thread
capezotte marked this conversation as resolved.
Outdated
struct pollfd check[2];

if (!flag)
return;
if (sscanf(flag, "%d,%d", &read_end, &write_end) == 2 && read_end >= 0 && write_end >= 0) {
/* assert valid fds */
check[0].fd = read_end;
check[1].fd = write_end;
if (poll(check, 2, 0) == -1 || check[0].revents & POLLNVAL || check[1].revents & POLLNVAL) {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should do a validity check like this. If the fds are invalid that's the caller's problem, and I'd like to minimize the POSIX usage as much as possible.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation recommends doing the check. As it's there mostly to address a PEBKAC (user forgot a + in the Makefile), I do understand if you want to drop it.

fatal("jobserver fds are not open");
return;
}
buildopts.gmakepipe[0] = read_end;
buildopts.gmakepipe[1] = write_end;
warn("using GNU Make jobserver");
} else {
fatal("invalid jobserver parameter");
}
}

static void
parsegmakeflags(char *env) {
char *arg;

if (!env)
return;
env = xmemdup(env, strlen(env) + 1);

arg = strtok(env, " ");
/* first word might contain dry run */
if (arg && strchr(arg, 'n'))
buildopts.dryrun = true;
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain this? As far as I can tell, the first word is supposed to be the single-character options, right? I see that by default it is just w. However, if I run make --no-print-directory, it is empty with just a blank space, so I think we might accidentally skip over the jobserver flags.

Another question: how is samu running in the first place if make is in dry run mode?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That does seem to be what the first word is for, though the protocol documentation only goes as far as saying "check the first word for the letter n". Thanks for catching the oversight.

As for the second question, GNU Make (version 4.4.1 my system) invokes jobserver clients even in dry run mode.

arg = strtok(NULL, " ");
while (arg) {
if (strncmp(arg, "-j", 2) == 0) {
/* handled by parent process */
} else if (strncmp(arg, "--jobserver-auth=", 17) == 0 || strncmp(arg, "--jobserver-fds=", 16) == 0) {
Comment thread
capezotte marked this conversation as resolved.
Outdated
jobserverflags(strchr(arg, '=')+1);
}
arg = strtok(NULL, " ");
}
free(env);
}

static void
parseenvargs(char *env)
{
Expand Down Expand Up @@ -149,6 +198,7 @@ main(int argc, char *argv[])

argv0 = progname(argv[0], "samu");
parseenvargs(getenv("SAMUFLAGS"));
parsegmakeflags(getenv("MAKEFLAGS"));
ARGBEGIN {
case '-':
arg = EARGF(usage());
Expand Down Expand Up @@ -201,7 +251,14 @@ main(int argc, char *argv[])
usage();
} ARGEND
argdone:
if (!buildopts.maxjobs) {
if (buildopts.gmakepipe[0] >= 0) {
if (buildopts.maxjobs)
warn("ignoring -j setting as GNU Make job client is enabled");
if (buildopts.maxload)
warn("ignoring -l setting as GNU Make job client is enabled");
buildopts.maxjobs = -1;
buildopts.maxload = 0;
} else if (!buildopts.maxjobs) {
#ifdef _SC_NPROCESSORS_ONLN
int n = sysconf(_SC_NPROCESSORS_ONLN);
switch (n) {
Expand Down