[Bug 668197] Review Request: ledctl - LED control app for Intel(R) storage controllers

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=668197

--- Comment #2 from Ondrej Vasik <ovasik at redhat.com> 2011-01-10 09:29:58 EST ---
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.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.