Bug 2274274
| Summary: | Review Request: smfc - control fans on Super Micro X10-X13 (and some X9) motherboards | ||
|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | Ewout van Mansom <ewout> |
| Component: | Package Review | Assignee: | Neil Hanlon <neil> |
| Status: | ASSIGNED --- | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
| Severity: | medium | Docs Contact: | |
| Priority: | unspecified | ||
| Version: | rawhide | CC: | neil, package-review |
| Target Milestone: | --- | Flags: | neil:
fedora-review?
|
| Target Release: | --- | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Whiteboard: | |||
| Fixed In Version: | Doc Type: | If docs needed, set a value | |
| Doc Text: | Story Points: | --- | |
| Clone Of: | Environment: | ||
| Last Closed: | Type: | --- | |
| Regression: | --- | Mount Type: | --- |
| Documentation: | --- | CRM: | |
| Verified Versions: | Category: | --- | |
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
| Cloudforms Team: | --- | Target Upstream Version: | |
| Embargoed: | |||
|
Description
Ewout van Mansom
2024-04-10 03:37:15 UTC
Upstream informed: https://github.com/petersulyok/smfc/issues/34 Upstream PR to automate RPM release management using tito: https://github.com/petersulyok/smfc/pull/35 (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.
|