fix: avoid mistakenly create seat group on debian-based systems#84
fix: avoid mistakenly create seat group on debian-based systems#84calsys456 wants to merge 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.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: calsys456 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 |
|
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
|
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.
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: