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 (file)
--- 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 bd682f7..91aacf7 100644 (file)
--- 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 5eb1dd6..7dd0ceb 100644 (file)
--- 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 @@ 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<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);
+
+  // 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<std::string>& 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<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()));
   }
 
-  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<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, &parallel, &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();