fix: avoid mistakenly create seat group on debian-based systems#84
fix: avoid mistakenly create seat group on debian-based systems#84zccrs merged 1 commit intolinuxdeepin:masterfrom
Conversation
seatd uses video group instead of seat group on debian-based systems, so we need a specified version of sysuser.conf to avoid creating seat group mistakenly.
|
Hi @calsys456. Thanks for your PR. I'm waiting for a linuxdeepin member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR adjusts how systemd sysusers configuration is installed so that Debian-based systems use a Debian-specific sysusers config that avoids creating an incorrect File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Relying on
if(EXISTS "/etc/debian_version")at configure time couples the build behavior to the host system rather than the target; consider exposing an explicit CMake option or distro flag instead of probing the build machine’s filesystem. - The
configure_file/installlogic for the sysuser config is now duplicated in both branches of the Debian check; consider factoring this into a small helper or variable to reduce duplication and keep the Debian-specific difference (file name) more obvious.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Relying on `if(EXISTS "/etc/debian_version")` at configure time couples the build behavior to the host system rather than the target; consider exposing an explicit CMake option or distro flag instead of probing the build machine’s filesystem.
- The `configure_file`/`install` logic for the sysuser config is now duplicated in both branches of the Debian check; consider factoring this into a small helper or variable to reduce duplication and keep the Debian-specific difference (file name) more obvious.
## Individual Comments
### Comment 1
<location> `services/CMakeLists.txt:13-15` </location>
<code_context>
if(EXISTS "/etc/debian_version")
install(FILES debian.ddm.pam DESTINATION ${CMAKE_INSTALL_FULL_SYSCONFDIR}/pam.d RENAME ddm)
+
+ # In debian-based systems, seatd uses video group instead of seat
+ # group, avoid creating seat group mistakenly by specifying
+ # different sysuser.conf
+ configure_file(debian.ddm-sysuser.conf.in debian.ddm-sysuser.conf)
+ install(FILES "${CMAKE_CURRENT_BINARY_DIR}/debian.ddm-sysuser.conf" DESTINATION "${SYSTEMD_SYSUSERS_DIR}" RENAME dde.conf)
</code_context>
<issue_to_address>
**suggestion (typo):** Fix capitalization and comma splice in the new comment about Debian-based systems.
The current comment is a single run-on sentence: `In debian-based systems, seatd uses video group instead of seat group, avoid creating seat group mistakenly by specifying different sysuser.conf`. Please capitalize "Debian-based" and split it into two sentences, e.g.
`# In Debian-based systems, seatd uses the video group instead of the seat group.`
`# Avoid creating the seat group mistakenly by specifying a different sysuser.conf.`
```suggestion
# In Debian-based systems, seatd uses the video group instead of the seat group.
# Avoid creating the seat group mistakenly by specifying a different sysuser.conf.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -12,6 +9,14 @@ endif() | |||
|
|
|||
| if(EXISTS "/etc/debian_version") | |||
There was a problem hiding this comment.
不应该这样做，应该添加一个cmake参数，在debian/rules 打包时关闭这些能力，而且要在readme里说明。不过这个提交不用改，可以新起一个。
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: calsys456, zccrs 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 |
There was a problem hiding this comment.
Pull request overview
This PR adjusts the systemd sysusers configuration installed by ddm so Debian-based systems don’t inadvertently create/use a seat group (seatd uses the video group there), while keeping the existing sysusers behavior for non-Debian systems.
Changes:
- Add a Debian-specific sysusers template that omits
seatgroup membership. - Update
services/CMakeLists.txtto install the Debian vs non-Debian sysusers config based on Debian detection.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
services/debian.ddm-sysuser.conf.in |
New Debian-specific sysusers template without seat group membership. |
services/CMakeLists.txt |
Installs Debian-specific sysusers config on Debian-based systems; keeps existing config elsewhere. |
| #Type Name ID GECOS Home directory Shell | ||
| u dde - "DDM Greeter Account" ${STATE_DIR} - |
There was a problem hiding this comment.
New file debian.ddm-sysuser.conf.in doesn’t include an SPDX license identifier header. Project guidelines require newly added files to carry an SPDX line (typically GPL-2.0-or-later), so please add it at the top of this file.
中文：新添加的文件 debian.ddm-sysuser.conf.in 缺少 SPDX 许可证标识头。项目规范要求新增文件包含 SPDX 行（通常为 GPL-2.0-or-later），请在文件开头补充。
| if(EXISTS "/etc/debian_version") | ||
| install(FILES debian.ddm.pam DESTINATION ${CMAKE_INSTALL_FULL_SYSCONFDIR}/pam.d RENAME ddm) | ||
|
|
||
| # In debian-based systems, seatd uses video group instead of seat | ||
| # group, avoid creating seat group mistakenly by specifying | ||
| # different sysuser.conf | ||
| configure_file(debian.ddm-sysuser.conf.in debian.ddm-sysuser.conf) | ||
| install(FILES "${CMAKE_CURRENT_BINARY_DIR}/debian.ddm-sysuser.conf" DESTINATION "${SYSTEMD_SYSUSERS_DIR}" RENAME dde.conf) |
There was a problem hiding this comment.
The Debian vs non-Debian selection is based on if(EXISTS "/etc/debian_version"), which is evaluated on the build host. This can produce incorrect install artifacts when cross-compiling or building Debian packages in a non-Debian environment/container. Consider making this an explicit CMake option (overridable by packagers) or using a more target-oriented detection mechanism.
中文：这里用 if(EXISTS "/etc/debian_version") 在构建机上判断 Debian/非 Debian，这在交叉编译或在非 Debian 环境/容器里构建 Debian 包时可能生成错误的安装产物。建议改为可由打包者覆盖的 CMake 选项，或使用更面向目标系统的检测方式。
3 participants
seatd uses video group instead of seat group on debian-based systems, so we need a specified version of sysuser.conf to avoid creating seat group mistakenly.
Summary by Sourcery
Adjust systemd sysusers configuration to avoid creating an incorrect seat group on Debian-based systems.
Bug Fixes: