Refactor command execution (#986)

fixes #970 --------- Co-authored-by: Ken Matsui <[email protected]>
poac-dev · Sep 25, 2024 · 6e5cdda · 6e5cdda
1 parent 30acce3
commit 6e5cdda
Show file tree

Hide file tree

Showing 14 changed files with 336 additions and 140 deletions.
diff --git a/Makefile b/Makefile
@@ -94,10 +94,10 @@ $(O)/tests/test_BuildConfig: $(O)/tests/test_BuildConfig.o $(O)/Algos.o \
   $(O)/TermColor.o $(O)/Manifest.o $(O)/Parallelism.o $(O)/Semver.o \
   $(O)/VersionReq.o $(O)/Git2/Repository.o $(O)/Git2/Object.o $(O)/Git2/Oid.o \
   $(O)/Git2/Global.o $(O)/Git2/Config.o $(O)/Git2/Exception.o $(O)/Git2/Time.o \
-  $(O)/Git2/Commit.o
+  $(O)/Git2/Commit.o $(O)/Command.o
 	$(CXX) $(CXXFLAGS) $^ $(LIBS) $(LDFLAGS) -o $@
 
-$(O)/tests/test_Algos: $(O)/tests/test_Algos.o $(O)/TermColor.o
+$(O)/tests/test_Algos: $(O)/tests/test_Algos.o $(O)/TermColor.o $(O)/Command.o
 	$(CXX) $(CXXFLAGS) $^ $(LIBS) $(LDFLAGS) -o $@
 
 $(O)/tests/test_Semver: $(O)/tests/test_Semver.o $(O)/TermColor.o
@@ -110,7 +110,7 @@ $(O)/tests/test_VersionReq: $(O)/tests/test_VersionReq.o $(O)/TermColor.o \
 $(O)/tests/test_Manifest: $(O)/tests/test_Manifest.o $(O)/TermColor.o \
   $(O)/Semver.o $(O)/VersionReq.o $(O)/Algos.o $(O)/Git2/Repository.o \
   $(O)/Git2/Global.o $(O)/Git2/Oid.o $(O)/Git2/Config.o $(O)/Git2/Exception.o \
-  $(O)/Git2/Object.o
+  $(O)/Git2/Object.o $(O)/Command.o
 	$(CXX) $(CXXFLAGS) $^ $(LIBS) $(LDFLAGS) -o $@
 
 

diff --git a/src/Algos.cc b/src/Algos.cc
@@ -1,5 +1,6 @@
 #include "Algos.hpp"
 
+#include "Command.hpp"
 #include "Exception.hpp"
 #include "Logger.hpp"
 #include "Rustify.hpp"
@@ -39,48 +40,23 @@ toMacroName(const std::string_view name) noexcept {
 }
 
 int
-execCmd(const std::string_view cmd) noexcept {
+execCmd(const Command& cmd) noexcept {
   logger::debug("Running `", cmd, '`');
-  const int status = std::system(cmd.data());
-  const int exitCode = status >> 8;
-  return exitCode;
-}
-
-static std::pair<std::string, int>
-getCmdOutputImpl(const std::string_view cmd) {
-  constexpr usize bufferSize = 128;
-  std::array<char, bufferSize> buffer{};
-  std::string output;
-
-  FILE* pipe = popen(cmd.data(), "r");
-  if (!pipe) {
-    throw PoacError("popen() failed!");
-  }
-
-  while (fgets(buffer.data(), buffer.size(), pipe) != nullptr) {
-    output += buffer.data();
-  }
-
-  const int status = pclose(pipe);
-  if (status == -1) {
-    throw PoacError("pclose() failed!");
-  }
-  const int exitCode = status >> 8;
-  return { output, exitCode };
+  return cmd.spawn().wait();
 }
 
 std::string
-getCmdOutput(const std::string_view cmd, const usize retry) {
+getCmdOutput(const Command& cmd, const usize retry) {
   logger::debug("Running `", cmd, '`');
 
   int exitCode = EXIT_SUCCESS;
   int waitTime = 1;
   for (usize i = 0; i < retry; ++i) {
-    const auto result = getCmdOutputImpl(cmd);
-    if (result.second == EXIT_SUCCESS) {
-      return result.first;
+    const auto [output, status] = cmd.output();
+    if (status == EXIT_SUCCESS) {
+      return output;
     }
-    exitCode = result.second;
+    exitCode = status;
 
     // Sleep for an exponential backoff.
     std::this_thread::sleep_for(std::chrono::seconds(waitTime));
@@ -91,10 +67,12 @@ getCmdOutput(const std::string_view cmd, const usize retry) {
 
 bool
 commandExists(const std::string_view cmd) noexcept {
-  std::string checkCmd = "command -v ";
-  checkCmd += cmd;
-  checkCmd += " >/dev/null 2>&1";
-  return execCmd(checkCmd) == EXIT_SUCCESS;
+  const int exitCode = Command("which")
+                           .addArg(cmd)
+                           .setStdoutConfig(Command::StdioConfig::Null)
+                           .spawn()
+                           .wait();
+  return exitCode == EXIT_SUCCESS;
 }
 
 // ref: https://wandbox.org/permlink/zRjT41alOHdwcf00

diff --git a/src/Algos.hpp b/src/Algos.hpp
@@ -1,5 +1,6 @@
 #pragma once
 
+#include "Command.hpp"
 #include "Exception.hpp"
 #include "Rustify.hpp"
 
@@ -20,8 +21,8 @@
 std::string toUpper(std::string_view str) noexcept;
 std::string toMacroName(std::string_view name) noexcept;
 
-int execCmd(std::string_view cmd) noexcept;
-std::string getCmdOutput(std::string_view cmd, usize retry = 3);
+int execCmd(const Command& cmd) noexcept;
+std::string getCmdOutput(const Command& cmd, usize retry = 3);
 bool commandExists(std::string_view cmd) noexcept;
 
 template <typename T>

diff --git a/src/BuildConfig.cc b/src/BuildConfig.cc
@@ -110,10 +110,10 @@ struct BuildConfig {
 
   std::string OUT_DIR;
   std::string CXX = "clang++";
-  std::string CXXFLAGS;
-  std::string DEFINES;
-  std::string INCLUDES = " -I../../include";
-  std::string LIBS;
+  std::vector<std::string> CXXFLAGS;
+  std::vector<std::string> DEFINES;
+  std::vector<std::string> INCLUDES = { "-I../../include" };
+  std::vector<std::string> LIBS;
 
   BuildConfig() = default;
   explicit BuildConfig(const std::string& packageName)
@@ -334,21 +334,20 @@ BuildConfig::emitCompdb(const std::string_view baseDir, std::ostream& os)
     const std::string file = targetInfo.sourceFile.value();
     // The output is the target.
     const std::string output = target;
-    std::string cmd = CXX;
-    cmd += ' ';
-    cmd += CXXFLAGS;
-    cmd += DEFINES;
-    cmd += INCLUDES;
-    cmd += " -c ";
-    cmd += file;
-    cmd += " -o ";
-    cmd += output;
+    const Command cmd = Command(CXX)
+                            .addArgs(CXXFLAGS)
+                            .addArgs(DEFINES)
+                            .addArgs(INCLUDES)
+                            .addArg("-c")
+                            .addArg(file)
+                            .addArg("-o")
+                            .addArg(output);
 
     oss << indent1 << "{\n";
     oss << indent2 << "\"directory\": " << baseDirPath << ",\n";
     oss << indent2 << "\"file\": " << std::quoted(file) << ",\n";
     oss << indent2 << "\"output\": " << std::quoted(output) << ",\n";
-    oss << indent2 << "\"command\": " << std::quoted(cmd) << "\n";
+    oss << indent2 << "\"command\": " << std::quoted(cmd.to_string()) << "\n";
     oss << indent1 << "},\n";
   }
 
@@ -366,18 +365,14 @@ BuildConfig::emitCompdb(const std::string_view baseDir, std::ostream& os)
 
 std::string
 BuildConfig::runMM(const std::string& sourceFile, const bool isTest) const {
-  std::string command = "cd ";
-  command += getOutDir();
-  command += " && ";
-  command += CXX;
-  command += CXXFLAGS;
-  command += DEFINES;
-  command += INCLUDES;
+  Command command =
+      Command(CXX).addArgs(CXXFLAGS).addArgs(DEFINES).addArgs(INCLUDES);
   if (isTest) {
-    command += " -DPOAC_TEST";
+    command.addArg("-DPOAC_TEST");
   }
-  command += " -MM ";
-  command += sourceFile;
+  command.addArg("-MM");
+  command.addArg(sourceFile);
+  command.setWorkingDirectory(getOutDir());
   return getCmdOutput(command);
 }
 
@@ -430,17 +425,16 @@ BuildConfig::containsTestCode(const std::string& sourceFile) const {
   while (std::getline(ifs, line)) {
     if (line.find("POAC_TEST") != std::string::npos) {
       // TODO: Can't we somehow elegantly make the compiler command sharable?
-      std::string command = CXX;
-      command += " -E ";
-      command += CXXFLAGS;
-      command += DEFINES;
-      command += INCLUDES;
-      command += ' ';
-      command += sourceFile;
+      Command command(CXX);
+      command.addArg("-E");
+      command.addArgs(CXXFLAGS);
+      command.addArgs(DEFINES);
+      command.addArgs(INCLUDES);
+      command.addArg(sourceFile);
 
       const std::string src = getCmdOutput(command);
 
-      command += " -DPOAC_TEST";
+      command.addArg("-DPOAC_TEST");
       const std::string testSrc = getCmdOutput(command);
 
       // If the source file contains POAC_TEST, by processing the source
@@ -570,42 +564,48 @@ void
 BuildConfig::installDeps(const bool includeDevDeps) {
   const std::vector<DepMetadata> deps = installDependencies(includeDevDeps);
   for (const DepMetadata& dep : deps) {
-    INCLUDES += ' ' + dep.includes;
-    LIBS += ' ' + dep.libs;
+    if (!dep.includes.empty())
+      INCLUDES.push_back(dep.includes);
+    if (!dep.libs.empty())
+      LIBS.push_back(dep.libs);
   }
-  logger::debug("INCLUDES: ", INCLUDES);
-  logger::debug("LIBS: ", LIBS);
+  logger::debug(fmt::format("INCLUDES: {}", INCLUDES));
+  logger::debug(fmt::format("LIBS: {}", LIBS));
 }
 
 void
 BuildConfig::addDefine(
     const std::string_view name, const std::string_view value
 ) {
-  DEFINES += fmt::format(" -D{}='\"{}\"'", name, value);
+  DEFINES.push_back(fmt::format("-D{}='\"{}\"'", name, value));
 }
 
 void
 BuildConfig::setVariables(const bool isDebug) {
   this->defineCondVar("CXX", CXX);
 
-  CXXFLAGS += " -std=c++" + getPackageEdition().getString();
+  CXXFLAGS.push_back("-std=c++" + getPackageEdition().getString());
   if (shouldColor()) {
-    CXXFLAGS += " -fdiagnostics-color";
+    CXXFLAGS.push_back("-fdiagnostics-color");
   }
   if (isDebug) {
-    CXXFLAGS += " -g -O0 -DDEBUG";
+    CXXFLAGS.push_back("-g");
+    CXXFLAGS.push_back("-O0");
+    CXXFLAGS.push_back("-DDEBUG");
   } else {
-    CXXFLAGS += " -O3 -DNDEBUG";
+    CXXFLAGS.push_back("-O3");
+    CXXFLAGS.push_back("-DNDEBUG");
   }
   const Profile& profile = isDebug ? getDevProfile() : getReleaseProfile();
   if (profile.lto) {
-    CXXFLAGS += " -flto";
+    CXXFLAGS.push_back("-flto");
   }
   for (const std::string_view flag : profile.cxxflags) {
-    CXXFLAGS += ' ';
-    CXXFLAGS += flag;
+    CXXFLAGS.emplace_back(flag);
   }
-  this->defineSimpleVar("CXXFLAGS", CXXFLAGS);
+  this->defineSimpleVar(
+      "CXXFLAGS", fmt::format("{:s}", fmt::join(CXXFLAGS, " "))
+  );
 
   const std::string pkgName = toMacroName(this->packageName);
   const Version& pkgVersion = getPackageVersion();
@@ -646,9 +646,13 @@ BuildConfig::setVariables(const bool isDebug) {
     addDefine(key, val);
   }
 
-  this->defineSimpleVar("DEFINES", DEFINES);
-  this->defineSimpleVar("INCLUDES", INCLUDES);
-  this->defineSimpleVar("LIBS", LIBS);
+  this->defineSimpleVar(
+      "DEFINES", fmt::format("{:s}", fmt::join(DEFINES, " "))
+  );
+  this->defineSimpleVar(
+      "INCLUDES", fmt::format("{:s}", fmt::join(INCLUDES, " "))
+  );
+  this->defineSimpleVar("LIBS", fmt::format("{:s}", fmt::join(LIBS, " ")));
 }
 
 void
@@ -956,18 +960,16 @@ modeToProfile(const bool isDebug) {
   return isDebug ? "dev" : "release";
 }
 
-std::string
+Command
 getMakeCommand() {
-  std::string makeCommand;
-  if (isVerbose()) {
-    makeCommand = "make";
-  } else {
-    makeCommand = "make -s --no-print-directory Q=@";
+  Command makeCommand("make");
+  if (!isVerbose()) {
+    makeCommand.addArg("-s").addArg("--no-print-directory").addArg("Q=@");
   }
 
   const usize numThreads = getParallelism();
   if (numThreads > 1) {
-    makeCommand += " -j" + std::to_string(numThreads);
+    makeCommand.addArg("-j" + std::to_string(numThreads));
   }
 
   return makeCommand;

diff --git a/src/BuildConfig.hpp b/src/BuildConfig.hpp
@@ -1,5 +1,7 @@
 #pragma once
 
+#include "Command.hpp"
+
 #include <string>
 #include <unordered_set>
 
@@ -16,4 +18,4 @@ std::string emitMakefile(bool isDebug, bool includeDevDeps);
 std::string emitCompdb(bool isDebug, bool includeDevDeps);
 std::string_view modeToString(bool isDebug);
 std::string_view modeToProfile(bool isDebug);
-std::string getMakeCommand();
+Command getMakeCommand();
diff --git a/src/Cmd/Build.cc b/src/Cmd/Build.cc
@@ -35,7 +35,7 @@ buildImpl(std::string& outDir, const bool isDebug) {
   const auto start = std::chrono::steady_clock::now();
 
   outDir = emitMakefile(isDebug, /*includeDevDeps=*/false);
-  const std::string makeCommand = getMakeCommand() + " -C " + outDir;
+  const Command makeCommand = getMakeCommand().addArg("-C").addArg(outDir);
   const int exitCode = execCmd(makeCommand);
 
   const auto end = std::chrono::steady_clock::now();