refactor: simply the builder configuration loading logic #1562
Conversation
Signed-off-by: reddevillg <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: reddevillg The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Summary of ChangesHello @reddevillg, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the builder configuration loading mechanism to enhance its robustness, centralization, and ease of use. It streamlines the process of locating and initializing the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
deepin pr auto reviewGit Diff 代码审查报告总体评价这次改动主要重构了配置加载逻辑，将配置管理从 具体问题与建议1. 语法与逻辑问题1.1 日志函数不一致性位置: qCritical() << builderCfg.error();改为: LogE("failed to load build config {}", builderCfg.error());问题: 日志系统从 Qt 的 1.2 文件路径处理不一致位置: auto cacheLocation = common::xdg::getXDGCacheHomeDir();
if (cacheLocation.empty()) {
return LINGLONG_ERR("failed to get cache dir, neither XDG_CACHE_HOME nor HOME is set");
}问题: 使用 2. 代码质量问题2.1 错误处理不够详细位置: if (!config) {
return LINGLONG_ERR(config);
}问题: 错误信息不够详细，没有指出具体是哪个配置文件加载失败。 2.2 路径硬编码位置: std::filesystem::path builderConfigPath = "builder/config.yaml";问题: 配置文件路径硬编码，缺乏灵活性。 2.3 缺少配置文件验证位置: if (config->version != 1) {
return LINGLONG_ERR(
fmt::format("wrong configuration file version {}", config->version));
}问题: 仅验证版本号，未验证其他配置字段的合法性。 3. 代码性能问题3.1 文件系统操作可能频繁位置: auto path = configDir / builderConfigPath;
if (!std::filesystem::exists(path, ec)) {问题: 每次加载配置都会检查文件是否存在，可能造成不必要的文件系统访问。 3.2 YAML 加载效率位置: auto config = utils::serialize::LoadYAMLFile<api::types::v1::BuilderConfig>(path);问题: 每次加载配置都会完整解析 YAML 文件。 4. 代码安全问题4.1 路径遍历风险位置: std::filesystem::path builderConfigPath = "builder/config.yaml";
auto configDir = common::dir::getUserRuntimeConfigDir();
if (configDir.empty()) {
return LINGLONG_ERR("failed to get config dir");
}问题: 如果 4.2 权限问题位置: std::error_code ec;
std::filesystem::create_directories(path.parent_path(), ec);
if (ec) {
return LINGLONG_ERR("failed to create config dir", ec);
}问题: 创建目录时没有设置适当的权限，可能导致配置文件被其他用户读取或修改。 4.3 敏感信息泄露风险位置: LogD("Load build config from {}", path);问题: 日志中记录完整路径可能泄露系统信息。 5. 测试覆盖问题5.1 测试用例不完整位置:
5.2 测试环境隔离位置: EnvironmentVariableGuard homeGuard("HOME", testDir().string());
EnvironmentVariableGuard xdgCacheGuard("XDG_CACHE_HOME", (testDir() / "cache").string());问题: 测试中修改环境变量，但可能影响其他测试。 总结这次重构在整体方向上是正确的，提高了代码的模块化和可维护性。但在错误处理、路径安全、权限设置等方面还有改进空间。建议在合并前解决上述安全和健壮性问题，并完善测试用例覆盖。 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the builder configuration loading logic, centralizing it and removing Qt dependencies in favor of the standard C++ filesystem library. This is a great improvement. I've added a few comments:
- A high-severity concern about the removal of project-specific configuration file loading, which appears to be a significant breaking change.
- A suggestion to improve consistency by adding
noexceptto a new function declaration. - A fix for a test case that seems to have flawed logic.
Overall, the changes are good, but the potential breaking change needs to be addressed or at least documented.
I am having trouble creating individual review comments. Click here to see my feedback.
apps/ll-builder/src/main.cpp (41-54)
This refactoring removes the functionality to search for project-specific configuration files (.ll-builder/config.yaml) by traversing up from the current directory. The new implementation in linglong::builder::loadConfig() only loads a single global user configuration.
This is a significant behavioral change that could break workflows for users who depend on per-project builder configurations.
If this change is intentional, it should be clearly documented as a breaking change for users. If it's unintentional, the project-specific config loading logic should be restored within the new loadConfig implementation.
libs/linglong/tests/ll-tests/src/linglong/builder/config_test.cpp (140-150)
This test case for directory creation failure appears to be flawed. Passing "config.yaml" to initDefaultBuildConfig results in its parent path being empty. std::filesystem::create_directories on an empty path is a no-op and doesn't set an error. Subsequently, saveConfig will likely succeed by creating config.yaml in the test's working directory, causing the ASSERT_FALSE to fail.
To correctly test this failure scenario, you should create a file where a directory is expected, which will cause create_directories to fail. Here is a suggested implementation:
TEST_F(BuilderConfigTest, InitDefaultBuildConfigCreateDirFailure)
{
EnvironmentVariableGuard homeGuard("HOME", testDir().string());
EnvironmentVariableGuard xdgCacheGuard("XDG_CACHE_HOME", (testDir() / "cache").string());
// Create a file where a directory is expected, to cause create_directories to fail.
const auto builderDirAsFile = testDir() / "builder";
std::ofstream(builderDirAsFile);
const auto configPath = builderDirAsFile / "config.yaml";
auto result = initDefaultBuildConfig(configPath);
ASSERT_FALSE(result.has_value());
EXPECT_TRUE(result.error().message().find("failed to create config dir") != std::string::npos);
}libs/linglong/src/linglong/builder/config.h (16-17)
For consistency with other function declarations in this header, initDefaultBuildConfig should be marked as noexcept. The implementation does not throw exceptions but returns errors via the Result type, which matches the noexcept pattern used for other functions like loadConfig and saveConfig.
Don't forget to add noexcept to the function definition in config.cpp as well.
utils::error::Result<api::types::v1::BuilderConfig>
initDefaultBuildConfig(const std::filesystem::path &path) noexcept;
Codecov Report❌ Patch coverage is
... and 3 files with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This pull request refactors the builder configuration loading logic to simplify the implementation and improve maintainability. The changes migrate from Qt-based types (QString, QStringList) to standard C++ types (std::filesystem::path, std::string) and remove complex multi-path configuration resolution in favor of a simpler, single-path approach.
Changes:
- Replaced Qt-based file path handling with std::filesystem::path throughout the configuration module
- Simplified configuration loading by removing multi-path search logic and consolidating to XDG config directory
- Added comprehensive test coverage for all configuration loading scenarios including error cases
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| libs/linglong/tests/ll-tests/src/linglong/builder/config_test.cpp | New comprehensive test suite covering all configuration functions including edge cases and error scenarios |
| libs/linglong/tests/ll-tests/CMakeLists.txt | Added new test file to the build configuration |
| libs/linglong/src/linglong/builder/config.h | Updated API signatures to use std::filesystem::path instead of QString and simplified to three main functions |
| libs/linglong/src/linglong/builder/config.cpp | Refactored implementation to use std::filesystem and removed complex multi-path resolution logic |
| apps/ll-builder/src/main.cpp | Simplified configuration loading by removing project-specific config paths and initialization logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2 participants
No description provided.