Bug 1662534 - Review Request: liquidctl - A tool for controlling NZXT liquid coolers, fans, and LED strips
Summary: Review Request: liquidctl - A tool for controlling NZXT liquid coolers, fans,...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Fabio Valentini
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2018-12-29 14:06 UTC by Artur Frenszek-Iwicki
Modified: 2019-01-18 02:13 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2019-01-18 01:37:56 UTC
Type: ---
Embargoed:
decathorpe: fedora-review+


Attachments (Terms of Use)

Description Artur Frenszek-Iwicki 2018-12-29 14:06:49 UTC
spec: https://svgames.pl/fedora/liquidctl-1.1.0-1.spec
srpm: https://svgames.pl/fedora/liquidctl-1.1.0-1.src.rpm
koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=31716192

Description: liquidctl is a tool for controlling various settings of NZXT devices, such as: liquid cooler pump speed, case fan speed, and colors of RGB LED strips.

Fedora Account System Username: suve


This is my first time packaging python stuff, so it's rather unlikely there aren't any issues.

Comment 1 Fabio Valentini 2018-12-29 14:39:04 UTC
Taking this review.

Comment 2 Fabio Valentini 2019-01-02 00:16:46 UTC
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.

Comment 3 Artur Frenszek-Iwicki 2019-01-05 17:15:42 UTC
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

Comment 4 Fabio Valentini 2019-01-07 18:13:05 UTC
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.

Comment 5 Fabio Valentini 2019-01-07 18:13:38 UTC
And, I forgot to mention: Remove the executable bit from the .spec file, too.

Comment 6 Artur Frenszek-Iwicki 2019-01-07 18:28:46 UTC
Thanks, I'll keep these in mind.

Comment 7 Gwyn Ciesla 2019-01-07 19:33:48 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/liquidctl

Comment 8 Fedora Update System 2019-01-08 17:24:16 UTC
liquidctl-1.1.0-2.fc29 has been submitted as an update to Fedora 29. https://bodhi.fedoraproject.org/updates/FEDORA-2019-49bdb318d4

Comment 9 Fedora Update System 2019-01-08 17:24:27 UTC
liquidctl-1.1.0-2.fc28 has been submitted as an update to Fedora 28. https://bodhi.fedoraproject.org/updates/FEDORA-2019-bd6d076007

Comment 10 Fedora Update System 2019-01-10 20:49:23 UTC
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

Comment 11 Fedora Update System 2019-01-10 22:15:01 UTC
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

Comment 12 Fedora Update System 2019-01-18 01:37:56 UTC
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.

Comment 13 Fedora Update System 2019-01-18 02:13:19 UTC
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.


Note You need to log in before you can comment on or make changes to this bug.