Bug 668197 - Review Request: ledctl - LED control app for Intel(R) storage controllers
Summary: Review Request: ledctl - LED control app for Intel(R) storage controllers
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Ondrej Vasik
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-01-08 21:48 UTC by Jiri Moskovcak
Modified: 2015-02-01 22:53 UTC (History)
4 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2011-02-16 09:46:40 UTC
Type: ---
Embargoed:
ovasik: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)

Description Jiri Moskovcak 2011-01-08 21:48:48 UTC
Spec URL: http://jmoskovc.fedorapeople.org/ledctl.spec
SRPM URL: http://jmoskovc.fedorapeople.org/ledctl-0.1-1.src.rpm
Description: 
The ledctl is user space application design to control LEDs associated
with each slot in an enclosure or a drive bay. There are two types of
system: 2-LEDs system (Activity LED, Status LED) and 3-LEDs system
(Activity LED, Locate LED, Fail LED). User must have root privileges to
use this application.

The ledctl application uses SGPIO and SES-2 protocol to control LEDs.
The program implements IBPI patterns of SFF-8489 specification for
SGPIO.  Please note some enclosures do not stick close to SFF-8489
specification.  It might happen that enclosure processor will accept an
IBPI pattern but it will blink the LEDs not according to SFF-8489
specification or it has limited number of patterns supported.

rpmlint output:
ledctl.spec: W: invalid-url Source0: whoknows.com/ledctl-0.1.tgz
ledctl.x86_64: W: no-url-tag

- there is no upstream url for the sources, will try to make author to place it somewhere..

- can't test the package functionality because I don't have the required HW

Comment 1 Jason Tibbitts 2011-01-08 23:01:20 UTC
I see no SCM request to process here.

Comment 2 Ondrej Vasik 2011-01-10 14:29:58 UTC
formal review is here, see the notes explaining OK* and BAD statuses below:

N/A      source files match upstream:
 can't be checked, as there is no public upstream url for sources download
 it's quite common in the case of intel packages in early life.

Just for reference, sha1sums of checked components:
$ sha256sum ledctl.spec 
01ef25293aaf5dd83f499a86f8c8d83ae6816eaf5f7a0f6884d15f14b2669dcd  ledctl.spec
$ sha256sum ledctl-0.1.tgz 
209a5430ffb166de65e9949f196e05be8da4937150997f1e509e27bd9ab601ec  ledctl-0.1.tgz
$ sha256sum ledctl-0.1-1.src.rpm 
ee2d3db2c4e781a3423fc0e8e66206e837ee29e8a3484b234d5b51e51df9af53  ledctl-0.1-1.src.rpm

OK      package meets naming and versioning guidelines.
OK      specfile is properly named, is cleanly written and uses macros
consistently.
OK      dist tag is present.
OK      license field matches the actual license.
        GPLv2+
OK     license is open source-compatible. License text included in package.
        GPLv2+
OK      latest version is being packaged.
OK     BuildRequires are proper.

        pod2man required for manpage generating, so BuildRequires: perl could be added for safety, however - perl is part of dependency tree of the basic buildtree so not required.

MAYBE   compiler flags are appropriate.

        make CFLAGS="$RPM_OPT_FLAGS --std=c99" - original Makefile uses hardcoded CFLAGS=-O0 -g -Wall -std=c99 - maybe you should remove this definition from Makefile if you want to use $RPM_OPT_FLAGS.

OK      package builds in mock (Rawhide/i686).
OK      debuginfo package looks complete.
BAD     rpmlint is silent.

$ rpmlint ledctl.spec ledctl*.rpm
ledctl.spec: W: invalid-url Source0: whoknows.com/ledctl-0.1.tgz
ledctl-debuginfo.i686: W: no-url-tag
ledctl.i686: W: spelling-error %description -l en_US LEDs -> LED, LED's, LED s
ledctl.i686: W: no-url-tag
ledctl.src: W: spelling-error %description -l en_US LEDs -> LED, LED's, LED s
ledctl.src: W: no-url-tag
ledctl.src: W: invalid-url Source0: whoknows.com/ledctl-0.1.tgz
3 packages and 1 specfiles checked; 0 errors, 7 warnings.

Please, do not use whoknows.com in Source0 - it will produce errors in the mass checks. I would prefer comment in spec (explaining why there is no upstream URL and tarball download place)

OK     final provides and requires look sane.
N/A     %check is present and all tests pass.
N/A      shared libraries are added to the regular linker search paths with
proper scriptlets
OK      owns the directories it creates.
OK      doesn't own any directories it shouldn't.
OK      no duplicates in %files.
OK      file permissions are appropriate.
OK      correct scriptlets present.
OK      code, not content.
OK      documentation is small, so no -docs subpackage is necessary.
OK      %docs are not necessary for the proper functioning of the package.
OK      headers in -devel
OK      pkgconfig files in -devel
OK      no libtool .la droppings.
OK      not a GUI app.

Summary:
Change the Source0 - no whoknows.com please. Add explanation to the spec file. I can confirm there is no Intel Enclosure LED utilities product at http://downloadcenter.intel.com/default.aspx at the moment, however URL could at least point to Intel pages.
Consider this CFLAGS change.

Please resubmit the spec file and srpm and I'll check the "bad things" before I'll approve this package review.

Comment 3 Jiri Moskovcak 2011-01-10 15:26:15 UTC
new spec and packages uploaded. 

Fixed:
MAYBE   compiler flags are appropriate
Change the Source0 - no whoknows.com please.
URL could at least point to Intel pages

rpmlint:

ledctl.spec: W: invalid-url Source0: ledctl.tgz
1 packages and 1 specfiles checked; 0 errors, 1 warnings.

Comment 4 Ondrej Vasik 2011-01-10 16:42:57 UTC
Thanks,
sha256sums of checked files:
08afcf3448cc9eb9b19a474367b81749731ca424aa28aa0178d1879c01eaec60  ledctl.spec
209a5430ffb166de65e9949f196e05be8da4937150997f1e509e27bd9ab601ec  ledctl.tgz
99465542c9706fb4e2e6438e0ec40cb92e0a7666255048d22c7c76af01ca7c01  ledctl-0.1-1.src.rpm

Looks sane for me now. APPROVED.

Comment 5 Jiri Moskovcak 2011-01-10 18:05:25 UTC
New Package SCM Request
=======================
Package Name: ledctl
Short Description: Enclosure LED Utilities
Owners: jmoskovc
Branches: f14 el6

Comment 6 Jason Tibbitts 2011-01-10 19:21:53 UTC
Git done (by process-git-requests).

Comment 7 Jiri Moskovcak 2011-01-11 12:51:35 UTC
(In reply to comment #6)
> Git done (by process-git-requests).

Thank you!

Comment 8 Jiri Moskovcak 2011-02-16 09:46:40 UTC
This package has been obsoleted by ledmon -> closing.


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