GN: Capture and print stderr from exec_script

tools/gn/function_exec_script.cc

Index: tools/gn/function_exec_script.cc
diff --git a/tools/gn/function_exec_script.cc b/tools/gn/function_exec_script.cc
index eb30a6ec399b64aac6ddad66441fdf03baf4aebf..05270b24f8653ed3b76c7935bc94f313af03194b 100644
--- a/tools/gn/function_exec_script.cc
+++ b/tools/gn/function_exec_script.cc
@@ -5,13 +5,12 @@
 #include "base/command_line.h"
 #include "base/files/file_util.h"
 #include "base/logging.h"
-#include "base/process/kill.h"
-#include "base/process/launch.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
 #include "tools/gn/err.h"
+#include "tools/gn/exec_process.h"
 #include "tools/gn/filesystem_utils.h"
 #include "tools/gn/functions.h"
 #include "tools/gn/input_conversion.h"
@@ -21,215 +20,10 @@
 #include "tools/gn/trace.h"
 #include "tools/gn/value.h"
 
-#if defined(OS_WIN)
-#include <windows.h>
-
-#include "base/win/scoped_handle.h"
-#include "base/win/scoped_process_information.h"
-#endif
-
-#if defined(OS_POSIX)
-#include <fcntl.h>
-#include <unistd.h>
-
-#include "base/posix/file_descriptor_shuffle.h"
-#endif
-
 namespace functions {
 
 namespace {
-
 const char kNoExecSwitch[] = "no-exec";
-
-#if defined(OS_WIN)
-bool ExecProcess(const CommandLine& cmdline,
-                 const base::FilePath& startup_dir,
-                 std::string* std_out,
-                 std::string* std_err,
-                 int* exit_code) {
-  SECURITY_ATTRIBUTES sa_attr;
-  // Set the bInheritHandle flag so pipe handles are inherited.
-  sa_attr.nLength = sizeof(SECURITY_ATTRIBUTES);
-  sa_attr.bInheritHandle = TRUE;
-  sa_attr.lpSecurityDescriptor = NULL;
-
-  // Create the pipe for the child process's STDOUT.
-  HANDLE out_read = NULL;
-  HANDLE out_write = NULL;
-  if (!CreatePipe(&out_read, &out_write, &sa_attr, 0)) {
-    NOTREACHED() << "Failed to create pipe";
-    return false;
-  }
-  base::win::ScopedHandle scoped_out_read(out_read);
-  base::win::ScopedHandle scoped_out_write(out_write);
-
-  // Create the pipe for the child process's STDERR.
-  HANDLE err_read = NULL;
-  HANDLE err_write = NULL;
-  if (!CreatePipe(&err_read, &err_write, &sa_attr, 0)) {
-    NOTREACHED() << "Failed to create pipe";
-    return false;
-  }
-  base::win::ScopedHandle scoped_err_read(err_read);
-  base::win::ScopedHandle scoped_err_write(err_write);
-
-  // Ensure the read handle to the pipe for STDOUT/STDERR is not inherited.
-  if (!SetHandleInformation(out_read, HANDLE_FLAG_INHERIT, 0)) {
-    NOTREACHED() << "Failed to disabled pipe inheritance";
-    return false;
-  }
-  if (!SetHandleInformation(err_read, HANDLE_FLAG_INHERIT, 0)) {
-    NOTREACHED() << "Failed to disabled pipe inheritance";
-    return false;
-  }
-
-  base::FilePath::StringType cmdline_str(cmdline.GetCommandLineString());
-
-  STARTUPINFO start_info = {};
-
-  start_info.cb = sizeof(STARTUPINFO);
-  start_info.hStdOutput = out_write;
-  // Keep the normal stdin.
-  start_info.hStdInput = GetStdHandle(STD_INPUT_HANDLE);
-  // FIXME(brettw) set stderr here when we actually read it below.
-  //start_info.hStdError = err_write;
-  start_info.hStdError = GetStdHandle(STD_ERROR_HANDLE);
-  start_info.dwFlags |= STARTF_USESTDHANDLES;
-
-  // Create the child process.
-  PROCESS_INFORMATION temp_process_info = {};
-  if (!CreateProcess(NULL,
-                     &cmdline_str[0],
-                     NULL, NULL,
-                     TRUE,  // Handles are inherited.
-                     0, NULL,
-                     startup_dir.value().c_str(),
-                     &start_info, &temp_process_info)) {
-    return false;
-  }
-  base::win::ScopedProcessInformation proc_info(temp_process_info);
-
-  // Close our writing end of pipes now. Otherwise later read would not be able
-  // to detect end of child's output.
-  scoped_out_write.Close();
-  scoped_err_write.Close();
-
-  // Read output from the child process's pipe for STDOUT
-  const int kBufferSize = 1024;
-  char buffer[kBufferSize];
-
-  // FIXME(brettw) read from stderr here! This is complicated because we want
-  // to read both of them at the same time, probably need overlapped I/O.
-  // Also uncomment start_info code above.
-  for (;;) {
-    DWORD bytes_read = 0;
-    BOOL success = ReadFile(out_read, buffer, kBufferSize, &bytes_read, NULL);
-    if (!success || bytes_read == 0)
-      break;
-    std_out->append(buffer, bytes_read);
-  }
-
-  // Let's wait for the process to finish.
-  WaitForSingleObject(proc_info.process_handle(), INFINITE);
-
-  DWORD dw_exit_code;
-  GetExitCodeProcess(proc_info.process_handle(), &dw_exit_code);
-  *exit_code = static_cast<int>(dw_exit_code);
-
-  return true;
-}
-#else
-bool ExecProcess(const CommandLine& cmdline,
-                 const base::FilePath& startup_dir,
-                 std::string* std_out,
-                 std::string* std_err,
-                 int* exit_code) {
-  *exit_code = EXIT_FAILURE;
-
-  std::vector<std::string> argv = cmdline.argv();
-
-  int pipe_fd[2];
-  pid_t pid;
-  base::InjectiveMultimap fd_shuffle1, fd_shuffle2;
-  scoped_ptr<char*[]> argv_cstr(new char*[argv.size() + 1]);
-
-  fd_shuffle1.reserve(3);
-  fd_shuffle2.reserve(3);
-
-  if (pipe(pipe_fd) < 0)
-    return false;
-
-  switch (pid = fork()) {
-    case -1:  // error
-      close(pipe_fd[0]);
-      close(pipe_fd[1]);
-      return false;
-    case 0:  // child
-      {
-        // DANGER: no calls to malloc are allowed from now on:
-        // http://crbug.com/36678
-
-        // Obscure fork() rule: in the child, if you don't end up doing exec*(),
-        // you call _exit() instead of exit(). This is because _exit() does not
-        // call any previously-registered (in the parent) exit handlers, which
-        // might do things like block waiting for threads that don't even exist
-        // in the child.
-        int dev_null = open("/dev/null", O_WRONLY);
-        if (dev_null < 0)
-          _exit(127);
-
-        fd_shuffle1.push_back(
-            base::InjectionArc(pipe_fd[1], STDOUT_FILENO, true));
-        fd_shuffle1.push_back(
-            base::InjectionArc(dev_null, STDERR_FILENO, true));
-        fd_shuffle1.push_back(
-            base::InjectionArc(dev_null, STDIN_FILENO, true));
-        // Adding another element here? Remeber to increase the argument to
-        // reserve(), above.
-
-        for (size_t i = 0; i < fd_shuffle1.size(); ++i)
-          fd_shuffle2.push_back(fd_shuffle1[i]);
-
-        if (!ShuffleFileDescriptors(&fd_shuffle1))
-          _exit(127);
-
-        base::SetCurrentDirectory(startup_dir);
-
-        // TODO(brettw) the base version GetAppOutput does a
-        // CloseSuperfluousFds call here. Do we need this?
-
-        for (size_t i = 0; i < argv.size(); i++)
-          argv_cstr[i] = const_cast<char*>(argv[i].c_str());
-        argv_cstr[argv.size()] = NULL;
-        execvp(argv_cstr[0], argv_cstr.get());
-        _exit(127);
-      }
-    default:  // parent
-      {
-        // Close our writing end of pipe now. Otherwise later read would not
-        // be able to detect end of child's output (in theory we could still
-        // write to the pipe).
-        close(pipe_fd[1]);
-
-        char buffer[256];
-        ssize_t bytes_read = 0;
-
-        while (true) {
-          bytes_read = HANDLE_EINTR(read(pipe_fd[0], buffer, sizeof(buffer)));
-          if (bytes_read <= 0)
-            break;
-          std_out->append(buffer, bytes_read);
-        }
-        close(pipe_fd[0]);
-
-        return base::WaitForExitCode(pid, exit_code);
-      }
-  }
-
-  return false;
-}
-#endif
-
 }  // namespace
 
 const char kExecScript[] = "exec_script";
@@ -378,11 +172,11 @@ Value RunExecScript(Scope* scope,
   // Execute the process.
   // TODO(brettw) set the environment block.
   std::string output;
-  std::string stderr_output;  // TODO(brettw) not hooked up, see above.
+  std::string stderr_output;
   int exit_code = 0;
   if (!CommandLine::ForCurrentProcess()->HasSwitch(kNoExecSwitch)) {
-    if (!ExecProcess(cmdline, startup_dir,
-                     &output, &stderr_output, &exit_code)) {
+    if (!internal::ExecProcess(
+            cmdline, startup_dir, &output, &stderr_output, &exit_code)) {
       *err = Err(function->function(), "Could not execute python.",
           "I was trying to execute \"" + FilePathToUTF8(python_path) + "\".");
       return Value();
@@ -395,7 +189,6 @@ Value RunExecScript(Scope* scope,
         "ms");
   }
 
-  // TODO(brettw) maybe we need stderr also for reasonable stack dumps.
   if (exit_code != 0) {
     std::string msg = "Current dir: " + FilePathToUTF8(startup_dir) +
         "\nCommand: " + FilePathToUTF8(cmdline.GetCommandLineString()) +
@@ -404,6 +197,9 @@ Value RunExecScript(Scope* scope,
       msg += " and printed out:\n\n" + output;
     else
       msg += ".";
+    if (!stderr_output.empty())
+      msg += "\nstderr:\n\n" + stderr_output;
+
     *err = Err(function->function(), "Script returned non-zero exit code.",
                msg);
     return Value();