Replace Subprocess::pipe* syntax sugar with Subprocess::Options().pipe*

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

diff --git a/folly/Subprocess.h b/folly/Subprocess.h
index d739288a1c49b73aded1a4679d54009ed534c048..2afa2382b1fd66fb9e091c29cbc9ac23eb853886 100644 (file)
--- a/folly/Subprocess.h
+++ b/folly/Subprocess.h
@@ -33,12 +33,12 @@
  * 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;

diff --git a/folly/experimental/test/ProgramOptionsTest.cpp b/folly/experimental/test/ProgramOptionsTest.cpp
index d5f58bc70c88b00c03b5fc6488e0abe2c3e60bb0..208c841570a5b2042c5f80802fbf5b129c271de4 100644 (file)
--- a/folly/experimental/test/ProgramOptionsTest.cpp
+++ b/folly/experimental/test/ProgramOptionsTest.cpp
@@ -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());
 

diff --git a/folly/test/SubprocessTest.cpp b/folly/test/SubprocessTest.cpp
index d9763ae63ecf19316c3164891d8a39057d3aa35a..74a822e30d601a33c7beb68a414622942e815dbd 100644 (file)
--- a/folly/test/SubprocessTest.cpp
+++ b/folly/test/SubprocessTest.cpp
@@ -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",

diff --git a/folly/tracing/test/StaticTracepointTest.cpp b/folly/tracing/test/StaticTracepointTest.cpp
index 7c53efd8e7f86bb235d22a56acc160a23c9b80da..710c565c7fb386e447e465613a17be552358854b 100644 (file)
--- a/folly/tracing/test/StaticTracepointTest.cpp
+++ b/folly/tracing/test/StaticTracepointTest.cpp
@@ -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());