Bug 1662534
| Summary: | Review Request: liquidctl - A tool for controlling NZXT liquid coolers, fans, and LED strips | ||
|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | Artur Frenszek-Iwicki <fedora> |
| Component: | Package Review | Assignee: | Fabio Valentini <decathorpe> |
| Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
| Severity: | medium | Docs Contact: | |
| Priority: | medium | ||
| Version: | rawhide | CC: | decathorpe, package-review |
| Target Milestone: | --- | Flags: | decathorpe:
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: | 2019-01-18 01:37:56 UTC | 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
Artur Frenszek-Iwicki
2018-12-29 14:06:49 UTC
Taking this review. I have some initial comments:
- This is a pure-python package, so it must have "BuildArch: noarch".
This also means you have can drop the "global debug_package %{nil}".
- "Requires: python3" should be automatically generated, you don't need to specify it.
- "BuildRequires: python3-devel" includes "BuildRequires: python3", so you can drop the latter, too.
- This project has its sources published on pypi, you could use the official release tarball from there instead of the github tag archive.
- Is the python package supposed to be imported by other scripts other than "liquidctl" that's shipped with this package?
If so, you should consider splitting it off into a python3-liquidctl sub-package.
PS: I found that "pyp2rpm" is a really good tool for creating python packages for fedora - its output usually only requires minor edits for compliant fedora packages.
Thanks! I addressed most of the points. I don't think the package is _intended_ to be imported by other scripts, but the code is written clean and modular enough that it definitely can be used this way. spec: https://svgames.pl/fedora/liquidctl-1.1.0-2.spec srpm: https://svgames.pl/fedora/liquidctl-1.1.0-2.src.rpm koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=31837304 Package looks good now, with the exception of the wrong filename for the .spec file:
> Note: liquidctl-1.1.0-2.spec should be liquidctl.spec
Please fix that before importing the package. It would result in broken koji builds, AFAIK.
Also, "BuildArch: noarch" propagates from the main package to sub-packages, so specifying it twice isn't necessary - but it doesn't hurt, either, I guess.
And, I forgot to mention: Remove the executable bit from the .spec file, too. Thanks, I'll keep these in mind. (fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/liquidctl liquidctl-1.1.0-2.fc29 has been submitted as an update to Fedora 29. https://bodhi.fedoraproject.org/updates/FEDORA-2019-49bdb318d4 liquidctl-1.1.0-2.fc28 has been submitted as an update to Fedora 28. https://bodhi.fedoraproject.org/updates/FEDORA-2019-bd6d076007 liquidctl-1.1.0-2.fc28 has been pushed to the Fedora 28 testing repository. If problems still persist, please make note of it in this bug report. See https://fedoraproject.org/wiki/QA:Updates_Testing for instructions on how to install test updates. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2019-bd6d076007 liquidctl-1.1.0-2.fc29 has been pushed to the Fedora 29 testing repository. If problems still persist, please make note of it in this bug report. See https://fedoraproject.org/wiki/QA:Updates_Testing for instructions on how to install test updates. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2019-49bdb318d4 liquidctl-1.1.0-2.fc28 has been pushed to the Fedora 28 stable repository. If problems still persist, please make note of it in this bug report. liquidctl-1.1.0-2.fc29 has been pushed to the Fedora 29 stable repository. If problems still persist, please make note of it in this bug report. |