Conversation
dougsland
requested review from
Yarboa,
aesteve-rh,
kleinffm,
nsednev,
pengshanyu and
rhatdan
as code owners
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR updates the qm-wayland RPM spec to stop owning the upstream systemd-user PAM file while still deploying a custom configuration by installing a suffixed copy and swapping it in via scriptlets during install and uninstall. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary of ChangesHello @dougsland, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the RPM spec file for Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 2 issues, and left some high level feedback:
- The %post script unconditionally overwrites /etc/pam.d/systemd-user with the qm-wayland variant on install/upgrade; consider using a drop-in or include mechanism instead of replacing the main PAM file to avoid clobbering local or upstream changes.
- When restoring systemd-user.rpmsave in %postun, you may overwrite a newer version provided by systemd or local modifications; consider restoring only if the current file matches the qm-wayland variant or if it doesn’t exist.
- To preserve file metadata, you might want to use cp -p (and similarly ensure permissions/ownership are correct) instead of cp -f when copying the PAM configuration in %post.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The %post script unconditionally overwrites /etc/pam.d/systemd-user with the qm-wayland variant on install/upgrade; consider using a drop-in or include mechanism instead of replacing the main PAM file to avoid clobbering local or upstream changes.
- When restoring systemd-user.rpmsave in %postun, you may overwrite a newer version provided by systemd or local modifications; consider restoring only if the current file matches the qm-wayland variant or if it doesn’t exist.
- To preserve file metadata, you might want to use cp -p (and similarly ensure permissions/ownership are correct) instead of cp -f when copying the PAM configuration in %post.
## Individual Comments
### Comment 1
<location> `rpm/wayland/qm-wayland.spec:74-79` </location>
<code_context>
%post
+# Backup original systemd-user PAM config and replace with our version
+if [ $1 -eq 1 ] || [ $1 -eq 2 ]; then
+ if [ -f %{_sysconfdir}/pam.d/systemd-user ] && [ ! -L %{_sysconfdir}/pam.d/systemd-user ]; then
+ if [ ! -f %{_sysconfdir}/pam.d/systemd-user.rpmsave ]; then
+ cp -p %{_sysconfdir}/pam.d/systemd-user %{_sysconfdir}/pam.d/systemd-user.rpmsave
+ fi
+ fi
+ cp -f %{_sysconfdir}/pam.d/systemd-user.qm-wayland %{_sysconfdir}/pam.d/systemd-user
+fi
%systemd_post qm-dbus.socket
</code_context>
<issue_to_address>
**issue (bug_risk):** Unconditional overwrite may modify the target of an existing symlink for systemd-user.
The backup step intentionally skips `systemd-user` when it’s a symlink, but the final `cp -f` always runs. If `systemd-user` is a symlink owned by another package or the admin, this will overwrite the symlink’s target instead of replacing a regular file. Please either skip this `cp` when `systemd-user` is a symlink or guard it (e.g., only run `cp` when `! -L`).
</issue_to_address>
### Comment 2
<location> `rpm/wayland/qm-wayland.spec:123` </location>
<code_context>
%{_unitdir}/wayland-session.service
%{_unitdir}/wayland.socket
-%{_sysconfdir}/pam.d/systemd-user
+%{_sysconfdir}/pam.d/systemd-user.qm-wayland
%{_sysconfdir}/pam.d/wayland-autologin
# Weston compositor configuration
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider marking `systemd-user.qm-wayland` as a config file to preserve local edits.
Because this file is used as the source for `/etc/pam.d/systemd-user`, any local changes would be overwritten on upgrade. Declaring it as `%config` (likely `%config(noreplace)`) would let RPM preserve admin modifications across package updates.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly avoids owning the systemd-user PAM configuration file by packaging a custom version and using scriptlets to manage it. The approach of backing up the original file on install and restoring it on uninstall is sound.
I've found a potential issue in the %post scriptlet where it could incorrectly handle a pre-existing symbolic link for systemd-user, leading to its replacement without a way to restore it. I've suggested a change to make this safer. I also have a minor suggestion to improve the robustness of the scriptlets by quoting variables. Overall, this is a good change that addresses the file ownership problem.
systemd-user is owned by systemd, we cannot define
%{_sysconfdir}/pam.d/systemd-user in the files.
Signed-off-by: Albert Esteve <aesteve@redhat.com>
Signed-off-by: Douglas Schilling Landgraf <dougsland@redhat.com>
aesteve-rh
force-pushed
the
qmwayland-systemd
branch
from
d735354 to
3e0897e
Compare
|
@dougsland try to build the packages manually |
|
/packit test |
believe the failures are not related to the patch still validating the errors and re-running the jobs. |
|
ci/cd errors not related to the patch. |
2 participants
systemd-user is owned by systemd, we cannot define %{_sysconfdir}/pam.d/systemd-user in the files.
Fix #965
Summary by Sourcery
Adjust qm-wayland packaging to ship a separate PAM configuration for systemd-user without owning the upstream-managed file.
Enhancements: