makeparallel: improve support for wrapping ninja
Allow makeparallel to pass better -j and -k arguments to ninja if the
first argument to makeparallel is --ninja. Uses getopt to parse
MAKEFLAGS to get values for --jobserver-fds, -k, and -j, and uses the
result to not pass any -j argument to ninja for make -j with no number,
and pass -k0 to ninja for make -k.
Also improve the test makefile to provide many more tests.
Bug: 24199503
Change-Id: Id6481430f77e9e952213be58a98fe78c46ee5d6a
diff --git a/tools/makeparallel/Makefile b/tools/makeparallel/Makefile
index ed8fdfc..4e79708 100644
--- a/tools/makeparallel/Makefile
+++ b/tools/makeparallel/Makefile
@@ -59,6 +59,34 @@
-include $(MAKEPARALLEL_INTERMEDIATES_PATH)/*.d
-.PHONY: test
-test: $(MAKEPARALLEL)
- MAKEFLAGS= $(MAKE) -j1234 -C $(MAKEPARALLEL_SRC_PATH) -f Makefile.test MAKEPARALLEL=$(MAKEPARALLEL) test
+.PHONY: makeparallel_test
+MAKEPARALLEL_TEST := MAKEFLAGS= MAKELEVEL= MAKEPARALLEL=$(MAKEPARALLEL) $(MAKE) -f Makefile.test test
+MAKEPARALLEL_NINJA_TEST := MAKEFLAGS= MAKELEVEL= MAKEPARALLEL="$(MAKEPARALLEL) --ninja" $(MAKE) -f Makefile.test test
+makeparallel_test: $(MAKEPARALLEL)
+ @EXPECTED="-j1234" $(MAKEPARALLEL_TEST) -j1234
+ @EXPECTED="-j123" $(MAKEPARALLEL_TEST) -j123
+ @EXPECTED="-j1" $(MAKEPARALLEL_TEST) -j1
+ @EXPECTED="-j1" $(MAKEPARALLEL_TEST)
+
+ @EXPECTED="-j1234" $(MAKEPARALLEL_NINJA_TEST) -j1234
+ @EXPECTED="-j123" $(MAKEPARALLEL_NINJA_TEST) -j123
+ @EXPECTED="-j1" $(MAKEPARALLEL_NINJA_TEST) -j1
+ @EXPECTED="-j1" $(MAKEPARALLEL_NINJA_TEST)
+ @EXPECTED="" $(MAKEPARALLEL_NINJA_TEST) -j
+ @EXPECTED="" $(MAKEPARALLEL_NINJA_TEST) -j -l
+
+ @EXPECTED="-j1234" $(MAKEPARALLEL_TEST) --no-print-directory -j1234
+ @EXPECTED="-j1234" $(MAKEPARALLEL_TEST) --no-print-directory -k -j1234
+ @EXPECTED="-j1234" $(MAKEPARALLEL_TEST) -k -j1234
+ @EXPECTED="-j1234" $(MAKEPARALLEL_TEST) -j1234 -k
+ @EXPECTED="-j1234" $(MAKEPARALLEL_TEST) -kt -j1234
+
+ @EXPECTED="-j1234" $(MAKEPARALLEL_NINJA_TEST) --no-print-directory -j1234
+ @EXPECTED="-j1234 -k0" $(MAKEPARALLEL_NINJA_TEST) --no-print-directory -k -j1234
+ @EXPECTED="-j1234 -k0" $(MAKEPARALLEL_NINJA_TEST) -k -j1234
+ @EXPECTED="-j1234 -k0" $(MAKEPARALLEL_NINJA_TEST) -j1234 -k
+ @EXPECTED="-j1234 -k0" $(MAKEPARALLEL_NINJA_TEST) -kt -j1234
+
+ @EXPECTED="-j1" $(MAKEPARALLEL_TEST) A=-j1234
+ @EXPECTED="-j1" $(MAKEPARALLEL_TEST) A\ -j1234=-j1234
+ @EXPECTED="-j1234" $(MAKEPARALLEL_TEST) A\ -j1234=-j1234 -j1234
diff --git a/tools/makeparallel/Makefile.test b/tools/makeparallel/Makefile.test
index bd682f7..91aacf7 100644
--- a/tools/makeparallel/Makefile.test
+++ b/tools/makeparallel/Makefile.test
@@ -2,4 +2,11 @@
.PHONY: test
test:
- +if [ "$$($(MAKEPARALLEL) echo)" = "-j1234" ]; then echo SUCCESS; else echo FAILED; fi
+ @+echo MAKEFLAGS=$${MAKEFLAGS}; \
+ result=$$($(MAKEPARALLEL) echo); \
+ echo result: $${result}; \
+ if [ "$${result}" = "$(EXPECTED)" ]; then \
+ echo SUCCESS && echo; \
+ else \
+ echo FAILED expected $(EXPECTED) && false; \
+ fi
diff --git a/tools/makeparallel/makeparallel.cpp b/tools/makeparallel/makeparallel.cpp
index 5eb1dd6..7dd0ceb 100644
--- a/tools/makeparallel/makeparallel.cpp
+++ b/tools/makeparallel/makeparallel.cpp
@@ -19,6 +19,7 @@
#include <errno.h>
#include <fcntl.h>
+#include <getopt.h>
#include <poll.h>
#include <signal.h>
#include <stdio.h>
@@ -55,41 +56,108 @@
}
}
-// Extract --jobserver-fds= argument from MAKEFLAGS environment variable.
-static int GetJobserver(int* in_fd, int* out_fd) {
+// Extract flags from MAKEFLAGS that need to be propagated to subproccess
+static std::vector<std::string> ReadMakeflags() {
+ std::vector<std::string> args;
+
const char* makeflags_env = getenv("MAKEFLAGS");
if (makeflags_env == nullptr) {
- return false;
+ return args;
}
+ // The MAKEFLAGS format is pretty useless. The first argument might be empty
+ // (starts with a leading space), or it might be a set of one-character flags
+ // merged together with no leading space, or it might be a variable
+ // definition.
+
std::string makeflags = makeflags_env;
- const std::string jobserver_fds_arg = "--jobserver-fds=";
- size_t start = makeflags.find(jobserver_fds_arg);
+ // Split makeflags into individual args on spaces. Multiple spaces are
+ // elided, but an initial space will result in a blank arg.
+ size_t base = 0;
+ size_t found;
+ do {
+ found = makeflags.find_first_of(" ", base);
+ args.push_back(makeflags.substr(base, found - base));
+ base = found + 1;
+ } while (found != makeflags.npos);
- if (start == std::string::npos) {
- return false;
+ // Drop the first argument if it is empty
+ while (args.size() > 0 && args[0].size() == 0) {
+ args.erase(args.begin());
}
- start += jobserver_fds_arg.size();
-
- std::string::size_type end = makeflags.find(' ', start);
-
- std::string::size_type len;
- if (end == std::string::npos) {
- len = std::string::npos;
- } else {
- len = end - start;
+ // Prepend a - to the first argument if it does not have one and is not a
+ // variable definition
+ if (args.size() > 0 && args[0][0] != '-') {
+ if (args[0].find('=') == makeflags.npos) {
+ args[0] = '-' + args[0];
+ }
}
- std::string jobserver_fds = makeflags.substr(start, len);
+ return args;
+}
- if (sscanf(jobserver_fds.c_str(), "%d,%d", in_fd, out_fd) != 2) {
- return false;
+static bool ParseMakeflags(std::vector<std::string>& args,
+ int* in_fd, int* out_fd, bool* parallel, bool* keep_going) {
+
+ std::vector<char*> getopt_argv;
+ // getopt starts reading at argv[1]
+ getopt_argv.reserve(args.size() + 1);
+ getopt_argv.push_back(strdup(""));
+ for (std::string& v : args) {
+ getopt_argv.push_back(strdup(v.c_str()));
}
- CheckFd(*in_fd);
- CheckFd(*out_fd);
+ opterr = 0;
+ optind = 1;
+ while (1) {
+ const static option longopts[] = {
+ {"jobserver-fds", required_argument, 0, 0},
+ {0, 0, 0, 0},
+ };
+ int longopt_index = 0;
+
+ int c = getopt_long(getopt_argv.size(), getopt_argv.data(), "kj",
+ longopts, &longopt_index);
+
+ if (c == -1) {
+ break;
+ }
+
+ switch (c) {
+ case 0:
+ switch (longopt_index) {
+ case 0:
+ {
+ // jobserver-fds
+ if (sscanf(optarg, "%d,%d", in_fd, out_fd) != 2) {
+ error(EXIT_FAILURE, 0, "incorrect format for --jobserver-fds: %s", optarg);
+ }
+ // TODO: propagate in_fd, out_fd
+ break;
+ }
+ default:
+ abort();
+ }
+ break;
+ case 'j':
+ *parallel = true;
+ break;
+ case 'k':
+ *keep_going = true;
+ break;
+ case '?':
+ // ignore unknown arguments
+ break;
+ default:
+ abort();
+ }
+ }
+
+ for (char *v : getopt_argv) {
+ free(v);
+ }
return true;
}
@@ -219,20 +287,47 @@
int main(int argc, char* argv[]) {
int in_fd = -1;
int out_fd = -1;
+ bool parallel = false;
+ bool keep_going = false;
+ bool ninja = false;
int tokens = 0;
+ if (argc > 1 && strcmp(argv[1], "--ninja") == 0) {
+ ninja = true;
+ argv++;
+ argc--;
+ }
+
const char* path = argv[1];
std::vector<char*> args(&argv[1], &argv[argc]);
- if (GetJobserver(&in_fd, &out_fd)) {
- fcntl(in_fd, F_SETFD, FD_CLOEXEC);
- fcntl(out_fd, F_SETFD, FD_CLOEXEC);
-
- tokens = GetJobserverTokens(in_fd);
+ std::vector<std::string> makeflags = ReadMakeflags();
+ if (ParseMakeflags(makeflags, &in_fd, &out_fd, ¶llel, &keep_going)) {
+ if (in_fd >= 0 && out_fd >= 0) {
+ CheckFd(in_fd);
+ CheckFd(out_fd);
+ fcntl(in_fd, F_SETFD, FD_CLOEXEC);
+ fcntl(out_fd, F_SETFD, FD_CLOEXEC);
+ tokens = GetJobserverTokens(in_fd);
+ }
}
std::string jarg = "-j" + std::to_string(tokens + 1);
- args.push_back(strdup(jarg.c_str()));
+
+ if (ninja) {
+ if (!parallel) {
+ // ninja is parallel by default, pass -j1 to disable parallelism if make wasn't parallel
+ args.push_back(strdup("-j1"));
+ } else if (tokens > 0) {
+ args.push_back(strdup(jarg.c_str()));
+ }
+ if (keep_going) {
+ args.push_back(strdup("-k0"));
+ }
+ } else {
+ args.push_back(strdup(jarg.c_str()));
+ }
+
args.push_back(nullptr);
pid_t pid = fork();