Skip to content

feat: support run command in runtime or base environment#1669

Merged
dengbo11 merged 1 commit into
OpenAtom-Linyaps:masterfrom
reddevillg:run_noapp
Jun 5, 2026
Merged

feat: support run command in runtime or base environment#1669
dengbo11 merged 1 commit into
OpenAtom-Linyaps:masterfrom
reddevillg:run_noapp

Conversation

@reddevillg

Copy link
Copy Markdown
Collaborator

No description provided.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request generalizes the container layer and item retrieval logic by transitioning from app-specific layers to target layers (which can fall back to runtime or base layers). The review feedback focuses on improving error handling across these changes: specifically, the reviewer recommends preserving the underlying error context by passing .error() to the error-handling macros (LINGLONG_ERR and LINGLONG_ERRV) when getCachedTargetItem() or getTargetLayer() fails, and printing the error instead of silently returning -1 in Cli::run.

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.

Comment thread libs/linglong/src/linglong/cli/cli.cpp
Comment thread libs/linglong/src/linglong/cli/cli.cpp
Comment thread libs/linglong/src/linglong/cli/cli.cpp
Comment thread libs/linglong/src/linglong/package_manager/package_manager.cpp
Comment thread libs/linglong/src/linglong/runtime/container_builder.cpp
@codecov

codecov Bot commented Jun 4, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 2.22222% with 44 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
libs/linglong/src/linglong/runtime/run_context.cpp 5.00% 18 Missing and 1 partial ⚠️
libs/linglong/src/linglong/cli/cli.cpp 0.00% 17 Missing ⚠️
...g/src/linglong/package_manager/package_manager.cpp 0.00% 4 Missing ⚠️
...inglong/src/linglong/runtime/container_builder.cpp 0.00% 4 Missing ⚠️
Files with missing lines Coverage Δ
libs/linglong/src/linglong/runtime/run_context.h 76.92% <ø> (ø)
...g/src/linglong/package_manager/package_manager.cpp 0.81% <0.00%> (ø)
...inglong/src/linglong/runtime/container_builder.cpp 8.58% <0.00%> (ø)
libs/linglong/src/linglong/cli/cli.cpp 1.77% <0.00%> (-0.01%) ⬇️
libs/linglong/src/linglong/runtime/run_context.cpp 14.03% <5.00%> (-0.36%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: reddevillg <reddevillg@gmail.com>
@deepin-ci-robot

Copy link
Copy Markdown
Collaborator

deepin pr auto review

你好!我是CodeGeeX。我已仔细审查了你提供的Git Diff。本次代码变更的主要目的是将运行上下文中对“应用层”的硬依赖,泛化为对“目标层”的依赖,使得在没有应用层的情况下,可以回退使用运行时层或基础层。这是一个很好的功能增强,提升了程序的健壮性和灵活性。

以下是我对本次变更在语法逻辑、代码质量、代码性能和代码安全方面的详细审查意见及改进建议:

1. 语法与逻辑

  • LINGLONG_ERR 传参逻辑错误(重要)
    在多处修改中,当 targetItem 为空时,错误输出写为:

    this->printer.printErr(LINGLONG_ERRV("failed to get cached target item", targetItem));
    // 以及
    return LINGLONG_ERR("failed to get cached target item", targetItem);

    问题targetItem 此时的类型是 utils::error::Result<...>,且处于失败/空状态。将一个代表失败的 Result 对象作为异常信息传递给错误宏,不仅无法提供有用的调试上下文,还可能引发编译错误或输出无意义的对象地址。错误信息应该说明为什么失败,而不是把失败的结果本身传进去。
    建议LINGLONG_ERR 应该只传递字符串,或者传递底层的错误信息。修改为:

    this->printer.printErr(LINGLONG_ERRV("failed to get cached target item"));
    return LINGLONG_ERR("failed to get cached target item");
  • getTargetLayer() 返回值生命周期风险(重要)
    getTargetLayer() 返回 std::reference_wrapper<RuntimeLayer>,其内部引用的是 RunContext 的成员变量(如 appLayer, runtimeLayer 等)。
    问题:如果调用者将这个引用提取出来,并在 RunContext 对象销毁后继续使用,会产生悬垂引用。
    建议:目前代码中 container_builder.cpp 的用法是 targetLayer->get().getReference()...,属于短生命周期立即使用,是安全的。但建议在 getTargetLayer() 的头文件声明处增加注释,明确说明返回的引用生命周期与 RunContext 对象绑定,防止未来维护者误用。

  • getCurrentContainers 中的复杂三元表达式

    .package = !info->app.empty()
        ? info->app
        : (info->runtime && !info->runtime->empty() ? *info->runtime : info->base),

    问题:嵌套的三元运算符降低了代码的可读性。
    建议:可以将其提取为一个局部的 lambda 表达式或使用 if/else 块,使逻辑更清晰:

    auto getPackageName = [&info]() -> const auto& {
        if (!info->app.empty()) return info->app;
        if (info->runtime && !info->runtime->empty()) return *info->runtime;
        return info->base;
    };
    // ...
    .package = getPackageName(),

2. 代码质量

  • getTargetLayer()getCachedTargetItem() 中的代码重复
    这两个函数都使用了相同的 if (appLayer) ... else if (runtimeLayer) ... else if (baseLayer) 降级逻辑。
    建议:可以提取一个私有的辅助函数来获取优先级最高的层,减少代码重复。例如:

    RuntimeLayer* RunContext::getPrioritizedLayer() {
        if (appLayer) return &(*appLayer);
        if (runtimeLayer) return &(*runtimeLayer);
        if (baseLayer) return &(*baseLayer);
        return nullptr;
    }

    然后 getTargetLayer()getCachedTargetItem() 都可以基于此函数实现,逻辑将更加简洁。

  • Lambda 捕获优化
    run_context.cpp 中:

    auto findTargetLayer = [this](const std::string &targetRefStr) ...

    原代码捕获了 this_linglong_trace_message,现在去掉了 _linglong_trace_message 的捕获,这是正确的,因为函数内部重新声明了 LINGLONG_TRACE。保留 [this] 是必须且合理的。

3. 代码性能

  • 降级查找的性能损耗
    getTargetLayer()getCachedTargetItem() 现在最多需要进行三次条件判断(appLayer -> runtimeLayer -> baseLayer)。
    评价:这种降级查找带来的性能开销仅仅是几个指针的判空,相比于文件系统 I/O 和容器启动过程,这部分开销可以忽略不计。不需要为此做额外的性能优化。

4. 代码安全

  • std::reference_wrapper 的安全性
    正如语法逻辑中提到的,std::reference_wrapper 本质上是非拥有的引用。在 C++ 中,返回内部成员的引用包装器需要严格控制对象生命周期。
    建议:确保 RunContext 的移动和拷贝语义被正确处理(目前看 RunContext 可能是只移动或不可拷贝的,这是好习惯)。严禁将 getTargetLayer() 的返回值存储在比 RunContext 生命周期更长的变量中。

  • 空指针解引用风险
    container_builder.cpp 中:

    triplet = targetLayer->get().getReference().arch.getTriplet();

    此处 targetLayer 已经在前置条件中检查过 !targetLayer,因此这里是安全的。但 getReference()arch 是否一定有值,取决于业务逻辑的强保证。假设这些是必选字段,当前写法是安全的。

总结与修改示例

本次修改逻辑清晰,很好地支持了非 App 场景的运行。主要需要修正的是错误宏的传参问题,以及建议优化嵌套三元表达式。

针对 LINGLONG_ERR 的修改建议(涉及 cli.cpp, package_manager.cpp, run_context.cpp):

// 修改前
if (!targetItem) {
    this->printer.printErr(LINGLONG_ERRV("failed to get cached target item", targetItem));
    return -1;
}

// 修改后
if (!targetItem) {
    this->printer.printErr(LINGLONG_ERRV("failed to get cached target item"));
    return -1;
}

针对 getCurrentContainers 的修改建议(cli.cpp):

// 修改前
.package = !info->app.empty()
    ? info->app
    : (info->runtime && !info->runtime->empty() ? *info->runtime : info->base),

// 修改后 (假设 C++17,使用 std::string_view 或 const std::string& 避免拷贝)
const auto& pkg = !info->app.empty() 
    ? info->app 
    : (info->runtime && !info->runtime->empty() ? *info->runtime : info->base);

myContainers.emplace_back(api::types::v1::CliContainer{
  .id = std::move(info->containerID),
  .package = pkg, // 如果类型是 string,这里可能需要 std::string(pkg)
  .pid = container->pid,
});

希望这些审查意见对你有所帮助!如果有任何疑问,欢迎随时讨论。

@reddevillg reddevillg requested review from ComixHe and dengbo11 June 5, 2026 02:05
@deepin-ci-robot

Copy link
Copy Markdown
Collaborator

[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:

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

@dengbo11 dengbo11 merged commit c99f0a4 into OpenAtom-Linyaps:master Jun 5, 2026
16 of 17 checks passed
@reddevillg reddevillg deleted the run_noapp branch June 8, 2026 05:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants