https://bugzilla.redhat.com/show_bug.cgi?id=2274274 Neil Hanlon <neil@xxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED --- Comment #3 from Neil Hanlon <neil@xxxxxxxx> --- (Skipping the review template) Great job, I'm excited to see smfc come to Fedora! Comments: 1. packaging guidelines require not using /opt (except in certain scenarios); You can use %{_sysconfdir} macro instead (which will expand to /etc). See https://docs.fedoraproject.org/en-US/packaging-guidelines/RPMMacros/ 2. Are there any tests which can be run? for python, we can at least use %pyproject_check_import macro if there aren't tests upstream. See https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_tests 3. in general (while replacing /opt/), you can use %{_sysconfdir} including for /etc/default/smfc -- so that if this changes in the future, the package can just pick it up without any additional effort. 4. There is one rpmlint error (ignoring the ones about /opt/ usage): smfc.noarch: E: description-line-too-long systemd service to control fans in CPU and HD zones with the help of IPMI on Super Micro X10-X13 (and some X9) motherboards. For this one, you just need to split it onto multiple lines wrapped at col 80. 5. Although the project is just a copy of the single file, we should still utilize the %build section to perform the build, as it is important to know whether the program actually runs ("compiles"); Plus, this allows us to run tests from upstream, and often catch issues with newer python versions (especially on, e.g., fedora rawhide). 6. related to 5, consider using the current python packaging macros (https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/) which can help autogenerate build/runtime requirements, as well as handle file lists and ensure that packages are installed to the location expected by Fedora. 7. Given your close work with upstream, consider using rpmautospec to simplify releases/commits. It might be worth looking into Packit.dev, in the future (though I've not tried it out myself). 8. systemd and bash are part of @core, so do not need to be Required. For python3, you should not need this for Fedora either, since all available python versions for supported releases are >3.7. If you plan to bring this to EPEL 8, it would be OK to add a Requires on there since platform-python is only 3.6.8. In short, I think the requires/Recommends/BuildRequires can be shortened to: ``` Requires: ipmitool BuildRequires: python3-devel BuildRequires: systemd-rpm-macros ``` 9. Instead of mkdir & install, you can use the `-D` flag on `install` to create leading directories. -- 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 https://bugzilla.redhat.com/show_bug.cgi?id=2274274 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202274274%23c3 -- _______________________________________________ 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 Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
