From b98a57bad7ba2d409938453a0d7640d199d38802 Mon Sep 17 00:00:00 2001 From: Joel Rosdahl Date: Thu, 24 Jul 2025 13:18:40 +0200 Subject: [PATCH] refactor: Extract command execution code from hashutil to util function --- src/ccache/core/exceptions.hpp | 11 --- src/ccache/execute.cpp | 11 +++ src/ccache/hashutil.cpp | 117 ++--------------------- src/ccache/util/CMakeLists.txt | 1 + src/ccache/util/exec.cpp | 163 +++++++++++++++++++++++++++++++++ src/ccache/util/exec.hpp | 33 +++++++ unittest/CMakeLists.txt | 1 + unittest/test_util_exec.cpp | 62 +++++++++++++ 8 files changed, 277 insertions(+), 122 deletions(-) create mode 100644 src/ccache/util/exec.cpp create mode 100644 src/ccache/util/exec.hpp create mode 100644 unittest/test_util_exec.cpp diff --git a/src/ccache/core/exceptions.hpp b/src/ccache/core/exceptions.hpp index da0e2880..2fc4dee4 100644 --- a/src/ccache/core/exceptions.hpp +++ b/src/ccache/core/exceptions.hpp @@ -49,15 +49,4 @@ class Fatal : public ErrorBase using ErrorBase::ErrorBase; }; -// Call a libc-style function (returns 0 on success and sets errno) and throw -// Fatal on error. -#define CHECK_LIB_CALL(function, ...) \ - { \ - int _result = function(__VA_ARGS__); \ - if (_result != 0) { \ - throw core::Fatal(FMT(#function " failed: {}", strerror(_result))); \ - } \ - } \ - static_assert(true) /* allow semicolon after macro */ - } // namespace core diff --git a/src/ccache/execute.cpp b/src/ccache/execute.cpp index 966252c5..84d2c6d1 100644 --- a/src/ccache/execute.cpp +++ b/src/ccache/execute.cpp @@ -57,6 +57,17 @@ # include // NOLINT: sigaddset et al are defined in signal.h #endif +// Call a libc-style function (returns 0 on success and sets errno) and throw +// Fatal on error. +#define CHECK_LIB_CALL(function, ...) \ + { \ + int _result = function(__VA_ARGS__); \ + if (_result != 0) { \ + throw core::Fatal(FMT(#function " failed: {}", strerror(_result))); \ + } \ + } \ + static_assert(true) /* allow semicolon after macro */ + namespace fs = util::filesystem; #ifdef _WIN32 diff --git a/src/ccache/hashutil.cpp b/src/ccache/hashutil.cpp index 7b56f388..df907b89 100644 --- a/src/ccache/hashutil.cpp +++ b/src/ccache/hashutil.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #include #include #include @@ -34,24 +35,11 @@ #include #include #include -#include #ifdef INODE_CACHE_SUPPORTED # include #endif -#ifdef HAVE_SPAWN_H -# include -#endif - -#ifdef HAVE_UNISTD_H -# include -#endif - -#ifdef HAVE_SYS_WAIT_H -# include -#endif - #ifdef HAVE_AVX2 # include #endif @@ -357,107 +345,14 @@ hash_command_output(Hash& hash, } } - auto arg_vector = args.to_argv(); - auto argv = const_cast(arg_vector.data()); - LOG("Executing compiler check command {}", - util::format_argv_for_logging(argv)); - -#ifdef _WIN32 - PROCESS_INFORMATION pi; - memset(&pi, 0x00, sizeof(pi)); - STARTUPINFO si; - memset(&si, 0x00, sizeof(si)); - - si.cb = sizeof(STARTUPINFO); - - HANDLE pipe_out[2]; - SECURITY_ATTRIBUTES sa = {sizeof(SECURITY_ATTRIBUTES), nullptr, TRUE}; - CreatePipe(&pipe_out[0], &pipe_out[1], &sa, 0); - SetHandleInformation(pipe_out[0], HANDLE_FLAG_INHERIT, 0); - si.hStdOutput = pipe_out[1]; - si.hStdError = pipe_out[1]; - si.hStdInput = GetStdHandle(STD_INPUT_HANDLE); - si.dwFlags = STARTF_USESTDHANDLES; - - std::string commandline; - commandline = util::format_argv_as_win32_command_string(argv); - BOOL ret = CreateProcess(nullptr, - const_cast(commandline.c_str()), - nullptr, - nullptr, - 1, - 0, - nullptr, - nullptr, - &si, - &pi); - CloseHandle(pipe_out[1]); - if (ret == 0) { + auto result = util::exec_to_string(args); + if (!result) { + LOG("Error executing compiler check command: {}", result.error()); return false; } - int fd = _open_osfhandle((intptr_t)pipe_out[0], O_BINARY); - const auto compiler_check_result = hash.hash_fd(fd); - if (!compiler_check_result) { - LOG("Error hashing compiler check command output: {}", - compiler_check_result.error()); - } - WaitForSingleObject(pi.hProcess, INFINITE); - DWORD exitcode; - GetExitCodeProcess(pi.hProcess, &exitcode); - CloseHandle(pipe_out[0]); - CloseHandle(pi.hProcess); - CloseHandle(pi.hThread); - if (exitcode != 0) { - LOG("Compiler check command returned {}", exitcode); - return false; - } - return bool(compiler_check_result); -#else - int pipefd[2]; - if (pipe(pipefd) == -1) { - throw core::Fatal(FMT("pipe failed: {}", strerror(errno))); - } - - posix_spawn_file_actions_t fa; - - CHECK_LIB_CALL(posix_spawn_file_actions_init, &fa); - CHECK_LIB_CALL(posix_spawn_file_actions_addclose, &fa, pipefd[0]); - CHECK_LIB_CALL(posix_spawn_file_actions_addclose, &fa, 0); - CHECK_LIB_CALL(posix_spawn_file_actions_adddup2, &fa, pipefd[1], 1); - CHECK_LIB_CALL(posix_spawn_file_actions_adddup2, &fa, pipefd[1], 2); - pid_t pid; - extern char** environ; - int result = posix_spawnp(&pid, argv[0], &fa, nullptr, argv, environ); - - posix_spawn_file_actions_destroy(&fa); - close(pipefd[1]); - - const auto hash_result = hash.hash_fd(pipefd[0]); - if (!hash_result) { - LOG("Error hashing compiler check command output: {}", hash_result.error()); - } - close(pipefd[0]); - - if (result != 0) { - LOG("posix_spawnp failed: {}", strerror(errno)); - return false; - } - - int status; - while ((result = waitpid(pid, &status, 0)) != pid) { - if (result == -1 && errno == EINTR) { - continue; - } - LOG("waitpid failed: {}", strerror(errno)); - return false; - } - if (!WIFEXITED(status) || WEXITSTATUS(status) != 0) { - LOG("Compiler check command returned {}", WEXITSTATUS(status)); - return false; - } - return bool(hash_result); -#endif + hash.hash(*result); + return true; } bool diff --git a/src/ccache/util/CMakeLists.txt b/src/ccache/util/CMakeLists.txt index ba2f7632..b06df0b6 100644 --- a/src/ccache/util/CMakeLists.txt +++ b/src/ccache/util/CMakeLists.txt @@ -8,6 +8,7 @@ set( direntry.cpp environment.cpp error.cpp + exec.cpp file.cpp filelock.cpp filesystem.cpp diff --git a/src/ccache/util/exec.cpp b/src/ccache/util/exec.cpp new file mode 100644 index 00000000..11cded4d --- /dev/null +++ b/src/ccache/util/exec.cpp @@ -0,0 +1,163 @@ +// Copyright (C) 2025 Joel Rosdahl and other contributors +// +// See doc/AUTHORS.adoc for a complete list of contributors. +// +// This program is free software; you can redistribute it and/or modify it +// under the terms of the GNU General Public License as published by the Free +// Software Foundation; either version 3 of the License, or (at your option) +// any later version. +// +// This program is distributed in the hope that it will be useful, but WITHOUT +// ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +// FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +// more details. +// +// You should have received a copy of the GNU General Public License along with +// this program; if not, write to the Free Software Foundation, Inc., 51 +// Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + +#include "exec.hpp" + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#ifdef HAVE_SPAWN_H +# include +#endif + +#ifdef HAVE_SYS_WAIT_H +# include +#endif + +#ifndef environ +DLLIMPORT extern char** environ; +#endif + +namespace fs = util::filesystem; + +// Call a libc-style function (returns 0 on success and sets errno) and return +// tl::unexpected on error. +#define CHECK_LIB_CALL(function, ...) \ + { \ + int _result = function(__VA_ARGS__); \ + if (_result != 0) { \ + return tl::unexpected(FMT(#function " failed: {}", strerror(_result))); \ + } \ + } \ + static_assert(true) /* allow semicolon after macro */ + +namespace util { + +tl::expected +exec_to_string(const Args& args) +{ + auto argv = args.to_argv(); + LOG("Executing command: {}", format_argv_for_logging(argv.data())); + + std::string output; + +#ifdef _WIN32 + PROCESS_INFORMATION pi; + memset(&pi, 0x00, sizeof(pi)); + STARTUPINFO si; + memset(&si, 0x00, sizeof(si)); + + si.cb = sizeof(STARTUPINFO); + + HANDLE read_handle; + HANDLE write_handle; + SECURITY_ATTRIBUTES sa = {sizeof(SECURITY_ATTRIBUTES), nullptr, TRUE}; + CreatePipe(&read_handle, &write_handle, &sa, 0); + SetHandleInformation(read_handle, HANDLE_FLAG_INHERIT, 0); + si.hStdOutput = write_handle; + si.hStdError = write_handle; + si.hStdInput = nullptr; + si.dwFlags = STARTF_USESTDHANDLES; + + std::string commandline = format_argv_as_win32_command_string(argv.data()); + BOOL ret = CreateProcess(nullptr, + const_cast(commandline.c_str()), + nullptr, + nullptr, + 1, // inherit handles + 0, // no console window + nullptr, + nullptr, + &si, + &pi); + CloseHandle(write_handle); + if (ret == 0) { + CloseHandle(read_handle); + DWORD error = GetLastError(); + return tl::unexpected( + FMT("CreateProcess failure: {} ({})", win32_error_message(error), error)); + } + char buffer[4096]; + DWORD bytes_read = 0; + while (ReadFile(read_handle, buffer, sizeof(buffer), &bytes_read, nullptr) + && bytes_read > 0) { + output.append(buffer, bytes_read); + } + + CloseHandle(read_handle); + WaitForSingleObject(pi.hProcess, INFINITE); + + DWORD exitcode; + GetExitCodeProcess(pi.hProcess, &exitcode); + CloseHandle(pi.hProcess); + CloseHandle(pi.hThread); + if (exitcode != 0) { + return tl::unexpected(FMT("Non-zero exit code: {}", exitcode)); + } +#else + int pipefd[2]; + CHECK_LIB_CALL(pipe, pipefd); + + posix_spawn_file_actions_t fa; + CHECK_LIB_CALL(posix_spawn_file_actions_init, &fa); + CHECK_LIB_CALL(posix_spawn_file_actions_addclose, &fa, pipefd[0]); + CHECK_LIB_CALL(posix_spawn_file_actions_addclose, &fa, 0); + CHECK_LIB_CALL(posix_spawn_file_actions_adddup2, &fa, pipefd[1], 1); + CHECK_LIB_CALL(posix_spawn_file_actions_adddup2, &fa, pipefd[1], 2); + + pid_t pid; + auto argv_mutable = const_cast(argv.data()); + int result = posix_spawnp(&pid, argv[0], &fa, nullptr, argv_mutable, environ); + int saved_errno = errno; + posix_spawn_file_actions_destroy(&fa); + close(pipefd[1]); + + if (result != 0) { + close(pipefd[0]); + return tl::unexpected( + FMT("posix_spawnp failed: {}", strerror(saved_errno))); + } + + read_fd(pipefd[0], [&](auto data) { + output.append(reinterpret_cast(data.data()), data.size()); + }); + + int status; + while ((result = waitpid(pid, &status, 0)) != pid) { + if (result == -1 && errno == EINTR) { + continue; + } + return tl::unexpected(FMT("waitpid failed: {}", strerror(errno))); + } + if (!WIFEXITED(status) || WEXITSTATUS(status) != 0) { + return tl::unexpected(FMT("Non-zero exit code: {}", WEXITSTATUS(status))); + } +#endif + + return output; +} + +} // namespace util diff --git a/src/ccache/util/exec.hpp b/src/ccache/util/exec.hpp new file mode 100644 index 00000000..5c0cfee1 --- /dev/null +++ b/src/ccache/util/exec.hpp @@ -0,0 +1,33 @@ +// Copyright (C) 2025 Joel Rosdahl and other contributors +// +// See doc/AUTHORS.adoc for a complete list of contributors. +// +// This program is free software; you can redistribute it and/or modify it +// under the terms of the GNU General Public License as published by the Free +// Software Foundation; either version 3 of the License, or (at your option) +// any later version. +// +// This program is distributed in the hope that it will be useful, but WITHOUT +// ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +// FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +// more details. +// +// You should have received a copy of the GNU General Public License along with +// this program; if not, write to the Free Software Foundation, Inc., 51 +// Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + +#pragma once + +#include + +#include + +#include + +namespace util { + +// Execute command in `args` and capture output (stdout and stderr combined), +// similar to execvp(2)/popen(3). +tl::expected exec_to_string(const Args& args); + +} // namespace util diff --git a/unittest/CMakeLists.txt b/unittest/CMakeLists.txt index 746c1e05..369ec9c5 100644 --- a/unittest/CMakeLists.txt +++ b/unittest/CMakeLists.txt @@ -25,6 +25,7 @@ set( test_util_direntry.cpp test_util_duration.cpp test_util_environment.cpp + test_util_exec.cpp test_util_expected.cpp test_util_file.cpp test_util_lockfile.cpp diff --git a/unittest/test_util_exec.cpp b/unittest/test_util_exec.cpp new file mode 100644 index 00000000..e0d1e80a --- /dev/null +++ b/unittest/test_util_exec.cpp @@ -0,0 +1,62 @@ +// Copyright (C) 2025 Joel Rosdahl and other contributors +// +// See doc/AUTHORS.adoc for a complete list of contributors. +// +// This program is free software; you can redistribute it and/or modify it +// under the terms of the GNU General Public License as published by the Free +// Software Foundation; either version 3 of the License, or (at your option) +// any later version. +// +// This program is distributed in the hope that it will be useful, but WITHOUT +// ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +// FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +// more details. +// +// You should have received a copy of the GNU General Public License along with +// this program; if not, write to the Free Software Foundation, Inc., 51 +// Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + +#include "testutil.hpp" + +#include +#include +#include + +#include + +using TestUtil::TestContext; + +TEST_CASE("util::exec_to_string") +{ + using util::exec_to_string; + + TestContext test_context; + + SUBCASE("stdout + stderr") + { +#ifdef _WIN32 + util::write_file("command.bat", "@echo off\r\necho fisk\r\necho sork>&2"); + util::Args args{"command.bat"}; +#else + util::Args args{"sh", "-c", "echo fisk; echo sork >&2"}; +#endif + auto result = exec_to_string(args); + REQUIRE(result); +#ifdef _WIN32 + CHECK(*result == "fisk\r\nsork\r\n"); +#else + CHECK(*result == "fisk\nsork\n"); +#endif + } + + SUBCASE("error") + { + auto result = exec_to_string({"doesnotexist"}); + REQUIRE(!result); +#ifdef _WIN32 + CHECK(util::starts_with(result.error(), "CreateProcess failure: ")); +#else + CHECK(result.error() == "posix_spawnp failed: No such file or directory"); +#endif + } +} -- 2.47.3