Skip to content
73 changes: 71 additions & 2 deletions build.c
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,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 @@ -319,6 +320,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 @@ -538,24 +548,76 @@ queryload(void)
#endif
}

/* returns remaining GNU/tokens */
static ssize_t
returngmake(void)
{
ssize_t returned = 0;
if (buildopts.gmakepipe[1] >= 0)
if ((returned = write(buildopts.gmakepipe[1], gmaketokens, gmakelatest - gmaketokens)) >= 0)
gmakelatest = gmaketokens;
return returned;
}

static void
gmakeatexit(void)
{
if (returngmake() < 0)
warn("last write to jobserver:");
}

static void
termsignal(int signum)
{
write(2, "terminating due to signal\n", 27);
(void)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(gmakeatexit) == 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 @@ -610,6 +672,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
67 changes: 66 additions & 1 deletion samu.c
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
#include <stdlib.h>
#include <string.h>
#include <unistd.h> /* for chdir */
#include <poll.h> /* for poll */
#include <fcntl.h> /* for open */
#include "arg.h"
#include "build.h"
#include "deps.h"
Expand Down Expand Up @@ -90,6 +92,61 @@ 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
char *fifo_path;
struct pollfd check[2];

if (!flag)
return;
if (sscanf(flag, "%d,%d", &read_end, &write_end) == 2) {
/* prepare error message */
errno = EBADF;
Comment thread
capezotte marked this conversation as resolved.
Outdated
} else if (sscanf(flag,"fifo:%ms", &fifo_path) == 1) {
read_end = open(fifo_path, O_RDONLY);
write_end = open(fifo_path, O_WRONLY);
free(fifo_path);
} else {
fatal("invalid jobserver parameter");
}

check[0].fd = read_end;
check[1].fd = write_end;
if (write_end <= 0 || read_end <= 0 || poll(check, 2, 0) == -1 || check[0].revents & POLLNVAL || check[1].revents & POLLNVAL) {
fatal("invalid jobserver fds:");
return;
}
buildopts.gmakepipe[0] = check[0].fd;
buildopts.gmakepipe[1] = check[1].fd;
warn("using GNU Make jobserver");
}

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 +206,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 +259,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 nproc = sysconf(_SC_NPROCESSORS_ONLN);
switch (nproc) {
Expand Down