-
Notifications
You must be signed in to change notification settings - Fork 477
[windows][installer] add additional logging during MSI preparation for execution #6402
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds detailed logging throughout the MSI preparation and execution steps to help diagnose failures and track progress.
- Introduces a
Logger
singleton that writes timestamped messages to a temp-directory log file. - Wraps key operations (
computeMD5
, resource extraction, directory creation, file mapping, and execution) withLog
calls.
Comments suppressed due to low confidence (1)
installer/installer_setup/main.cpp:29
- The code uses
std::filesystem::path
but does not#include <filesystem>
. Add#include <filesystem>
near the top to avoid build errors.
#include <ctime>
installer/installer_setup/main.cpp
Outdated
} | ||
const auto now = std::chrono::system_clock::now(); | ||
const auto now_c = std::chrono::system_clock::to_time_t(now); | ||
ofs << std::ctime(&now_c) << " " << message << std::endl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using std::ctime
embeds a trailing newline and relies on a static buffer, which is not thread-safe. Consider using std::localtime_s
(or std::localtime_r
) with std::put_time
to format the timestamp without an extra newline and in a threadsafe manner.
ofs << std::ctime(&now_c) << " " << message << std::endl; | |
std::tm localTime; | |
localtime_s(&localTime, &now_c); | |
std::ostringstream timestamp; | |
timestamp << std::put_time(&localTime, "%Y-%m-%d %H:%M:%S"); | |
ofs << timestamp.str() << " " << message << std::endl; |
Copilot uses AI. Check for mistakes.
} | ||
logPath /= "boinc_installer.log"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If GetTempPath
fails, logPath
remains empty and the log will open in the working directory. Add handling for a failed GetTempPath
(e.g., fallback or an early exit) to avoid writing logs to an unintended location.
} | |
logPath /= "boinc_installer.log"; | |
} else { | |
// Fallback to a path relative to the executable | |
logPath = std::filesystem::current_path(); | |
logPath /= "boinc_installer.log"; | |
} |
Copilot uses AI. Check for mistakes.
|
||
void Log(const std::string& message) { | ||
Logger::Instance().Log(message); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The free function Log
may collide with other symbols or macros. Consider placing it in a namespace (e.g., installer::Log
) or renaming it to avoid global scope pollution.
void Log(const std::string& message) { | |
Logger::Instance().Log(message); | |
} | |
namespace installer { | |
void Log(const std::string& message) { | |
Logger::Instance().Log(message); | |
} | |
} |
Copilot uses AI. Check for mistakes.
…r execution Signed-off-by: Vitalii Koshura <lestat.de.lionkur@gmail.com>
018967e
to
7eba605
Compare
No description provided.