From 59a8ec41ba1de2722406a437eb241d19128114ea Mon Sep 17 00:00:00 2001 From: TheAssassin Date: Fri, 7 Aug 2020 15:39:32 +0200 Subject: [PATCH] Replace cpp-subprocess with own linuxdeploy-subprocess --- .gitmodules | 3 -- CMakeLists.txt | 5 -- include/linuxdeploy/plugin/base_impl.h | 57 ++++++++++------------ include/linuxdeploy/util/subprocess.h | 19 -------- include/linuxdeploy/util/util.h | 5 +- lib/CMakeLists.txt | 5 -- lib/cpp-subprocess | 1 - src/CMakeLists.txt | 3 +- src/core/appdir.cpp | 17 +++---- src/core/copyright/copyright_dpkgquery.cpp | 20 ++------ src/core/copyright/copyright_dpkgquery.h | 3 -- src/core/elf.cpp | 53 ++++++++------------ src/plugin/CMakeLists.txt | 2 +- src/plugin/plugin.cpp | 1 - src/plugin/plugin_type0.cpp | 1 - src/util/CMakeLists.txt | 9 ++-- src/util/subprocess.cpp | 56 --------------------- 17 files changed, 69 insertions(+), 191 deletions(-) delete mode 100644 include/linuxdeploy/util/subprocess.h delete mode 160000 lib/cpp-subprocess delete mode 100644 src/util/subprocess.cpp diff --git a/.gitmodules b/.gitmodules index 6bd501f..e5b7228 100644 --- a/.gitmodules +++ b/.gitmodules @@ -1,6 +1,3 @@ -[submodule "lib/cpp-subprocess"] - path = lib/cpp-subprocess - url = https://github.com/arun11299/cpp-subprocess.git [submodule "lib/args"] path = lib/args url = https://github.com/Taywee/args.git diff --git a/CMakeLists.txt b/CMakeLists.txt index ff6d154..4962760 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -10,11 +10,6 @@ set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} "${PROJECT_SOURCE_DIR}/cmake/Modules/ set(USE_SYSTEM_BOOST OFF CACHE BOOL "Set to ON to use system boost libraries instead of building up to date boost libraries from source") set(USE_SYSTEM_CIMG ON CACHE BOOL "Set to OFF to use CImg library bundled in lib directory") -if (EXISTS "${PROJECT_SOURCE_DIR}/lib/cpp-subprocess/subprocess.hpp") -else() - message (FATAL_ERROR "Missing submodule(s), please 'git submodule update --init --recursive'...") -endif() - # support for ccache # call CMake with -DUSE_CCACHE=ON to make use of it set(USE_CCACHE ON CACHE BOOL "") diff --git a/include/linuxdeploy/plugin/base_impl.h b/include/linuxdeploy/plugin/base_impl.h index 0efc3b5..bd6f064 100644 --- a/include/linuxdeploy/plugin/base_impl.h +++ b/include/linuxdeploy/plugin/base_impl.h @@ -2,16 +2,18 @@ #include #include #include +#include #include // library headers #include #include -#include +#include // local headers #include "linuxdeploy/core/log.h" #include "linuxdeploy/util/util.h" +#include "linuxdeploy/subprocess/process.h" #pragma once @@ -46,8 +48,8 @@ namespace linuxdeploy { private: int getApiLevelFromExecutable() { - auto output = subprocess::check_output({pluginPath.c_str(), "--plugin-api-version"}); - std::string stdoutOutput = output.buf.data(); + const subprocess::subprocess proc({pluginPath.string(), "--plugin-api-version"}); + const auto stdoutOutput = proc.check_output(); try { auto apiLevel = std::stoi(stdoutOutput); @@ -63,9 +65,8 @@ namespace linuxdeploy { // check whether plugin implements --plugin-type try { - auto output = subprocess::check_output({pluginPath.c_str(), "--plugin-type"}); - - std::string stdoutOutput = output.buf.data(); + const subprocess::subprocess proc({pluginPath.c_str(), "--plugin-type"}); + const auto stdoutOutput = proc.check_output(); // the specification requires a single line, but we'll silently accept more than that, too if (std::count(stdoutOutput.begin(), stdoutOutput.end(), '\n') >= 1) { @@ -76,7 +77,7 @@ namespace linuxdeploy { else if (firstLine == "output") type = OUTPUT_TYPE; } - } catch (const subprocess::CalledProcessError&) {} + } catch (const std::logic_error&) {} return type; } @@ -127,8 +128,8 @@ namespace linuxdeploy { template int PluginBase::run(const boost::filesystem::path& appDirPath) { - auto pluginPath = path(); - auto args = {pluginPath.c_str(), "--appdir", appDirPath.string().c_str()}; + const auto pluginPath = path(); + const std::initializer_list args = {pluginPath.string(), "--appdir", appDirPath.string()}; auto log = ldLog(); log << "Running process:"; @@ -137,7 +138,7 @@ namespace linuxdeploy { } log << std::endl; - std::map environmentVariables{}; + subprocess::subprocess_env_map_t environmentVariables{}; if (this->pluginType() == PLUGIN_TYPE::INPUT_TYPE) { // add $LINUXDEPLOY, which points to the current binary @@ -146,27 +147,22 @@ namespace linuxdeploy { environmentVariables["LINUXDEPLOY"] = linuxdeploy::util::getOwnExecutablePath(); } - auto process = subprocess::Popen( - args, - subprocess::output{subprocess::PIPE}, - subprocess::error{subprocess::PIPE}, - subprocess::environment{environmentVariables} - ); + subprocess::process process(args, environmentVariables); std::vector pfds(2); auto* opfd = &pfds[0]; auto* epfd = &pfds[1]; - opfd->fd = fileno(process.output()); + opfd->fd = process.stdout_fd(); opfd->events = POLLIN; - epfd->fd = fileno(process.error()); + epfd->fd = process.stderr_fd(); epfd->events = POLLIN; - for (const auto& fd : {process.output(), process.error()}) { - auto flags = fcntl(fileno(fd), F_GETFL, 0); + for (const auto& fd : {process.stdout_fd(), process.stderr_fd()}) { + auto flags = fcntl(fd, F_GETFL, 0); flags |= O_NONBLOCK; - fcntl(fileno(fd), F_SETFL, flags); + fcntl(fd, F_SETFL, flags); } auto printOutput = [&pfds, opfd, epfd, this, &process]() { @@ -207,11 +203,10 @@ namespace linuxdeploy { throw std::runtime_error("Buffer overflow"); while (true) { - auto bytesRead = fread( + auto bytesRead = read( + process.stdout_fd(), reinterpret_cast(stdoutBuf.data() + stdoutBufSize), - sizeof(char), - static_cast(stdoutBuf.size() - stdoutBufSize), - process.output() + static_cast(stdoutBuf.size() - stdoutBufSize) ); if (bytesRead == 0) @@ -230,11 +225,10 @@ namespace linuxdeploy { throw std::runtime_error("Buffer overflow"); while (true) { - auto bytesRead = fread( + auto bytesRead = read( + process.stderr_fd(), reinterpret_cast(stderrBuf.data() + stderrBufSize), - sizeof(char), - static_cast(stderrBuf.size() - stderrBufSize), - process.error() + static_cast(stderrBuf.size() - stderrBufSize) ); if (bytesRead == 0) @@ -251,15 +245,13 @@ namespace linuxdeploy { return true; }; - int retcode; - do { if (!printOutput()) { ldLog() << LD_ERROR << "Failed to communicate with process" << std::endl; process.kill(); return -1; } - } while ((retcode = process.poll()) < 0); + } while (process.poll()); if (!printOutput()) { ldLog() << LD_ERROR << "Failed to communicate with process" << std::endl; @@ -267,6 +259,7 @@ namespace linuxdeploy { return -1; } + const auto retcode = process.close(); return retcode; } } diff --git a/include/linuxdeploy/util/subprocess.h b/include/linuxdeploy/util/subprocess.h deleted file mode 100644 index ceeff60..0000000 --- a/include/linuxdeploy/util/subprocess.h +++ /dev/null @@ -1,19 +0,0 @@ -/** - * Wrapper for cpp-subprocess. Provides additional convenience functions. - */ - -#pragma once - -// library includes -#include - -namespace linuxdeploy { - namespace util { - namespace subprocess { - using namespace ::subprocess; - - // Reads output channels of existing Popen object into buffers and returns the contents - std::pair check_output_error(Popen& proc); - } - } -} diff --git a/include/linuxdeploy/util/util.h b/include/linuxdeploy/util/util.h index 32bf521..9113abe 100644 --- a/include/linuxdeploy/util/util.h +++ b/include/linuxdeploy/util/util.h @@ -1,10 +1,11 @@ // local includes -#include "misc.h" -#include "subprocess.h" +#include "linuxdeploy/util/assert.h" +#include "linuxdeploy/util/misc.h" // import functions from misc module for convenience namespace linuxdeploy { namespace util { using namespace misc; + using namespace assert; } } diff --git a/lib/CMakeLists.txt b/lib/CMakeLists.txt index 1c36aa3..df8b55d 100644 --- a/lib/CMakeLists.txt +++ b/lib/CMakeLists.txt @@ -1,10 +1,5 @@ include(CTest) -add_library(subprocess INTERFACE) -target_sources(subprocess INTERFACE ${CMAKE_CURRENT_SOURCE_DIR}/cpp-subprocess/subprocess.hpp) -target_include_directories(subprocess INTERFACE ${CMAKE_CURRENT_SOURCE_DIR}/cpp-subprocess) -target_compile_options(subprocess INTERFACE "-Wno-deprecated") - add_library(args INTERFACE) target_sources(args INTERFACE ${CMAKE_CURRENT_SOURCE_DIR}/args/args.hxx) target_include_directories(args INTERFACE args) diff --git a/lib/cpp-subprocess b/lib/cpp-subprocess deleted file mode 160000 index 6931e3d..0000000 --- a/lib/cpp-subprocess +++ /dev/null @@ -1 +0,0 @@ -Subproject commit 6931e3d69fb36e6eae099585646e54ac644bf99c diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 4563db2..6728cbc 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -43,6 +43,7 @@ execute_process( add_subdirectory(util) add_subdirectory(plugin) +add_subdirectory(subprocess) add_subdirectory(core) add_executable(linuxdeploy main.cpp core.cpp) @@ -65,7 +66,7 @@ add_executable(plugin_test plugin_test_main.cpp) target_link_libraries(plugin_test linuxdeploy_plugin) set_target_properties(plugin_test PROPERTIES RUNTIME_OUTPUT_DIRECTORY "${PROJECT_BINARY_DIR}/bin") -add_executable(appdir_test appdir_test_main.cpp) +add_executable(appdir_test appdir_test_main.cpp ../include/linuxdeploy/util/assert.h) target_link_libraries(appdir_test linuxdeploy_core args) target_include_directories(appdir_test PRIVATE ${CMAKE_CURRENT_BINARY_DIR}/core) set_target_properties(appdir_test PROPERTIES RUNTIME_OUTPUT_DIRECTORY "${PROJECT_BINARY_DIR}/bin") diff --git a/src/core/appdir.cpp b/src/core/appdir.cpp index 65e45dd..5e46a72 100644 --- a/src/core/appdir.cpp +++ b/src/core/appdir.cpp @@ -1,5 +1,6 @@ // system headers #include +#include #include #include @@ -7,7 +8,6 @@ #include #include #include -#include // local headers @@ -16,6 +16,7 @@ #include "linuxdeploy/core/log.h" #include "linuxdeploy/desktopfile/desktopfileentry.h" #include "linuxdeploy/util/util.h" +#include "linuxdeploy/subprocess/subprocess.h" #include "copyright.h" // auto-generated headers @@ -187,19 +188,15 @@ namespace linuxdeploy { } else { ldLog() << "Calling strip on library" << filePath << std::endl; - std::map env; + subprocess::subprocess_env_map_t env; env.insert(std::make_pair(std::string("LC_ALL"), std::string("C"))); - subprocess::Popen proc( - {stripPath.c_str(), filePath.c_str()}, - subprocess::output(subprocess::PIPE), - subprocess::error(subprocess::PIPE), - subprocess::environment(env) - ); + subprocess::subprocess proc({stripPath, filePath.string()}, env); - std::string err = util::subprocess::check_output_error(proc).second; + const auto result = proc.run(); + const auto& err = result.stderr_string(); - if (proc.retcode() != 0 && + if (result.exit_code() != 0 && !util::stringContains(err, "Not enough room for program headers")) { ldLog() << LD_ERROR << "Strip call failed:" << err << std::endl; success = false; diff --git a/src/core/copyright/copyright_dpkgquery.cpp b/src/core/copyright/copyright_dpkgquery.cpp index b6944d7..9073f04 100644 --- a/src/core/copyright/copyright_dpkgquery.cpp +++ b/src/core/copyright/copyright_dpkgquery.cpp @@ -2,6 +2,7 @@ #include "copyright_dpkgquery.h" #include "linuxdeploy/core/log.h" #include "linuxdeploy/util/util.h" +#include "linuxdeploy/subprocess/subprocess.h" namespace linuxdeploy { namespace core { @@ -9,27 +10,16 @@ namespace linuxdeploy { using namespace log; std::vector DpkgQueryCopyrightFilesManager::getCopyrightFilesForPath(const bf::path& path) { - auto call = [](const std::initializer_list& args) { - auto proc = subprocess::Popen( - args, - subprocess::output(subprocess::PIPE), - subprocess::error(subprocess::PIPE) - ); + subprocess::subprocess proc{{"dpkg-query", "-S", path.c_str()}}; - auto output = proc.communicate(); - return std::make_pair(proc.retcode(), output.first); - }; + auto result = proc.run(); - auto dpkgQueryPackages = call({"dpkg-query", "-S", path.c_str()}); - - if (dpkgQueryPackages.first != 0 - || dpkgQueryPackages.second.buf.empty() - || dpkgQueryPackages.second.buf.front() == '\0') { + if (result.exit_code() != 0 || result.stdout_contents().empty()) { ldLog() << LD_WARNING << "Could not find copyright files for file" << path << "using dpkg-query" << std::endl; return {}; } - auto packageName = util::split(util::splitLines(dpkgQueryPackages.second.buf.data())[0], ':')[0]; + auto packageName = util::split(util::splitLines(result.stdout_string())[0], ':')[0]; if (!packageName.empty()) { auto copyrightFilePath = bf::path("/usr/share/doc") / packageName / "copyright"; diff --git a/src/core/copyright/copyright_dpkgquery.h b/src/core/copyright/copyright_dpkgquery.h index d4ac700..55b6ad4 100644 --- a/src/core/copyright/copyright_dpkgquery.h +++ b/src/core/copyright/copyright_dpkgquery.h @@ -1,6 +1,3 @@ -// library includes -#include - // local includes #include "copyright.h" diff --git a/src/core/elf.cpp b/src/core/elf.cpp index 245c775..5ad459c 100644 --- a/src/core/elf.cpp +++ b/src/core/elf.cpp @@ -2,16 +2,17 @@ #include #include #include +#include // library includes #include -#include #include // local headers #include "linuxdeploy/core/elf.h" #include "linuxdeploy/core/log.h" #include "linuxdeploy/util/util.h" +#include "linuxdeploy/subprocess/subprocess.h" using namespace linuxdeploy::core::log; @@ -147,7 +148,7 @@ namespace linuxdeploy { std::vector paths; - std::map env; + subprocess::subprocess_env_map_t env; env.insert(std::make_pair(std::string("LC_ALL"), std::string("C"))); // workaround for https://sourceware.org/bugzilla/show_bug.cgi?id=25263 @@ -156,19 +157,14 @@ namespace linuxdeploy { // note that this is just a bug in ldd, the linker has always worked as intended const auto resolvedPath = bf::canonical(d->path); - subprocess::Popen lddProc( - {"ldd", resolvedPath.string().c_str()}, - subprocess::output{subprocess::PIPE}, - subprocess::error{subprocess::PIPE}, - subprocess::environment(env) - ); + subprocess::subprocess lddProc({"ldd", resolvedPath.string()}, env); - auto outputAndError = util::subprocess::check_output_error(lddProc); + const auto stdoutContents = lddProc.check_output(); const boost::regex expr(R"(\s*(.+)\s+\=>\s+(.+)\s+\((.+)\)\s*)"); boost::smatch what; - for (const auto& line : util::splitLines(outputAndError.first)) { + for (const auto& line : util::splitLines(stdoutContents)) { if (boost::regex_search(line, what, expr)) { auto libraryPath = what[2].str(); util::trim(libraryPath); @@ -195,30 +191,26 @@ namespace linuxdeploy { const auto patchelfPath = PrivateData::getPatchelfPath(); try { - subprocess::Popen patchelfProc( - {patchelfPath.c_str(), "--print-rpath", d->path.c_str()}, - subprocess::output(subprocess::PIPE), - subprocess::error(subprocess::PIPE) - ); + subprocess::subprocess patchelfProc({patchelfPath, "--print-rpath", d->path.string()}); - auto patchelfOutput = util::subprocess::check_output_error(patchelfProc); - auto& patchelfStdout = patchelfOutput.first; - auto& patchelfStderr = patchelfOutput.second; + const auto result = patchelfProc.run(); - if (patchelfProc.retcode() != 0) { + if (result.exit_code() != 0) { // if file is not an ELF executable, there is no need for a detailed error message - if (patchelfProc.retcode() == 1 && patchelfStderr.find("not an ELF executable")) { + if (result.exit_code() == 1 && result.stderr_string().find("not an ELF executable") != std::string::npos) { return ""; } else { - ldLog() << LD_ERROR << "Call to patchelf failed:" << std::endl << patchelfStderr; + ldLog() << LD_ERROR << "Call to patchelf failed:" << std::endl << result.stderr_string(); return ""; } } - util::trim(patchelfStdout, '\n'); - util::trim(patchelfStdout); + auto stdoutContents = result.stdout_string(); - return patchelfStdout; + util::trim(stdoutContents, '\n'); + util::trim(stdoutContents); + + return stdoutContents; } catch (const std::exception&) { return ""; } @@ -229,17 +221,12 @@ namespace linuxdeploy { const auto patchelfPath = PrivateData::getPatchelfPath(); try { - subprocess::Popen patchelfProc( - {patchelfPath.c_str(), "--set-rpath", value.c_str(), d->path.c_str()}, - subprocess::output(subprocess::PIPE), - subprocess::error(subprocess::PIPE) - ); + subprocess::subprocess patchelfProc({patchelfPath.c_str(), "--set-rpath", value.c_str(), d->path.c_str()}); - const auto patchelfOutput = util::subprocess::check_output_error(patchelfProc); - const auto& patchelfStderr = patchelfOutput.second; + const auto result = patchelfProc.run(); - if (patchelfProc.retcode() != 0) { - ldLog() << LD_ERROR << "Call to patchelf failed:" << std::endl << patchelfStderr << std::endl; + if (result.exit_code() != 0) { + ldLog() << LD_ERROR << "Call to patchelf failed:" << std::endl << result.stderr_string() << std::endl; return false; } } catch (const std::exception&) { diff --git a/src/plugin/CMakeLists.txt b/src/plugin/CMakeLists.txt index ad51629..7836f1e 100644 --- a/src/plugin/CMakeLists.txt +++ b/src/plugin/CMakeLists.txt @@ -1,4 +1,4 @@ file(GLOB PLUGIN_HEADERS ${PROJECT_SOURCE_DIR}/include/linuxdeploy/plugin/*.h) add_library(linuxdeploy_plugin STATIC plugin_type0.cpp plugin.cpp ${PLUGIN_HEADERS}) -target_link_libraries(linuxdeploy_plugin PUBLIC linuxdeploy_core ${BOOST_LIBS} subprocess) +target_link_libraries(linuxdeploy_plugin PUBLIC linuxdeploy_core ${BOOST_LIBS} linuxdeploy_subprocess) diff --git a/src/plugin/plugin.cpp b/src/plugin/plugin.cpp index 4b34943..9ca8beb 100644 --- a/src/plugin/plugin.cpp +++ b/src/plugin/plugin.cpp @@ -7,7 +7,6 @@ #include #include #include -#include // local headers #include "linuxdeploy/core/log.h" diff --git a/src/plugin/plugin_type0.cpp b/src/plugin/plugin_type0.cpp index 1462d5b..db04185 100644 --- a/src/plugin/plugin_type0.cpp +++ b/src/plugin/plugin_type0.cpp @@ -6,7 +6,6 @@ // library headers #include #include -#include // local headers #include "linuxdeploy/core/log.h" diff --git a/src/util/CMakeLists.txt b/src/util/CMakeLists.txt index d5c396e..7922032 100644 --- a/src/util/CMakeLists.txt +++ b/src/util/CMakeLists.txt @@ -1,7 +1,10 @@ cmake_minimum_required(VERSION 3.0) -file(GLOB HEADERS ${PROJECT_SOURCE_DIR}/include/linuxdeploy/util/*.h) +set(headers_dir ${PROJECT_SOURCE_DIR}/include/linuxdeploy/util/) -add_library(linuxdeploy_util STATIC subprocess.cpp ${HEADERS}) +add_library(linuxdeploy_util INTERFACE) +target_sources(linuxdeploy_util INTERFACE + ${headers_dir}/misc.h + ${headers_dir}/util.h +) target_include_directories(linuxdeploy_util INTERFACE ${CMAKE_CURRENT_SOURCE_DIR} ${PROJECT_SOURCE_DIR}/include) -target_link_libraries(linuxdeploy_util PUBLIC subprocess) diff --git a/src/util/subprocess.cpp b/src/util/subprocess.cpp deleted file mode 100644 index 3416260..0000000 --- a/src/util/subprocess.cpp +++ /dev/null @@ -1,56 +0,0 @@ -// global headers -#include -#include -#include - -// local headers -#include "linuxdeploy/util/subprocess.h" - -namespace linuxdeploy { - namespace util { - namespace subprocess { - std::pair check_output_error(Popen& proc) { - std::vector procStdout, procStderr; - - // make file descriptors non-blocking - for (const auto& fd : {proc.output(), proc.error()}) { - auto flags = fcntl(fileno(fd), F_GETFL, 0); - flags |= O_NONBLOCK; - fcntl(fileno(fd), F_SETFL, flags); - } - - while (true) { - constexpr auto bufSize = 4096; - std::vector buf(bufSize, '\0'); - - for (auto& fd : {proc.output(), proc.error()}) { - auto& outBuf = fd == proc.output() ? procStdout : procStderr; - - auto size = fread(buf.data(), sizeof(char), buf.size(), fd); - - if (size < 0) - throw std::runtime_error("Error reading fd"); - - if (size == 0) - continue; - - assert(size <= buf.size()); - - auto outBufSize = outBuf.size(); - outBuf.reserve(outBufSize + size + 1); - std::copy(buf.begin(), buf.begin() + size, std::back_inserter(outBuf)); - } - - // make sure process exited and all data has been read from stdout and stderr - if (proc.poll() >= 0 && feof(proc.output()) != 0 && feof(proc.error()) != 0) - break; - } - - std::string stdoutContents(procStdout.begin(), procStdout.end()); - std::string stderrContents(procStderr.begin(), procStderr.end()); - - return std::make_pair(stdoutContents, stderrContents); - } - } - } -}