Replace Subprocess::pipe* syntax sugar with Subprocess::Options().pipe*
authorAlexey Spiridonov <lesha@fb.com>
Wed, 12 Apr 2017 21:43:04 +0000 (14:43 -0700)
committerFacebook Github Bot <facebook-github-bot@users.noreply.github.com>
Wed, 12 Apr 2017 21:55:36 +0000 (14:55 -0700)
Summary:
This is a bit too magical -- it's not clear that the thing produces an Options object. If you do know that you can chain further option setters off this thing, it's nice, but otherwise, the first impression is "what just happened?".

So, let's have one good way for doing things.

Reviewed By: yfeldblum

Differential Revision: D4863947

fbshipit-source-id: 3dfe83cfc077d47f604f47dcb21149fbaa2d2243

folly/Subprocess.h
folly/experimental/test/ProgramOptionsTest.cpp
folly/test/SubprocessTest.cpp
folly/tracing/test/StaticTracepointTest.cpp

index d739288..2afa238 100644 (file)
  * to complete, returning the exit status.
  *
  * A thread-safe [1] version of popen() (type="r", to read from the child):
- *    Subprocess proc(cmd, Subprocess::pipeStdout());
+ *    Subprocess proc(cmd, Subprocess::Options().pipeStdout());
  *    // read from proc.stdoutFd()
  *    proc.wait();
  *
  * A thread-safe [1] version of popen() (type="w", to write to the child):
- *    Subprocess proc(cmd, Subprocess::pipeStdin());
+ *    Subprocess proc(cmd, Subprocess::Options().pipeStdin());
  *    // write to proc.stdinFd()
  *    proc.wait();
  *
@@ -439,10 +439,6 @@ class Subprocess {
 #endif
   };
 
-  static Options pipeStdin() { return Options().stdinFd(PIPE); }
-  static Options pipeStdout() { return Options().stdoutFd(PIPE); }
-  static Options pipeStderr() { return Options().stderrFd(PIPE); }
-
   // Non-copiable, but movable
   Subprocess(const Subprocess&) = delete;
   Subprocess& operator=(const Subprocess&) = delete;
index d5f58bc..208c841 100644 (file)
@@ -55,7 +55,7 @@ std::string callHelper(ProgramOptionsStyle style,
     break;
   }
 
-  Subprocess proc(allArgs, Subprocess::pipeStdout(), nullptr, &env);
+  Subprocess proc(allArgs, Subprocess::Options().pipeStdout(), nullptr, &env);
   auto p = proc.communicate();
   EXPECT_EQ(0, proc.wait().exitStatus());
 
index d9763ae..74a822e 100644 (file)
@@ -222,8 +222,9 @@ TEST(SimpleSubprocessTest, FdLeakTest) {
   // Test where the exec call fails() with pipes
   checkFdLeak([] {
     try {
-      Subprocess proc(std::vector<std::string>({"/no/such/file"}),
-                      Subprocess::pipeStdout().stderrFd(Subprocess::PIPE));
+      Subprocess proc(
+          std::vector<std::string>({"/no/such/file"}),
+          Subprocess::Options().pipeStdout().stderrFd(Subprocess::PIPE));
       ADD_FAILURE() << "expected an error when running /no/such/file";
     } catch (const SubprocessSpawnError& ex) {
       EXPECT_EQ(ENOENT, ex.errnoValue());
@@ -258,7 +259,7 @@ TEST(ParentDeathSubprocessTest, ParentDeathSignal) {
 }
 
 TEST(PopenSubprocessTest, PopenRead) {
-  Subprocess proc("ls /", Subprocess::pipeStdout());
+  Subprocess proc("ls /", Subprocess::Options().pipeStdout());
   int found = 0;
   gen::byLine(File(proc.stdoutFd())) |
     [&] (StringPiece line) {
@@ -317,8 +318,9 @@ TEST(AfterForkCallbackSubprocessTest, TestAfterForkCallbackError) {
 }
 
 TEST(CommunicateSubprocessTest, SimpleRead) {
-  Subprocess proc(std::vector<std::string>{ "/bin/echo", "-n", "foo", "bar"},
-                  Subprocess::pipeStdout());
+  Subprocess proc(
+      std::vector<std::string>{"/bin/echo", "-n", "foo", "bar"},
+      Subprocess::Options().pipeStdout());
   auto p = proc.communicate();
   EXPECT_EQ("foo bar", p.first);
   proc.waitChecked();
@@ -375,7 +377,8 @@ TEST(CommunicateSubprocessTest, Duplex2) {
       "-e", "s/a test/a successful test/",
       "-e", "/^another line/w/dev/stderr",
     });
-    auto options = Subprocess::pipeStdin().pipeStdout().pipeStderr().usePath();
+    auto options =
+        Subprocess::Options().pipeStdin().pipeStdout().pipeStderr().usePath();
     Subprocess proc(cmd, options);
     auto out = proc.communicateIOBuf(std::move(input));
     proc.waitChecked();
@@ -463,7 +466,8 @@ TEST(CommunicateSubprocessTest, Chatty) {
     int wcount = 0;
     int rcount = 0;
 
-    auto options = Subprocess::pipeStdin().pipeStdout().pipeStderr().usePath();
+    auto options =
+        Subprocess::Options().pipeStdin().pipeStdout().pipeStderr().usePath();
     std::vector<std::string> cmd {
       "sed",
       "-u",
index 7c53efd..710c565 100644 (file)
@@ -98,7 +98,8 @@ static std::string getNoteRawContent(const std::string& fileName) {
       "section",
       ".note." + kUSDTSubsectionName,
       fileName);
-  auto subProc = folly::Subprocess(args, folly::Subprocess::pipeStdout());
+  auto subProc =
+      folly::Subprocess(args, folly::Subprocess::Options().pipeStdout());
   auto output = subProc.communicate();
   auto retCode = subProc.wait();
   CHECK(retCode.exited());