Skip to content

Conversation

@yixinshark
Copy link
Contributor

@yixinshark yixinshark commented Jul 14, 2025

Changed the symlink target in CMakeLists.txt from dde-session-initialized.target to dde-session-core.target. Updated the service file to reflect the new dependencies, replacing references to dde-session-initialized.target with dde-session-core.target. This ensures proper service management and initialization order.

Log: as title

Summary by Sourcery

Update systemd service dependencies for ApplicationManager1 to depend on dde-session-core.target instead of dde-session-initialized.target.

Bug Fixes:

  • Replace references to dde-session-initialized.target with dde-session-core.target in the systemd service file

Build:

  • Adjust CMakeLists install_symlink target from dde-session-initialized.target.wants to dde-session-core.target.wants

@sourcery-ai
Copy link

sourcery-ai bot commented Jul 14, 2025

Reviewer's Guide

The pull request adjusts the systemd target dependency for ApplicationManager1 by changing references from dde-session-initialized.target to dde-session-core.target in both the CMake installation step and the service unit template, ensuring proper initialization order.

File-Level Changes

Change Details Files
Update symlink target in CMakeLists.txt to dde-session-core.target
  • Replaced install_symlink destination from dde-session-initialized.target.wants to dde-session-core.target.wants
misc/CMakeLists.txt
Modify service unit dependencies in org.desktopspec.ApplicationManager1.service.in
  • Replaced occurrences of dde-session-initialized.target with dde-session-core.target in After and Wants directives
misc/systemd/user/org.desktopspec.ApplicationManager1.service.in

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @yixinshark - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@deepin-bot
Copy link

deepin-bot bot commented Jul 31, 2025

TAG Bot

New tag: 1.2.34
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #287

@deepin-bot
Copy link

deepin-bot bot commented Sep 4, 2025

TAG Bot

New tag: 1.2.35
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #290

@deepin-bot
Copy link

deepin-bot bot commented Oct 10, 2025

TAG Bot

New tag: 1.2.36
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #292

@deepin-bot
Copy link

deepin-bot bot commented Nov 27, 2025

TAG Bot

New tag: 1.2.38
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #303

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request updates the systemd service dependencies for ApplicationManager1 from dde-session-initialized.target to dde-session-core.target, reflecting a change in the session initialization architecture. The changes ensure that the ApplicationManager service integrates with the new session target structure.

Key Changes:

  • Updated systemd service file to use dde-session-core.target instead of dde-session-initialized.target
  • Removed the Requisite dependency directive while keeping PartOf and Before directives
  • Updated CMakeLists.txt symlink installation to place the service in the correct target's wants directory

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
misc/systemd/user/org.desktopspec.ApplicationManager1.service.in Updated service dependencies to reference dde-session-core.target; removed Requisite directive
misc/CMakeLists.txt Updated symlink target from dde-session-initialized.target.wants to dde-session-core.target.wants

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@deepin-bot
Copy link

deepin-bot bot commented Dec 11, 2025

TAG Bot

New tag: 1.2.39
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #307

@deepin-bot
Copy link

deepin-bot bot commented Dec 18, 2025

TAG Bot

New tag: 1.2.40
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #310

@deepin-bot
Copy link

deepin-bot bot commented Dec 19, 2025

TAG Bot

New tag: 1.2.41
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #312

@zccrs
Copy link
Member

zccrs commented Dec 25, 2025

这个提交还需要吗?怎么不加reviewer?

@deepin-bot
Copy link

deepin-bot bot commented Dec 25, 2025

TAG Bot

New tag: 1.2.42
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #318

@deepin-bot
Copy link

deepin-bot bot commented Jan 23, 2026

TAG Bot

New tag: 1.2.43
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #323

@deepin-bot
Copy link

deepin-bot bot commented Jan 29, 2026

TAG Bot

New tag: 1.2.44
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #327

@yixinshark yixinshark requested review from BLumia and fly602 February 3, 2026 11:52
Changed the symlink target in CMakeLists.txt from dde-session-initialized.target to dde-session-core.target. Updated the service file to reflect the new dependencies, replacing references to dde-session-initialized.target with dde-session-core.target. This ensures proper service management and initialization order.

Log: as title
@deepin-ci-robot
Copy link

deepin pr auto review

这段代码修改主要是将一个 systemd 服务单元的符号链接从 dde-session-initialized.target.wants 修改为 dde-session-core.target.wants

以下是针对该修改的详细审查意见,包括语法逻辑、代码质量、代码性能和代码安全四个方面:

1. 语法逻辑

  • 审查结果:通过
  • 分析
    • 从 CMake 语法角度来看,install_symlink 是一个自定义命令(通常在项目的其他 CMake 文件中定义),传递给它的两个参数 org.desktopspec.ApplicationManager1.servicedde-session-core.target.wants 格式正确,符合字符串参数的要求。
    • 从 Systemd 逻辑来看,将服务链接放入 .wants 目录意味着该服务被作为依赖添加到相应的 Target 中。修改后的逻辑表明 org.desktopspec.ApplicationManager1.service 将在 dde-session-core.target 启动时被启动。

2. 代码质量

  • 审查结果:良好
  • 分析
    • 意图清晰:修改明确地改变了服务启动的触发点。从名称推断,dde-session-core(核心会话)比 dde-session-initialized(已初始化会话)更早或更基础。这意味着 ApplicationManager1 被移动到了启动流程的更早阶段,这通常是为了确保应用管理器在依赖它的其他服务(如桌面面板、任务栏等)启动之前就已经准备就绪。
    • 一致性:确保项目中其他类似的初始化服务也是挂载到 dde-session-core.target 下的,以保持启动顺序的一致性。

3. 代码性能

  • 审查结果:影响极小/无
  • 分析
    • 这段代码仅在构建和安装阶段执行,不会影响运行时性能。
    • 在运行时,Systemd 解析符号链接的开销极小,可以忽略不计。唯一的性能影响在于系统启动顺序的改变:如果 ApplicationManager1 启动较慢,将其移至更早的 core 阶段可能会轻微延长用户感知到的“桌面加载时间”,或者反之,如果它阻塞了关键路径,可能会影响系统启动速度。

4. 代码安全

  • 审查结果:需注意依赖关系
  • 分析
    • 循环依赖风险:将服务前移到 dde-session-core.target 必须确保 ApplicationManager1 自身不依赖于 dde-session-core 之后才启动的服务。如果 ApplicationManager1 依赖于某个在 dde-session-initialized 阶段才启动的服务,这会导致启动失败或死锁。
    • 权限与上下文:确保 ApplicationManager1core 阶段启动时,所需的所有系统资源(如 D-Bus 总线、文件系统挂载点、网络等)已经就绪。如果在资源未就绪时启动,可能导致服务崩溃或产生安全上下文错误。

综合改进建议

  1. 验证启动顺序

    • 建议检查 org.desktopspec.ApplicationManager1.service 文件内部的 After=Requires= 指令。确保它没有显式要求 dde-session-initialized.target,否则需要同步修改该服务文件,以避免逻辑冲突。
  2. 文档化变更

    • 建议在提交信息或相关文档中注明此次修改的原因(例如:“为了确保应用管理器在桌面环境基础组件加载前启动”),以便后续维护。
  3. 测试建议

    • 在修改后进行完整的系统启动测试,观察日志(journalctl -xe),确认 ApplicationManager1 成功启动且没有报错。
    • 检查是否有其他服务因为 ApplicationManager1 的位置变动而启动失败。

总结:代码修改本身没有语法错误,符合 CMake 规范。主要风险在于 Systemd 服务启动顺序的变更可能引发的依赖问题。请务必确认 ApplicationManager1 的依赖项是否支持在 core 阶段运行。

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: BLumia, yixinshark

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

@yixinshark yixinshark merged commit fb2716e into linuxdeepin:master Feb 4, 2026
17 checks passed
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.

4 participants