Skip to content

fix: register ddm as a display manager via debconf - #89

Merged
wineee merged 1 commit into
linuxdeepin:masterfrom
wineee:fix-me
May 9, 2026
Merged

wineee merged 1 commit into
linuxdeepin:masterfrom
wineee:fix-me

Conversation

@wineee

@wineee wineee commented May 9, 2026

Copy link
Copy Markdown
Member

Reinstalling ddm would reset the default display manager to lightdm because ddm did not participate in the Debian display manager selection mechanism. Add missing debconf templates and config script (modeled after lightdm) to properly register ddm, and uncomment Provides: x-display-manager in debian/control.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Sorry @wineee, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@deepin-ci-robot

Copy link
Copy Markdown

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the Debian packaging for ddm so it participates in Debian’s display manager selection mechanism (debconf) and can be selected/kept as the default display manager across reinstalls.
本 PR 更新 ddm 的 Debian 打包,使其参与 Debian 的显示管理器选择机制(debconf),从而在重装时也能被选为并保持默认显示管理器。

Changes:

  • Add debian/ddm.templates with ddm/daemon_name and shared/default-x-display-manager debconf questions.
    新增 debian/ddm.templates,提供 ddm/daemon_name 与 shared/default-x-display-manager 的 debconf 问题模板。
  • Add debian/ddm.config to drive the default display manager selection during configuration.
    新增 debian/ddm.config,在配置阶段引导选择默认显示管理器。
  • Enable Provides: x-display-manager for the ddm package.
    为 ddm 包启用 Provides: x-display-manager。

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
debian/ddm.templates Introduces debconf templates needed for the shared display-manager selection UI.
debian/ddm.config Adds a debconf config script to sync choices and handle default-display-manager switching.
debian/control Declares ddm as an x-display-manager provider so it participates in selection.

Comment thread debian/ddm.config Outdated
Comment thread debian/ddm.config
Comment thread debian/ddm.templates
@wineee
wineee marked this pull request as ready for review May 9, 2026 03:26

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Sorry @wineee, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@wineee wineee changed the title fix: register ddm as default display manager via debconf fix: register ddm as a display manager via debconf May 9, 2026
Groveer
Groveer previously approved these changes May 9, 2026
Reinstalling ddm would reset the default display manager to lightdm
because ddm did not participate in the Debian display manager selection
mechanism. Add missing debconf templates and config script (modeled
after lightdm) to properly register ddm, and uncomment Provides:
x-display-manager in debian/control.
@deepin-ci-robot

Copy link
Copy Markdown

deepin pr auto review

这份 diff 展示了对 Debian 软件包 ddm 的维护脚本(config, prerm, postrm)和控制文件(control, templates)的修改。主要目的是让 ddm 能够正确注册为系统的默认显示管理器,并处理多架构环境下的包名问题。

以下是针对语法逻辑、代码质量、代码性能和代码安全的详细审查意见:

1. 语法与逻辑审查

整体逻辑:
修改后的代码逻辑是通用的 Debian 显示管理器切换逻辑(参考了 lightdm 和 gdm 的实现)。主要流程包括:

  1. config: 询问用户默认显示管理器,并处理架构限定符。
  2. prerm: 在卸载前,如果当前是默认管理器,则尝试切换到另一个已安装的管理器。
  3. postrm: 在卸载后,更新 systemd 的符号链接指向新的管理器。

具体问题与建议:

  • debian/ddm.config:

    • 架构限定符处理: sed 's/:[[:alnum:]_-]*//g' 用于移除 :amd64 等后缀。逻辑正确,但正则表达式 [:alnum:]_- 包含了连字符和下划线。虽然合法,但通常架构限定符只包含字母和数字。不过为了稳健性,保留它是可以的。
    • 文件移动: mv "$DEFAULT_DISPLAY_MANAGER_FILE" "${DEFAULT_DISPLAY_MANAGER_FILE}.dpkg-tmp"。这行代码在 config 阶段执行。通常 config 脚本不应修改文件系统状态,尤其是移动文件。文件的实际替换通常由 postinst 脚本根据 config 的输出结果来完成。这里的逻辑似乎是想标记文件将被更新,但直接 mv 可能会导致如果后续安装中断,文件状态不一致。
    • 建议: 检查 config 脚本是否真的需要在这里 mv 文件,还是应该只记录状态,让 postinst 去做实际的文件写入。
  • debian/ddm.prerm:

    • 架构限定符处理: sed 's/:[[:alnum:]_-]*//g' 与 config 中保持一致,很好。
    • 变量引用: echo "$RET" > "$DEFAULT_DISPLAY_MANAGER_FILE" 中,$DEFAULT_DISPLAY_MANAGER_FILE 没有加引号(虽然在这里路径通常不含空格,但加上引号更安全)。代码中其他地方都加了引号,这里漏了。
    • 回退逻辑: echo "/usr/bin/$NEW_DM" > "$DEFAULT_DISPLAY_MANAGER_FILE"。当无法获取 daemon_name 时,回退到 /usr/bin/$NEW_DM。这是一个合理的假设,但最好确认 ddm 的二进制文件确实在这个路径。
  • debian/ddm.postrm:

    • Systemd 链接更新: 逻辑是读取 /etc/X11/default-display-manager,获取新的 DM 名称,然后更新 /etc/systemd/system/display-manager.service 的软链接。这是处理 systemd 集成的标准做法。
    • Masked 检查: if [ ... "$(readlink "$DEFAULT_SERVICE")" = /dev/null ]。检查服务是否被 mask 是个好习惯。
  • debian/control:

    • Provides: x-display-manager: 取消注释是正确的,这是让系统识别该包为显示管理器的关键。
    • Multi-Arch: same: 被移除了。如果 ddm 包含架构相关的二进制文件,移除 same 是正确的(意味着不能同时安装多个架构的版本)。如果它是纯数据包或库,可能需要 same。鉴于它是一个显示管理器(通常包含二进制),移除 same 是合理的。

2. 代码质量

  • 一致性: 代码中大量使用了 set -e,这是 Debian 维护脚本的标准做法,遇到错误立即退出,很好。
  • 可读性: 变量命名清晰(如 DEFAULT_DISPLAY_MANAGER_FILE, CURRENT_DEFAULT)。
  • 注释: 代码中有必要的注释,解释了为什么要处理架构限定符,这很有帮助。
  • Shell 脚本风格:
    • 在 prerm 中,缩进使用了空格( db_get),但在 config 中没有严格对齐。建议统一使用空格缩进。
    • sed 命令被重复使用。虽然逻辑简单,但如果需要修改正则,需要改多处。可以考虑定义一个函数或变量,但在脚本中这种简单用法也是可以接受的。

3. 代码性能

  • Shell 调用: 脚本调用了多次 db_get, db_metaget 等 debconf 命令。这是 debconf 交互的必要开销,无法避免。
  • 文件操作: grep, cat, head 的组合使用是高效的。
  • 总体: 维护脚本通常只在包安装/卸载时运行一次,性能不是主要瓶颈,代码的可读性和正确性更重要。

4. 代码安全

  • 引号使用: 大部分变量都正确使用了双引号,防止了单词分割和通配符扩展。
    • 例外: 在 prerm 中 if [ "$(cat $DEFAULT_DISPLAY_MANAGER_FILE)" = "$RET" ]; then,cat 命令内的变量未加引号。如果路径包含空格(虽然不太可能),会导致错误。
    • 修正建议: 改为 if [ "$(cat "$DEFAULT_DISPLAY_MANAGER_FILE")" = "$RET" ]; then。
  • 文件覆盖: echo "$RET" > "$DEFAULT_DISPLAY_MANAGER_FILE" 会直接覆盖文件。由于这是在 prerm 阶段且是为了切换默认管理器,这是预期行为,但确保文件权限正确很重要。
  • 符号链接处理: ln -sf 是强制覆盖,在切换默认管理器时是必要的。
  • 输入验证: 脚本依赖于 debconf 返回的值。虽然 debconf 通常比较可靠,但 NEW_DM 或 CURRENT_DM 如果为空或包含恶意字符,可能会导致问题。不过,由于这些值来源于已安装包的列表,风险较低。

总结与改进建议

这份 diff 主要是为了让 ddm 能够正确参与 Debian 的显示管理器切换机制。代码整体质量不错,逻辑清晰。

主要改进建议:

  1. 修复 prerm 中的引号缺失:
    在 debian/ddm.prerm 文件中,找到这行:

    if [ "$(cat $DEFAULT_DISPLAY_MANAGER_FILE)" = "$RET" ]; then

    修改为:

    if [ "$(cat "$DEFAULT_DISPLAY_MANAGER_FILE")" = "$RET" ]; then
  2. 审查 debian/ddm.config 中的文件操作:
    建议确认 mv "$DEFAULT_DISPLAY_MANAGER_FILE" ... 是否应该在 config 阶段执行。标准的 Debian 打包实践通常将文件系统的持久化修改放在 postinst 脚本中。config 脚本应仅负责收集用户输入。如果 config 阶段失败或中断,移动文件可能会导致系统处于不一致的状态。建议将文件移动逻辑移至 postinst,或者仅在此处设置一个标记。

  3. 错误处理增强:
    在 postrm 中,如果 readlink 或 ln 失败,脚本会因 set -e 退出。这通常是好的,但可以考虑在关键操作失败时输出更具体的错误信息到 stderr,方便调试。

  4. sed 正则表达式优化:
    sed 's/:[[:alnum:]_-]*//g' 中的 _- 在字符类中是表示范围(从下划线到连字符),这在 ASCII 码中顺序是反的(_ 是 95,- 是 45)。虽然 GNU sed 可能能容忍,但为了最大兼容性,建议将连字符放在字符类的开头或结尾,或者转义。
    改为:sed 's/:[[:alnum:]_-]*//g' -> sed 's/:[[:alnum:]_-]\+//g' (使用 \+ 表示一个或多个,更准确) 或者保持原样但调整顺序:sed 's/:[[:alnum:]_-]*//g' -> sed 's/:[_-[:alnum:]]*//g'。

除此之外,代码已经相当成熟,符合 Debian 维护脚本的标准规范。

@deepin-ci-robot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Groveer, wineee, Zeno-sole

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

@wineee
wineee merged commit fe50059 into linuxdeepin:master May 9, 2026
10 checks passed
@wineee
wineee deleted the fix-me branch May 9, 2026 06:39
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.

5 participants