From 02bd6b2044c99e50f022e3d225d3eb2bb11ed2c0 Mon Sep 17 00:00:00 2001 From: Soo Hwan Na Date: Fri, 5 Jul 2024 17:29:06 +0900 Subject: [PATCH] TgBot++: android_builder: Make PythonClass singleton - Make a sperate fd for python script log - Slience EBADF log - And other improvements --- src/android_builder/ArgumentBuilder.cpp | 4 +- src/android_builder/ConfigParsers.cpp | 28 ++- src/android_builder/ForkAndRun.cpp | 50 ++++- src/android_builder/ForkAndRun.hpp | 39 +++- src/android_builder/PythonClass.cpp | 14 +- src/android_builder/PythonClass.hpp | 32 ++- .../scripts/build_rom_utils.py | 3 + src/android_builder/scripts/print.py | 23 +++ .../scripts/subprocess_utils.py | 3 + src/android_builder/tasks/PerBuildData.hpp | 6 +- src/android_builder/tasks/ROMBuildTask.hpp | 97 ++++----- src/android_builder/tasks/RepoSyncTask.cpp | 186 +++++++----------- src/android_builder/tasks/RepoSyncTask.hpp | 80 ++++++-- src/command_modules/runtime/cmd_RomBuild.cpp | 93 +++++---- src/include/internal/_FileDescriptor_posix.h | 28 ++- 15 files changed, 438 insertions(+), 248 deletions(-) create mode 100644 src/android_builder/scripts/print.py diff --git a/src/android_builder/ArgumentBuilder.cpp b/src/android_builder/ArgumentBuilder.cpp index 2258b9f6..0a73673f 100644 --- a/src/android_builder/ArgumentBuilder.cpp +++ b/src/android_builder/ArgumentBuilder.cpp @@ -28,11 +28,12 @@ PyObject* ArgumentBuilder::build() { PyErr_Print(); return nullptr; } + DLOG(INFO) << "Building arguments: size=" << argument_count; arguments_ref.reserve(argument_count); for (Py_ssize_t i = 0; i < argument_count; ++i) { PyObject* value = nullptr; std::visit( - [&value, &arguments_ref](auto&& arg) { + [&value, &arguments_ref, i](auto&& arg) { using T = std::decay_t; if constexpr (std::is_same_v) { value = PyLong_FromLong(arg); @@ -41,6 +42,7 @@ PyObject* ArgumentBuilder::build() { value = PyUnicode_FromString(arg.c_str()); arguments_ref.emplace_back(value); } + DLOG(INFO) << "Arguments[" << i << "]: " << arg; }, arguments[i]); if (value == nullptr) { diff --git a/src/android_builder/ConfigParsers.cpp b/src/android_builder/ConfigParsers.cpp index a4181bd0..b2eb936f 100644 --- a/src/android_builder/ConfigParsers.cpp +++ b/src/android_builder/ConfigParsers.cpp @@ -2,7 +2,7 @@ #include -constexpr bool parserDebug = false; +constexpr bool parserDebug = true; // Name RomName LocalManifestUrl LocalManifestBranch device variant template <> @@ -44,14 +44,17 @@ std::vector parse(std::ifstream data) { LOG(INFO) << "LocalManifestBranch: " << config.local_manifest.branch; LOG(INFO) << "Device: " << config.device; - LOG(INFO) << "Variant: " - << (config.variant == BuildConfig::Variant::kUser - ? "User" - : (config.variant == - BuildConfig::Variant::kUserDebug - ? "UserDebug" - : "Eng")); - LOG(INFO) << "---------------------------"; + switch (config.variant) { + case BuildConfig::Variant::kUser: + LOG(INFO) << "Variant: User"; + break; + case BuildConfig::Variant::kUserDebug: + LOG(INFO) << "Variant: UserDebug"; + break; + case BuildConfig::Variant::kEng: + LOG(INFO) << "Variant: Eng"; + break; + } LOG(INFO) << "---------------------------"; } configs.emplace_back(config); @@ -72,6 +75,13 @@ std::vector parse(std::ifstream data) { splitWithSpaces(line, items); if (items.size() != RomConfig::elem_size) { LOG(WARNING) << "Skipping invalid line: " << line; + if (parserDebug) { + LOG(INFO) << "Invalid rom config size: " << items.size(); + int count = 0; + for (const auto& item : items) { + LOG(INFO) << '[' << count << "]=" << item; + } + } continue; } rom.name = items[RomConfig::indexOf_name]; diff --git a/src/android_builder/ForkAndRun.cpp b/src/android_builder/ForkAndRun.cpp index 847dd295..5a6a8705 100644 --- a/src/android_builder/ForkAndRun.cpp +++ b/src/android_builder/ForkAndRun.cpp @@ -1,34 +1,54 @@ #include "ForkAndRun.hpp" +#include #include +#include #include #include #include #include #include -#include +#include #include bool ForkAndRun::execute() { Pipe stdout_pipe{}; Pipe stderr_pipe{}; + Pipe python_pipe{}; - if (!stderr_pipe.pipe() || !stdout_pipe.pipe()) { + if (!stderr_pipe.pipe() || !stdout_pipe.pipe() || !python_pipe.pipe()) { + stderr_pipe.close(); + stdout_pipe.close(); + python_pipe.close(); PLOG(ERROR) << "Failed to create pipes"; return false; } pid_t pid = fork(); if (pid == 0) { + FDLogSink sink; + + absl::AddLogSink(&sink); dup2(stdout_pipe.writeEnd(), STDOUT_FILENO); dup2(stderr_pipe.writeEnd(), STDERR_FILENO); close(stdout_pipe.readEnd()); close(stderr_pipe.readEnd()); - if (runFunction()) { - _exit(EXIT_SUCCESS); - } else { - _exit(EXIT_FAILURE); - } + close(python_pipe.readEnd()); + + PyObject* os = PyImport_ImportModule("os"); + PyObject* os_environ = PyObject_GetAttrString(os, "environ"); + PyObject* value = PyUnicode_FromString( + std::to_string(python_pipe.writeEnd()).c_str()); + PyMapping_SetItemString(os_environ, "PYTHON_LOG_FD", value); + + // Clean up + Py_DECREF(value); + Py_DECREF(os_environ); + Py_DECREF(os); + + int ret = runFunction() ? EXIT_SUCCESS : EXIT_FAILURE; + absl::RemoveLogSink(&sink); + _exit(ret); } else if (pid > 0) { Pipe program_termination_pipe{}; bool breakIt = false; @@ -37,6 +57,7 @@ bool ForkAndRun::execute() { childProcessId = pid; close(stdout_pipe.writeEnd()); close(stderr_pipe.writeEnd()); + close(python_pipe.writeEnd()); program_termination_pipe.pipe(); selector.add( @@ -63,6 +84,18 @@ bool ForkAndRun::execute() { } }, Selector::Mode::READ); + selector.add( + python_pipe.readEnd(), + [&python_pipe] { + BufferType buf{}; + ssize_t bytes_read = + read(python_pipe.readEnd(), buf.data(), buf.size() - 1); + if (bytes_read >= 0) { + printf("Python output: "); + fputs(buf.data(), stdout); + } + }, + Selector::Mode::READ); selector.add( program_termination_pipe.readEnd(), [program_termination_pipe, &breakIt]() { breakIt = true; }, @@ -93,6 +126,9 @@ bool ForkAndRun::execute() { pollThread.join(); // Cleanup + selector.remove(stdout_pipe.readEnd()); + selector.remove(stderr_pipe.readEnd()); + selector.remove(program_termination_pipe.readEnd()); program_termination_pipe.close(); stderr_pipe.close(); stdout_pipe.close(); diff --git a/src/android_builder/ForkAndRun.hpp b/src/android_builder/ForkAndRun.hpp index 8cdcc32e..bd8bf526 100644 --- a/src/android_builder/ForkAndRun.hpp +++ b/src/android_builder/ForkAndRun.hpp @@ -1,11 +1,39 @@ #pragma once // Helper class to fork and run a subprocess with stdout/err +#include + #include +#include #include +#include +#include "absl/log/log_entry.h" +#include "absl/log/log_sink.h" #include "socket/selector/SelectorPosix.hpp" +struct FDLogSink : public absl::LogSink { + void Send(const absl::LogEntry& logSink) override { + const auto message = logSink.text_message_with_prefix_and_newline(); + constexpr std::string_view prefix = "SubProcess: "; + if (isWritable && logSink.log_severity() <= absl::LogSeverity::kWarning) { + write(stdout_fd, prefix.data(), prefix.size()); + write(stdout_fd, message.data(), message.size()); + } + } + explicit FDLogSink() : stdout_fd(::dup(STDOUT_FILENO)) { + if (stdout_fd < 0) { + PLOG(ERROR) << "Failed to duplicate stdout"; + isWritable = false; + } + } + ~FDLogSink() override { ::close(stdout_fd); } + + private: + int stdout_fd; + bool isWritable = true; +}; + class ForkAndRun { public: virtual ~ForkAndRun() = default; @@ -24,9 +52,7 @@ class ForkAndRun { * * @param buffer The buffer containing the new stdout data. */ - virtual void onNewStdoutBuffer(BufferType& buffer) { - std::cout << buffer.data() << std::endl; - } + virtual void onNewStdoutBuffer(BufferType& buffer) {} /** * @brief Callback function for handling new stderr data. @@ -38,7 +64,8 @@ class ForkAndRun { * @param buffer The buffer containing the new stderr data. */ virtual void onNewStderrBuffer(BufferType& buffer) { - std::cerr << buffer.data() << std::endl; + std::string lines = buffer.data(); + std::cerr << boost::trim_copy(lines) << std::endl; } /** @@ -57,7 +84,9 @@ class ForkAndRun { * * @param signal The signal that caused the subprocess to exit. */ - virtual void onSignal(int signal) {} + virtual void onSignal(int signal) { + onExit(signal); + } /** * @brief The function to be run in the subprocess. diff --git a/src/android_builder/PythonClass.cpp b/src/android_builder/PythonClass.cpp index 1ee58dc8..cfd4c769 100644 --- a/src/android_builder/PythonClass.cpp +++ b/src/android_builder/PythonClass.cpp @@ -2,23 +2,35 @@ #include +#include #include -#include namespace details { template <> bool convert(PyObject* value) { + if (!PyBool_Check(value)) { + LOG(ERROR) << "Invalid boolean value: expected boolean object"; + return std::numeric_limits::min(); + } return PyObject_IsTrue(value) == 1; } template <> long convert(PyObject* value) { + if (!PyLong_Check(value)) { + LOG(ERROR) << "Invalid integer value: expected integer object"; + return std::numeric_limits::min(); + } return PyLong_AsLong(value); } template <> double convert(PyObject* value) { + if (!PyFloat_Check(value)) { + LOG(ERROR) << "Invalid float value: expected float object"; + return std::numeric_limits::quiet_NaN(); + } return PyFloat_AsDouble(value); } diff --git a/src/android_builder/PythonClass.hpp b/src/android_builder/PythonClass.hpp index 5afd353b..e3eb5e18 100644 --- a/src/android_builder/PythonClass.hpp +++ b/src/android_builder/PythonClass.hpp @@ -36,9 +36,11 @@ class PythonClass : public std::enable_shared_from_this { explicit PythonClass() = default; public: + using Ptr = std::shared_ptr; static std::shared_ptr get() { static PyInitHolder holder; - return std::make_shared(PythonClass()); + static auto instance = std::make_shared(PythonClass()); + return instance; } class ModuleHandle; @@ -56,6 +58,7 @@ class PythonClass : public std::enable_shared_from_this { public: friend class ModuleHandle; + using Ptr = std::shared_ptr; std::string name; // Logging purposes ~FunctionHandle() { Py_XDECREF(function); } FunctionHandle(FunctionHandle&& other) noexcept @@ -65,7 +68,29 @@ class PythonClass : public std::enable_shared_from_this { other.function = nullptr; } - // Manage the arg parameter lifetime yourself + /** + * Calls the specified Python function with the given arguments. + * + * @param args The arguments to be passed to the Python function. + * @param out A pointer to the variable where the result will be stored. + * If the function returns a value, it will be converted to + * the specified type and stored in out. If the function does not return + * a value, out can be nullptr. + * + * @return true If the function call is successful and no error occurs. + * false If an error occurs during the function call. + * + * @note The caller is responsible for managing the lifetime of the args + * parameter. + * + * @note If an error occurs during the function call, the error message + * will be printed to the standard error stream. The caller can retrieve + * the error message using PyErr_Print() or PyErr_Fetch(). + * + * @see https://docs.python.org/3/c-api/object.html#c.Py_DECREF + * @see https://docs.python.org/3/c-api/exceptions.html#c.PyErr_Print + * @see https://docs.python.org/3/c-api/exceptions.html#c.PyErr_Fetch + */ template bool call(PyObject* args, T* out) { LOG(INFO) << "Calling Python function: module " @@ -80,6 +105,8 @@ class PythonClass : public std::enable_shared_from_this { LOG(ERROR) << "Failed to call function"; return false; } + // If the function returned a value, convert it to the specified + // type and store it in out. if constexpr (!std::is_void_v) { T resultC = details::convert(result); if (PyErr_Occurred()) { @@ -88,6 +115,7 @@ class PythonClass : public std::enable_shared_from_this { *out = resultC; } } + DLOG(INFO) << "Call succeeded"; // Decrement the reference count of the result Py_DECREF(result); return true; diff --git a/src/android_builder/scripts/build_rom_utils.py b/src/android_builder/scripts/build_rom_utils.py index d5880e0b..eff6dd1c 100644 --- a/src/android_builder/scripts/build_rom_utils.py +++ b/src/android_builder/scripts/build_rom_utils.py @@ -1,5 +1,8 @@ import subprocess_utils import os +import print + +print = print.print_fd def find_vendor_str() -> str: vendor_path = 'vendor/' diff --git a/src/android_builder/scripts/print.py b/src/android_builder/scripts/print.py new file mode 100644 index 00000000..a49e619a --- /dev/null +++ b/src/android_builder/scripts/print.py @@ -0,0 +1,23 @@ +import os + +def print_fd(*args, **kwargs): + """ + Prints the provided arguments to the file descriptor specified by the environment variable 'PYTHON_LOG_FD'. + + Parameters: + *args: Variable length argument list. The arguments to be printed. + **kwargs: Arbitrary keyword arguments. The arguments to be passed to the print function. + + Returns: + None + + Note: + The file descriptor should be opened in write mode ('w') before using this function. + The file descriptor will be closed after printing the arguments. + """ + print(*args, **kwargs) + if False: # Tmp disabled + logfd = int(os.environ['PYTHON_LOG_FD']) + out = open(logfd, 'w') + print(*args, file=out, **kwargs) + out.close() \ No newline at end of file diff --git a/src/android_builder/scripts/subprocess_utils.py b/src/android_builder/scripts/subprocess_utils.py index d311d4f8..dbad2b6a 100644 --- a/src/android_builder/scripts/subprocess_utils.py +++ b/src/android_builder/scripts/subprocess_utils.py @@ -1,9 +1,12 @@ import subprocess import sys +import print testing = False testingRet = True +print = print.print_fd + def run_command(command: str) -> bool: print("Running command: %s" % command) sys.stdout.flush() diff --git a/src/android_builder/tasks/PerBuildData.hpp b/src/android_builder/tasks/PerBuildData.hpp index 474d3510..d599266c 100644 --- a/src/android_builder/tasks/PerBuildData.hpp +++ b/src/android_builder/tasks/PerBuildData.hpp @@ -3,8 +3,12 @@ #include struct PerBuildData { + enum class Result { SUCCESS, ERROR_NONFATAL, ERROR_FATAL }; BuildConfig bConfig; RomConfig rConfig; std::filesystem::path scriptDirectory; - bool* result; + struct ResultData { + Result value; + std::string msg; + } *result; }; diff --git a/src/android_builder/tasks/ROMBuildTask.hpp b/src/android_builder/tasks/ROMBuildTask.hpp index 30b60c61..f93227b2 100644 --- a/src/android_builder/tasks/ROMBuildTask.hpp +++ b/src/android_builder/tasks/ROMBuildTask.hpp @@ -1,29 +1,31 @@ #include +#include #include -#include -#include #include #include #include -#include #include #include +#include #include #include #include #include -#include -#include -#include "BotReplyMessage.h" #include "PerBuildData.hpp" #include "PythonClass.hpp" #include "internal/_std_chrono_templates.h" +#include "object.h" struct ROMBuildTask : ForkAndRun, BotClassBase { ~ROMBuildTask() override = default; + void logErrorAndSave(const std::string& message) const { + LOG(ERROR) << "Error during ROM build: " << message; + data.result->msg.append(message + "\n"); + } + /** * @brief Runs the function that performs the repository synchronization. * @@ -35,17 +37,15 @@ struct ROMBuildTask : ForkAndRun, BotClassBase { * otherwise. */ bool runFunction() override { - auto py = PythonClass::get(); - - py->addLookupDirectory(data.scriptDirectory); - auto repomod = py->importModule("build_rom_utils"); + data.result->value = PerBuildData::Result::ERROR_FATAL; + auto repomod = _py->importModule("build_rom_utils"); if (!repomod) { - LOG(ERROR) << "Failed to import build_rom_utils module"; + logErrorAndSave("Failed to import build_rom_utils module"); return false; } auto build_rom = repomod->lookupFunction("build_rom"); if (!build_rom) { - LOG(ERROR) << "Failed to find build_rom function"; + logErrorAndSave("Failed to find build_rom function"); return false; } ArgumentBuilder builder(4); @@ -63,28 +63,39 @@ struct ROMBuildTask : ForkAndRun, BotClassBase { } builder.add_argument(data.rConfig.target); builder.add_argument(guessJobCount()); + auto* arg = builder.build(); + if (arg == nullptr) { + logErrorAndSave("Failed to build arguments"); + return false; + } bool result = false; - if (!build_rom->call(builder.build(), &result)) { - LOG(ERROR) << "Failed to call build ROM"; + if (!build_rom->call(arg, &result)) { + logErrorAndSave("Failed to call build ROM"); + return false; } + Py_DECREF(arg); if (result) { LOG(INFO) << "ROM build succeeded"; + data.result->value = PerBuildData::Result::SUCCESS; } else { LOG(ERROR) << "ROM build failed"; } - *data.result = result; return result; } int guessJobCount() { static std::once_flag once; static int jobCount = 6; + static constexpr int Multiplier = 1024; std::call_once(once, [this] { double total_memory = NAN; if (!_get_total_mem->call(nullptr, &total_memory)) { return; } - jobCount = static_cast(sqrt(total_memory / 1024) * 4); + total_memory /= Multiplier; // Convert to GB + LOG(INFO) << "Total memory: " << total_memory << "GB"; + jobCount = static_cast(sqrt(total_memory) * 2); + LOG(INFO) << "Using job count: " << jobCount; }); return jobCount; } @@ -108,20 +119,24 @@ struct ROMBuildTask : ForkAndRun, BotClassBase { buildInfoBuffer << "Time spent: " << to_string(now - startTime) << std::endl; buildInfoBuffer << "Last updated on: " << fromTP(now) << std::endl; - buildInfoBuffer << "Target device: " << data.bConfig.device; + buildInfoBuffer << "Target device: " << data.bConfig.device << std::endl; buildInfoBuffer << "Job count: " << guessJobCount(); if (_get_used_mem->call(nullptr, &memUsage)) { buildInfoBuffer << ", memory usage: " << memUsage << "%"; + } else { + buildInfoBuffer << ", memory usage: unavailable"; } + buildInfoBuffer << std::endl; + buildInfoBuffer << "Variant: "; switch (data.bConfig.variant) { case BuildConfig::Variant::kUser: - buildInfoBuffer << ", variant: user"; + buildInfoBuffer << "user"; break; case BuildConfig::Variant::kUserDebug: - buildInfoBuffer << ", variant: userdebug"; + buildInfoBuffer << "userdebug"; break; case BuildConfig::Variant::kEng: - buildInfoBuffer << ", variant: eng"; + buildInfoBuffer << "eng"; break; } buildInfoBuffer << std::endl << std::endl; @@ -130,20 +145,6 @@ struct ROMBuildTask : ForkAndRun, BotClassBase { } } - /** - * @brief Handles new standard error data. - * - * This function is called when new standard error data is available. It - * overrides the base class's onNewStderrBuffer() method to provide custom - * behavior. - * - * @param buffer The buffer containing the new standard error data. - */ - void onNewStderrBuffer(ForkAndRun::BufferType& buffer) override { - std::string lines = buffer.data(); - LOG(ERROR) << "onNewStderr: " << boost::trim_copy(lines); - } - /** * @brief Handles the process exit event. * @@ -156,7 +157,6 @@ struct ROMBuildTask : ForkAndRun, BotClassBase { std::stringstream exitInfoBuffer; exitInfoBuffer << "Exit code: " << exitCode; bot_editMessage(_bot, message, exitInfoBuffer.str()); - *data.result = exitCode == 0; LOG(INFO) << "Process exited with code: " << exitCode; } @@ -175,8 +175,13 @@ struct ROMBuildTask : ForkAndRun, BotClassBase { LOG(WARNING) << "Process received signal: " << signalCode; } + [[noreturn]] static void errorAndThrow(const std::string& message) { + LOG(ERROR) << message; + throw std::runtime_error(message); + } + /** - * @brief Constructs a RepoSyncF object with the provided data. + * @brief Constructs a ROMBuildTask object with the provided data. * * This constructor initializes a RepoSyncF object with the given data. * @@ -191,31 +196,29 @@ struct ROMBuildTask : ForkAndRun, BotClassBase { clock = std::chrono::system_clock::now(); startTime = std::chrono::system_clock::now(); - auto c = PythonClass::get(); - c->addLookupDirectory(data.scriptDirectory); - auto repomod = c->importModule("system_info"); + _py = PythonClass::get(); + _py->addLookupDirectory(data.scriptDirectory); + auto repomod = _py->importModule("system_info"); if (!repomod) { - LOG(ERROR) << "Failed to import system_info module"; - return; + errorAndThrow("Failed to import system_info module"); } _get_total_mem = repomod->lookupFunction("get_memory_total"); if (!_get_total_mem) { - LOG(ERROR) << "Failed to find get_memory_total function"; - return; + errorAndThrow("Failed to find get_memory_total function"); } _get_used_mem = repomod->lookupFunction("get_memory_usage"); if (!_get_used_mem) { - LOG(ERROR) << "Failed to find get_memory_usage function"; - return; + errorAndThrow("Failed to find get_memory_usage function"); } } private: PerBuildData data; TgBot::Message::Ptr message; - std::shared_ptr _get_total_mem; - std::shared_ptr _get_used_mem; + PythonClass::Ptr _py; + PythonClass::FunctionHandle::Ptr _get_total_mem; + PythonClass::FunctionHandle::Ptr _get_used_mem; decltype(std::chrono::system_clock::now()) clock; decltype(std::chrono::system_clock::now()) startTime; }; \ No newline at end of file diff --git a/src/android_builder/tasks/RepoSyncTask.cpp b/src/android_builder/tasks/RepoSyncTask.cpp index d54b18fe..bf4121dd 100644 --- a/src/android_builder/tasks/RepoSyncTask.cpp +++ b/src/android_builder/tasks/RepoSyncTask.cpp @@ -8,6 +8,55 @@ #include "tasks/PerBuildData.hpp" +bool RepoSyncLocalHook::process(const std::string& line) { + if (line.find(kUpdatingFiles) != std::string::npos) { + // Stop logspam + return true; + } + if (line.find(kChangesWouldBeLost) != std::string::npos) { + errorAndLog( + "Repo sync failed due to local issue: Changes would be lost"); + hadProblems = true; + return true; + } + + if (line.find(kRepoCheckoutFailStart) != std::string::npos) { + errorAndLog("Repo sync failed due to local issue: CheckoutFail"); + hasCheckoutIssues = true; + hadProblems = true; + return true; + } + + if (hasCheckoutIssues) { + std::error_code ec; + if (line.find(kRepoCheckoutFailEnd) != std::string::npos) { + // Clear the flag + hasCheckoutIssues = false; + return true; + } + std::filesystem::remove_all(line, ec); + std::string msg = "Checkout Failed Directory: " + line + ": "; + if (ec) { + errorAndLog(msg + "Failed to remove: " + ec.message()); + hadFatalProblems = true; + } else { + errorAndLog(msg + "Removed"); + } + return true; + } + return false; +} +bool RepoSyncNetworkHook::process(const std::string& line) { + static const std::regex kSyncErrorNetworkRegex( + R"(^error:\s+Cannot\s+fetch\s+[^\s]+(?:/[^\s]+)*\s+from\s+https:\/\/.+$)"); + if (std::regex_match(line, kSyncErrorNetworkRegex)) { + LOG(INFO) << "Detected sync issue, caused by network"; + hadProblems = true; + return true; + } + return false; +} + bool RepoSyncTask::runFunction() { try { RepoUtils utils(data.scriptDirectory); @@ -35,134 +84,19 @@ bool RepoSyncTask::runFunction() { return true; } -void RepoSyncTask::onNewStdoutBuffer(ForkAndRun::BufferType& buffer) { - std::vector lines; - boost::split(lines, buffer.data(), [](const char c) { return c == '\n'; }); - for (const auto& line : lines) { - if (line.empty()) { - continue; - } - std::cout << "Repo sync stdout: " << line << std::endl; - } -} - -class NewStdErrBufferHook { - public: - enum Action { kContinueLoop, kPass, kErrorLocalIssue, kErrorNetworkIssue }; - - /** - * @brief Virtual method to process each line of stderr buffer. - * - * This method is intended to be overridden by subclasses to handle specific - * processing logic for stderr lines. - * - * @param line A single line of text from the stderr buffer. - * @return An enum value indicating whether the loop should continue or - * pass. - * - * @note This method is called for each line of stderr output during the - * execution of the task. - * - * @note Subclasses should override this method to implement their own - * processing logic. - */ - virtual Action process(const std::string& line) = 0; - - virtual ~NewStdErrBufferHook() = default; -}; - -class RepoSyncLocalHook : public NewStdErrBufferHook { - static constexpr std::string_view kUpdatingFiles = "Updating files:"; - static constexpr std::string_view kChangesWouldBeLost = - "error: Your local changes to the following files would be overwritten " - "by checkout:"; - static constexpr std::string_view kRepoCheckoutFailStart = "Failing repos:"; - static constexpr std::string_view kRepoCheckoutFailEnd = - "Try re-running with \"-j1 --fail-fast\" to exit at the first error."; - bool hasCheckoutIssues = false; - - public: - Action process(const std::string& line) override { - if (line.find(kUpdatingFiles) != std::string::npos) { - // Stop logspam - return Action::kContinueLoop; - } - if (line.find(kChangesWouldBeLost) != std::string::npos) { - LOG(ERROR) - << "Repo sync failed due to local issue: Changes would be lost"; - return Action::kErrorLocalIssue; - } - - if (line.find(kRepoCheckoutFailStart) != std::string::npos) { - LOG(ERROR) << "Repo sync failed due to local issue: CheckoutFail"; - hasCheckoutIssues = true; - return Action::kErrorLocalIssue; - } - - if (hasCheckoutIssues) { - std::error_code ec; - if (line.find(kRepoCheckoutFailEnd) != std::string::npos) { - // Clear the flag - hasCheckoutIssues = false; - return Action::kErrorLocalIssue; - } - LOG(INFO) << "Repo sync failed directory: " << line; - std::filesystem::remove_all(line, ec); - if (ec) { - LOG(ERROR) << "Failed to remove directory: " << line - << ", error: " << ec.message(); - } - return Action::kErrorLocalIssue; - } - return Action::kPass; - } - ~RepoSyncLocalHook() override = default; -}; - -class RepoSyncNetworkHook : public NewStdErrBufferHook { - public: - Action process(const std::string& line) override { - static const std::regex kSyncErrorNetworkRegex( - R"(^error:\s+Cannot\s+fetch\s+[^\s]+(?:/[^\s]+)*\s+from\s+https:\/\/.+$)"); - if (std::regex_match(line, kSyncErrorNetworkRegex)) { - LOG(INFO) << "Detected sync issue, caused by network"; - return Action::kErrorNetworkIssue; - } - return Action::kPass; - } - ~RepoSyncNetworkHook() override = default; -}; - void RepoSyncTask::onNewStderrBuffer(ForkAndRun::BufferType& buffer) { std::vector lines; - RepoSyncLocalHook localHook; - RepoSyncNetworkHook networkHook; boost::split(lines, buffer.data(), [](const char c) { return c == '\n'; }); for (const auto& line : lines) { if (line.empty()) { continue; } - switch (localHook.process(line)) { - case NewStdErrBufferHook::kContinueLoop: - continue; - case NewStdErrBufferHook::kPass: - break; - case NewStdErrBufferHook::kErrorLocalIssue: - case NewStdErrBufferHook::kErrorNetworkIssue: - // Nothing for now... - break; + if (localHook.process(line)) { + continue; } - switch (networkHook.process(line)) { - case NewStdErrBufferHook::kContinueLoop: - continue; - case NewStdErrBufferHook::kPass: - break; - case NewStdErrBufferHook::kErrorNetworkIssue: - networkSyncError = true; - break; - default: - break; + if (networkHook.process(line)) { + continue; } std::cerr << "Repo sync stderr: " << line << std::endl; } @@ -170,8 +104,20 @@ void RepoSyncTask::onNewStderrBuffer(ForkAndRun::BufferType& buffer) { void RepoSyncTask::onExit(int exitCode) { LOG(INFO) << "Repo sync exited with code: " << exitCode; - *data.result = exitCode == 0; + if (localHook.hasProblems()) { + data.result->msg = localHook.getLogMessage(); + data.result->value = localHook.hasFatalProblems() + ? PerBuildData::Result::ERROR_FATAL + : PerBuildData::Result::ERROR_NONFATAL; + } else if (networkHook.hasProblems()) { + data.result->msg = networkHook.getLogMessage(); + data.result->value = PerBuildData::Result::ERROR_NONFATAL; + } else { + data.result->value = PerBuildData::Result::SUCCESS; + data.result->msg = "Repo sync successful"; + } } + void RepoSyncTask::onSignal(int signalCode) { LOG(INFO) << "Repo sync received signal: " << strsignal(signalCode); } diff --git a/src/android_builder/tasks/RepoSyncTask.hpp b/src/android_builder/tasks/RepoSyncTask.hpp index 68c69d91..55fb164e 100644 --- a/src/android_builder/tasks/RepoSyncTask.hpp +++ b/src/android_builder/tasks/RepoSyncTask.hpp @@ -3,6 +3,69 @@ #include #include "PerBuildData.hpp" +class NewStdErrBufferHook { + std::stringstream logMessage; + + protected: + bool hadProblems = false; + bool hadFatalProblems = false; + + void errorAndLog(const std::string& message) { + LOG(ERROR) << message; + logMessage << message << std::endl; + } + + public: + /** + * @brief Virtual method to process each line of stderr buffer. + * + * This method is intended to be overridden by subclasses to handle specific + * processing logic for stderr lines. + * + * @param line A single line of text from the stderr buffer. + * @return A boolean value indicating whether the hook processed the line. + * + * @note This method is called for each line of stderr output during the + * execution of the task. + * + * @note Subclasses should override this method to implement their own + * processing logic. + */ + virtual bool process(const std::string& line) = 0; + + [[nodiscard]] std::string getLogMessage() const noexcept { + return logMessage.str(); + } + [[nodiscard]] bool hasProblems() const noexcept { + return hadProblems; + } + [[nodiscard]] bool hasFatalProblems() const noexcept { + return hadProblems && hadFatalProblems; + } + virtual ~NewStdErrBufferHook() = default; +}; + +class RepoSyncLocalHook : public NewStdErrBufferHook { + static constexpr std::string_view kUpdatingFiles = "Updating files:"; + static constexpr std::string_view kChangesWouldBeLost = + "error: Your local changes to the following files would be overwritten " + "by checkout:"; + static constexpr std::string_view kRepoCheckoutFailStart = "Failing repos:"; + static constexpr std::string_view kRepoCheckoutFailEnd = + "Try re-running with \"-j1 --fail-fast\" to exit at the first error."; + + bool hasCheckoutIssues = false; + public: + bool process(const std::string& line) override; + ~RepoSyncLocalHook() override = default; +}; + +class RepoSyncNetworkHook : public NewStdErrBufferHook { + public: + bool process(const std::string& line) override; + ~RepoSyncNetworkHook() override = default; +}; + struct RepoSyncTask : ForkAndRun { constexpr static std::string_view kLocalManifestPath = ".repo/local_manifests"; @@ -18,18 +81,7 @@ struct RepoSyncTask : ForkAndRun { * otherwise. */ bool runFunction() override; - - /** - * @brief Handles new standard output data. - * - * This function is called when new standard output data is available. It - * overrides the base class's onNewStdoutBuffer() method to provide custom - * behavior. - * - * @param buffer The buffer containing the new standard output data. - */ - void onNewStdoutBuffer(ForkAndRun::BufferType& buffer) override; - + /** * @brief Handles new standard error data. * @@ -74,4 +126,8 @@ struct RepoSyncTask : ForkAndRun { private: PerBuildData data; std::atomic_bool networkSyncError = false; + std::atomic_bool localSyncError = false; + + RepoSyncLocalHook localHook; + RepoSyncNetworkHook networkHook; }; \ No newline at end of file diff --git a/src/command_modules/runtime/cmd_RomBuild.cpp b/src/command_modules/runtime/cmd_RomBuild.cpp index d44b050e..fa3f1b8a 100644 --- a/src/command_modules/runtime/cmd_RomBuild.cpp +++ b/src/command_modules/runtime/cmd_RomBuild.cpp @@ -14,6 +14,7 @@ #include "PythonClass.hpp" #include "cmd_dynamic.h" #include "command_modules/CommandModule.h" +#include "tasks/PerBuildData.hpp" namespace { @@ -78,54 +79,75 @@ bool parseConfigAndGetTarget(PerBuildData* data, MessageWrapper& wrapper, } bool repoSync(PerBuildData data, MessageWrapper& wrapper) { - bool promise{}; - data.result = &promise; + PerBuildData::ResultData result{}; + data.result = &result; RepoSyncTask repoSync(data); - if (!repoSync.execute()) { - LOG(ERROR) << "Failed to exec"; - wrapper.sendMessageOnExit("Sync failed: Internal error"); - } else { - if (!promise) { - LOG(ERROR) << "Repo sync failed"; - wrapper.sendMessageOnExit("Repo sync failed"); - } else { + + do { + bool execRes = repoSync.execute(); + if (!execRes) { + LOG(ERROR) << "Failed to exec"; + wrapper.sendMessageOnExit( + "Repo sync failed: Couldn't start process"); + return false; + } + } while (result.value == PerBuildData::Result::ERROR_NONFATAL); + + switch (result.value) { + case PerBuildData::Result::SUCCESS: wrapper.sendMessageOnExit("Repo sync completed successfully"); return true; - } + case PerBuildData::Result::ERROR_FATAL: + LOG(ERROR) << "Repo sync failed"; + wrapper.sendMessageOnExit("Repo sync failed\n:" + result.msg); + break; + default: + break; } return false; } bool build(PerBuildData data, MessageWrapper& wrapper) { - bool promise{}; static const std::filesystem::path kErrorLogFile = "out/error.log"; std::error_code ec; const auto api = wrapper.getBot().getApi(); + PerBuildData::ResultData result{}; - data.result = &promise; + data.result = &result; auto msg = api.sendMessage(wrapper.getChatId(), "Starting build..."); wrapper.switchToReplyToMessage(); + ROMBuildTask romBuild(wrapper.getBot(), msg, data); - if (!romBuild.execute()) { - LOG(ERROR) << "Failed to exec"; - wrapper.sendMessageOnExit("Build failed: Couldn't start build process"); - return false; - } - if (!promise) { - LOG(ERROR) << "Failed to build ROM"; - wrapper.sendMessageOnExit("Build failed"); - if (std::filesystem::file_size(kErrorLogFile, ec) != 0U) { - api.sendDocument( - wrapper.getMessage()->chat->id, - TgBot::InputFile::fromFile(kErrorLogFile, "text/plain")); - } else { - api.sendMessage(wrapper.getMessage()->chat->id, - "Could not send " + kErrorLogFile.string() + ": " + - ec.message()); + + do { + bool execRes = romBuild.execute(); + if (!execRes) { + LOG(ERROR) << "Failed to exec"; + wrapper.sendMessageOnExit( + "Build failed: Couldn't start build process"); + return false; } - } else { - wrapper.sendMessageOnExit("Build completed successfully"); - return true; + } while (result.value == PerBuildData::Result::ERROR_NONFATAL); + + switch (result.value) { + case PerBuildData::Result::ERROR_FATAL: + LOG(ERROR) << "Failed to build ROM"; + wrapper.sendMessageOnExit("Build failed:\n" + result.msg); + if (std::filesystem::file_size(kErrorLogFile, ec) != 0U) { + api.sendDocument( + wrapper.getMessage()->chat->id, + TgBot::InputFile::fromFile(kErrorLogFile, "text/plain")); + } else { + api.sendMessage(wrapper.getMessage()->chat->id, + "Could not send " + kErrorLogFile.string() + + ": " + ec.message()); + } + break; + case PerBuildData::Result::SUCCESS: + wrapper.sendMessageOnExit("Build completed successfully"); + return true; + case PerBuildData::Result::ERROR_NONFATAL: + break; } return false; } @@ -164,25 +186,28 @@ static void romBuildCommand(const Bot& bot, const Message::Ptr message) { wrapper.sendMessageOnExit("Failed to create build directory"); return; } - std::filesystem::current_path(kBuildDirectory, ec); + // Sync the repository if (!repoSync(PBData, wrapper)) { returnToCwd(); return; } + // Build the ROM if (!build(PBData, wrapper)) { returnToCwd(); return; } - + +#if 0 bot_sendMessage(bot, wrapper.getChatId(), "Will upload"); bool uploadResult = false; PBData.result = &uploadResult; UploadFileTask uploadTask(wrapper, PBData); uploadTask.execute(); +#endif returnToCwd(); } diff --git a/src/include/internal/_FileDescriptor_posix.h b/src/include/internal/_FileDescriptor_posix.h index d4982902..46682ab5 100644 --- a/src/include/internal/_FileDescriptor_posix.h +++ b/src/include/internal/_FileDescriptor_posix.h @@ -2,6 +2,7 @@ #include #include +#include using pipe_t = std::array; @@ -12,17 +13,22 @@ inline bool isValidFd(int fd) { return fd != kInvalidFD; } inline void closeFd(int& fd) { int rc = 0; - if (isValidFd(fd)) rc = close(fd); + if (isValidFd(fd)) { + rc = close(fd); + } - PLOG_IF(ERROR, rc != 0) << "Failed to close fd: " << fd; + PLOG_IF(ERROR, (rc != 0 && errno != EBADF)) + << "Failed to close fd: " << fd; fd = kInvalidFD; } /** * @brief A class representing a pipe with two file descriptors. * - * This class represents a pipe with two file descriptors, one for reading and one for writing. - * It provides methods to check if the pipe is valid, invalidate the pipe, close the pipe, and get the read and write file descriptors. + * This class represents a pipe with two file descriptors, one for reading and + * one for writing. It provides methods to check if the pipe is valid, + * invalidate the pipe, close the pipe, and get the read and write file + * descriptors. */ struct Pipe { /** @@ -38,7 +44,8 @@ struct Pipe { /** * @brief Checks if the pipe is valid. * - * This method checks if the pipe is valid by verifying if both file descriptors are valid. + * This method checks if the pipe is valid by verifying if both file + * descriptors are valid. * * @return True if the pipe is valid, false otherwise. */ @@ -49,7 +56,8 @@ struct Pipe { /** * @brief Invalidates the pipe. * - * This method invalidates the pipe by setting both file descriptors to an invalid value. + * This method invalidates the pipe by setting both file descriptors to an + * invalid value. */ void invalidate() { underlying[0] = kInvalidFD; @@ -59,7 +67,8 @@ struct Pipe { /** * @brief Closes the pipe. * - * This method closes the pipe by calling the closeFd method on both file descriptors. + * This method closes the pipe by calling the closeFd method on both file + * descriptors. */ void close() { closeFd(underlying[0]); @@ -69,7 +78,8 @@ struct Pipe { /** * @brief Creates a pipe. * - * This method creates a pipe by calling the pipe system call and stores the file descriptors in the underlying array. + * This method creates a pipe by calling the pipe system call and stores the + * file descriptors in the underlying array. * * @return True if the pipe is successfully created, false otherwise. */ @@ -79,7 +89,7 @@ struct Pipe { return rc == 0; } - int operator[] (EndPoint ep) const { + int operator[](EndPoint ep) const { return underlying[static_cast(ep)]; }