https://bugzilla.redhat.com/show_bug.cgi?id=1756582 --- Comment #4 from Christopher Engelhard <ce@xxxxxxx> --- Hi Michal, thanks for the review & detailed feedback. Issues: ======= 1) The package does not own '/usr/libexec/sshguard' directory Reply: Fixed. 2) systemd vs sysvinit 2.1) Fix the condition Reply: I swear, one day I will wtite a flawless rpm macro ... I reverted it to checking for el6, specifically. I think the package will also work on older RHELs, but I have never checked. 2.2) Logrotate is used only with sysvinit and not with systemd - is that an intention? Reply: Yes. Usually, sshguard expects to run with systemd and logs via journald. I only have it log to a file on SysVInit. 2.3) When the package is build with systemd service instead of sysvinit script, do you think it still worth to ship the example [...] file [...]? Reply: You're right. I don't remember why I switched to creating my own service file. I changed it back to using upstream's example. 2.4) Please note, that the base package contains systemd service file, but the package does not require systemd. [...] Reply: Fixed. It should require systemd unless run on a sysvinit system. 2.5) The systemd service contains e.g. "After=firewalld.service". If the service is not present or not started, this won't have any effect. [...] Check if that's OK. Reply: I'd say it's OK, but that is debatable. The After= lines are really just there to ensure proper ordering IF the backend is present. If people use the backend-subpackages, the appropriate service will be pulled in as a dependency. If they're not, they will in any case have to configure sshguard themselves, including which backend to use, so I think it's fair to assume they'll also install that backend, or if not, understand what went wrong. Please advise. 3) I'd suggest to have every changelog entry (each header) separated by a newline [...]. Reply: Fixed. 4) I saw two bundled libraries that I suspect they are bundled, can you please confirm? Reply: Yes, that seems to be the case, I overlooked that. I don't think they're in Fedora, I'll package them separately if necessary and get back to you. Should I open a new review request for each and link to them from here? spec correcting {1-3): https://copr-be.cloud.fedoraproject.org/results/lcts/sshguard-testing/fedora-rawhide-x86_64/01042441-sshguard/sshguard.spec diff: https://gitlab.com/lcts-rpm/sshguard/compare/sshguard-2.4.0-8...master src package: https://copr-be.cloud.fedoraproject.org/results/lcts/sshguard-testing/fedora-rawhide-x86_64/01042441-sshguard/sshguard-2.4.0-8.git.5.9154ef5.fc32.src.rpm I'll make a new package release once I've taken care of (4). -- 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
