From: Colin Cross Date: Fri, 18 Sep 2015 21:50:26 +0000 (-0700) Subject: makeparallel: improve support for wrapping ninja X-Git-Tag: android-x86-7.1-r1~608^2~19^2~147^2~10^2~1 X-Git-Url: http://git.osdn.net/view?a=commitdiff_plain;h=69047fab7ee08a62f26b0311bc75dc03e0d5547e;p=android-x86%2Fbuild.git 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 ed8fdfc43..4e7970899 100644 --- a/tools/makeparallel/Makefile +++ b/tools/makeparallel/Makefile @@ -59,6 +59,34 @@ makeparallel_clean: -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 bd682f742..91aacf7d7 100644 --- a/tools/makeparallel/Makefile.test +++ b/tools/makeparallel/Makefile.test @@ -2,4 +2,11 @@ MAKEPARALLEL ?= ./makeparallel .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 5eb1dd669..7dd0ceb2a 100644 --- a/tools/makeparallel/makeparallel.cpp +++ b/tools/makeparallel/makeparallel.cpp @@ -19,6 +19,7 @@ #include #include +#include #include #include #include @@ -55,41 +56,108 @@ static void CheckFd(int fd) { } } -// 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 ReadMakeflags() { + std::vector 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); + + // Drop the first argument if it is empty + while (args.size() > 0 && args[0].size() == 0) { + args.erase(args.begin()); + } - if (start == std::string::npos) { - return false; + // 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]; + } } - start += jobserver_fds_arg.size(); + return args; +} - std::string::size_type end = makeflags.find(' ', start); +static bool ParseMakeflags(std::vector& args, + int* in_fd, int* out_fd, bool* parallel, bool* keep_going) { - std::string::size_type len; - if (end == std::string::npos) { - len = std::string::npos; - } else { - len = end - start; + std::vector 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())); } - std::string jobserver_fds = makeflags.substr(start, len); + opterr = 0; + optind = 1; + while (1) { + const static option longopts[] = { + {"jobserver-fds", required_argument, 0, 0}, + {0, 0, 0, 0}, + }; + int longopt_index = 0; - if (sscanf(jobserver_fds.c_str(), "%d,%d", in_fd, out_fd) != 2) { - return false; + 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(); + } } - CheckFd(*in_fd); - CheckFd(*out_fd); + for (char *v : getopt_argv) { + free(v); + } return true; } @@ -219,20 +287,47 @@ static void PutJobserverTokens(int out_fd, int tokens) { 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 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 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();