refactor: improve build dependency pull

[reddevillg]

No description provided.

[deepin-ci-robot]

deepin pr auto review

你好！我是CodeGeeX。我已仔细审查了你提供的Git Diff。本次修改的核心是将依赖拉取与解析的逻辑（clearDependency 和 pullDependency）进行了重构与合并，提取到了 detail 命名空间中，并引入了本地/远程版本比较与降级逻辑，同时补充了单元测试。

整体来看，重构提高了代码的内聚性，消除了重复代码，且单元测试覆盖了核心分支。但在语法逻辑、代码质量和安全性方面，我发现了一些需要改进的地方，以下是详细的审查意见：

1. 语法与逻辑

问题 1.1：pullDependency 降级逻辑中 bestRef 的悬垂引用风险
在 pullDependency 中，存在如下代码：

bool preferRemote = remoteRef && (!localRef || remoteRef->version > localRef->version);
auto &bestRef = preferRemote ? remoteRef : localRef;

这里 bestRef 是一个引用。如果 preferRemote 为 true，则 bestRef 绑定到 remoteRef；如果为 false，则绑定到 localRef。
紧接着：

auto pullRes = pullResolvedRef(*bestRef, repo, module);
if (pullRes) {
    return std::move(*bestRef); // 【风险点】
}

如果 preferRemote 为 true 且 pullRes 失败，代码会进入降级逻辑：

if (preferRemote && localRef) {
    // ...
    auto fallbackRes = pullResolvedRef(*localRef, repo, module);

此时 localRef 仍然有效。但是，如果在前面 pullRes 成功时执行 return std::move(*bestRef);，因为 bestRef 是 remoteRef 的引用，std::move(*bestRef) 实际上移动了 remoteRef.value()。这虽然不会立刻导致崩溃（因为 remoteRef 依然持有该值），但语义不清晰，容易在后续维护中引发未定义行为。

改进建议： 避免使用引用指向不同的 optional，直接使用指针或通过解引用获取局部变量，使逻辑更清晰安全：

bool preferRemote = remoteRef && (!localRef || remoteRef->version > localRef->version);
package::Reference bestRefValue = preferRemote ? std::move(*remoteRef) : std::move(*localRef);

auto pullRes = pullResolvedRef(bestRefValue, repo, module);
if (pullRes) {
    return std::move(bestRefValue);
}

if (preferRemote && localRef) {
    LogW("failed to pull remote version {}, falling back to local: {}",
         bestRefValue.toString(),
         pullRes.error().message());
    auto fallbackRes = pullResolvedRef(*localRef, repo, module);
    if (fallbackRes) {
        return std::move(*localRef);
    }
    return LINGLONG_ERR("failed to pull local fallback version " + localRef->toString(),
                        fallbackRes);
}

return LINGLONG_ERR("failed to pull version " + bestRefValue.toString(), pullRes);

问题 1.2：pullResolvedRef 的降级判断依赖 getLayerDir 的错误返回
在 pullResolvedRef 中：

if (repo.getLayerDir(ref, module)) {
    return LINGLONG_OK;
}

原注释 // 如果依赖已存在，则直接使用 被删除了。如果 getLayerDir 因为权限问题、文件系统损坏等原因报错，这里也会进入拉取逻辑。虽然对于构建工具来说，拉取覆盖可能是一个合理的容错手段，但最好明确区分“不存在”和“读取失败”。

改进建议： 如果 getLayerDir 能区分 NotFound 和 IOError，建议仅在 NotFound 时跳过，否则可能掩盖文件系统权限问题。

2. 代码质量

问题 2.1：pullDependency 中 clearReference 的冗余调用
当 useRemote 为 true 时，代码先查询了一次本地 clearReference，然后又查询了一次远程 clearReference：

auto localResult = repo.clearReference(*fuzzyRef, { .forceRemote = false, .fallbackToRemote = false, ... });
// ...
auto remoteResult = repo.clearReference(*fuzzyRef, { .forceRemote = true, .fallbackToRemote = true, ... });

而在旧代码 ensureUtils 中，也是分两次查询的。但是，OSTreeRepo::clearReference 的选项中有一个 fallbackToRemote。如果本地不存在，设置 fallbackToRemote=true 可以一次性解决。
当然，当前逻辑是为了比较本地和远程的版本号，如果本地版本更高则优先用本地。这种设计是合理的，但建议在代码中补充注释说明为何需要两次 IO 查询，以免后续开发者误以为是冗余代码而“优化”掉。

问题 2.2：pullDependency 中 useRemote=false 时的行为差异
当 useRemote=false 时，如果本地不存在，直接报错返回。但在 pullResolvedRef 中，如果本地不存在，并没有尝试拉取。这是符合逻辑的。但是，在 buildStagePullDependency 中：

auto baseRef = detail::pullDependency(this->project->base, this->repo, "binary", !this->buildOptions.skipPullDepend);

如果 skipPullDepend 为 true，则 useRemote=false，此时如果本地没有缓存，会直接报错。这是符合预期的，但建议在 pullDependency 的函数声明处增加详细的 Doxygen 注释，说明 useRemote 参数不仅控制“是否从远程拉取”，还控制“是否尝试查询远程版本”。

问题 2.3：头文件中 detail 命名空间的闭合位置
在 linglong_builder.h 中：

namespace detail {
// ...
utils::error::Result<package::Reference> pullDependency(...);
} // namespace detail

把内部实现细节放到了头文件的 detail 中，这虽然是为了让单元测试 pull_dependency_test.cpp 能够访问，但这破坏了封装性，使得外部文件可以随意调用 detail 函数。

改进建议：
更好的做法是将 pullDependency 和 pullResolvedRef 的声明移到专门的内部头文件（如 linglong_builder_p.h 或 pull_dependency.h），或者在测试文件中通过引入源文件（#include "linglong/builder/linglong_builder.cpp"，虽然不优雅但在某些项目中可行）来测试。如果项目目前倾向于简单处理，现状也可接受，但需注意不要在业务代码中调用 detail::pullDependency。

3. 代码性能

问题 3.1：FuzzyReference::parse 的重复解析
在 pullDependency 中：

auto fuzzyRef = package::FuzzyReference::parse(fuzzyRefStr);

而在调用侧（如 ensureUtils）：

auto fuzzyRef = package::FuzzyReference::parse(id);
// ...
auto ref = detail::pullDependency(fuzzyRef->toString(), this->repo, "binary", true);

这里先将 id 解析成 FuzzyReference，然后又 toString()，传给 pullDependency 后再次 parse。字符串的序列化、反序列化和正则解析是存在性能开销的。

改进建议： 修改 pullDependency 的签名，直接接收 FuzzyReference，避免无谓的字符串转换和重新解析：

utils::error::Result<package::Reference> pullDependency(const package::FuzzyReference &fuzzyRef,
                                                        repo::OSTreeRepo &repo,
                                                        const std::string &module,
                                                        bool useRemote) noexcept;

这样在 ensureUtils 等调用处可以直接传入 *fuzzyRef。

4. 代码安全

问题 4.1：单元测试中的 Mock 绕过了虚函数机制的风险
在 pull_dependency_test.cpp 中：

class MockRepo : public repo::OSTreeRepo
{
    // ...
    MOCK_METHOD(utils::error::Result<void>,
                pull,
                (service::Task & taskContext,
                 const package::ReferenceWithRepo &refRepo,
                 const std::string &module),
                (override, noexcept));
};

为了使 Mock 生效，你在 ostree_repo.h 中给 getLayerDir 和 pull 加了 virtual 关键字。这是一个重大的设计变更。
OSTreeRepo 看起来是一个重量级的系统操作类，将其方法设为虚函数会：

1. 增加 vtable 开销（通常可忽略）。
2. 破坏 C++ 的静态分派优化。
3. 最重要的是，允许子类化并重写核心的拉取和读取逻辑，这在构建系统中可能带来供应链篡改的风险（如果有人恶意继承并替换逻辑）。

改进建议：
推荐使用依赖注入或接口隔离原则（ISP）。提取一个轻量级的接口类（如 IRefResolver 和 ILayerPuller），让 OSTreeRepo 实现这些接口，然后 pullDependency 依赖于这些接口而不是具体的 OSTreeRepo 基类。这样既方便了单元测试 Mock，又不需要将 OSTreeRepo 的核心方法开放为 virtual。

问题 4.2：测试中的硬编码临时目录

std::filesystem::path m_tmpDir{ "/tmp/ll-pull-dep-test" };

硬编码 /tmp 路径在多用户并发构建或测试时可能产生竞争条件，甚至被恶意利用进行符号链接攻击（虽然这里是测试代码，风险较低）。

改进建议： 使用系统提供的临时目录生成机制，确保每次运行路径唯一：

std::filesystem::path m_tmpDir = std::filesystem::temp_directory_path() / "ll-pull-dep-test-XXXXXX";
// 或者使用 QTemporaryDir

总结

本次重构方向非常正确，极大地提升了依赖拉取流程的内聚性和可维护性。建议优先修复 1.1 中的引用与移动语义问题，并考虑 3.1 的接口优化以提升性能。关于 4.1 的 virtual 化，如果团队目前接受为了测试而开放虚函数，则可以保留，但长期建议重构为接口依赖。

[gemini-code-assist]

Code Review

This pull request refactors the dependency pulling logic in linglong_builder.cpp by introducing detail::pullDependency and detail::pullResolvedRef helper functions, making OSTreeRepo methods virtual for mocking, and adding a comprehensive unit test suite. The review feedback suggests improving error propagation by passing the underlying result errors to LINGLONG_ERR when resolution fails, and replacing the hardcoded temporary directory path in the tests with QTemporaryDir to avoid concurrent execution conflicts.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[codecov]

Codecov Report

❌ Patch coverage is 52.83019% with 25 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...linglong/src/linglong/builder/linglong_builder.cpp	52.83%	24 Missing and 1 partial ⚠️

Files with missing lines	Coverage Δ
...s/linglong/src/linglong/builder/linglong_builder.h	0.00% <ø> (ø)
libs/linglong/src/linglong/repo/ostree_repo.h	27.27% <ø> (+18.18%)	⬆️
...linglong/src/linglong/builder/linglong_builder.cpp	6.19% <52.83%> (+3.46%)	⬆️

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[deepin-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dengbo11, reddevillg

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment