https://bugzilla.redhat.com/show_bug.cgi?id=1832623 --- Comment #9 from Michel Alexandre Salim <michel@xxxxxxxxxxxxxxx> --- (In reply to Stuart D Gathman from comment #8) Thanks for the thorough review! Going to comment in-line. > Package Review > ============== > > Legend: > [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated > > > Issues: > ======= > - systemd_user_post is invoked in %post and systemd_user_preun in %preun > for Systemd user units service files. > Note: Systemd user unit service file(s) in psi-notify > See: > https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/ > #_user_units > > Reviewer note: the user service is enabled by default for all users. I can > see > the reason for this (we are installing an extension to the desktop > environments), but maybe the description should mention this. > They should not be; from the docs: "These enable and disable user units according to presets". And the presets default to disabling every unit that's not whitelisted; so the intention is a sysadmin / org can ship a custom preset and pre-enable some services. ~ ❯ systemctl --user status psi-notify ● psi-notify.service Loaded: loaded (/usr/lib/systemd/user/psi-notify.service; disabled; vendor preset: disabled) ~ ❯ ls /usr/lib/systemd/user-preset 90-default-user.preset 90-systemd.preset 99-default-disable.preset ~ ❯ cat /usr/lib/systemd/user-preset/99-default-disable.preset disable * ❯ rpm -E '%systemd_user_post' if [ $1 -eq 1 ] && [ -x /usr/bin/systemctl ] ; then # Initial installation /usr/bin/systemctl --no-reload preset \--global || : fi ~ ❯ rpm -E '%systemd_user_preun' error: This macro requires some arguments ~ ❯ rpm -E '%systemd_user_preun psi-notify' if [ $1 -eq 0 ] && [ -x /usr/bin/systemctl ] ; then # Package removal, not upgrade /usr/bin/systemctl --global disable psi-notify || : fi > - the (one) source file does not have a copyright in the source. Advise > upstream to include a copyright notice and reference the license in the > source. Or is this no longer recommended? That's definitely recommended. I'll notify upstream. Thanks for noticing! > > - Technically there should be a Requires: systemd - although this seems > rather pedantic as all Fedora systems, including EPEL as of Nov, have > systemd. Maybe this is a bug in fedora-review. It complains > that e.g. /usr/lib/systemd has no documented owner. Yes, need to include something for file ownership. > > - Should demo.gif be included in %doc ? Will add that. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx
