Bug 670302

Comments:

a) To mjg59 in his role as upstream:

- libbacklight.h lacks extern "C" guards.
=> API is not C++ safe.

- libbacklight.c suffers from implicit decls:

libbacklight.c: In function 'backlight_get':
libbacklight.c:74:2: warning: implicit declaration of function 'read'
libbacklight.c:80:2: warning: implicit declaration of function 'strtol'
libbacklight.c:83:2: warning: implicit declaration of function 'close'
libbacklight.c: In function 'backlight_set_brightness':
libbacklight.c:125:2: warning: implicit declaration of function 'write'
...
=> Code is like to suffer from portability (type-size) issues.
You'll likely want to "#include <unistd.h>"

- Consider to a licensing headers to all source files and a LICENCE/COPYING file.

- Consider to provide at least a minimum amount of documentation.
ATM, there is no trace of documentation, not even comments in libbacklight.h describing what these functions are supposed to do.


b) to mjg59 in his role as packager:

- configure.ac applies AM_SILENT_RULES.
=> AM_SILENT_RULES causes build.log to skip essential information (e.g. compiler calls). I'd recommend upstream to remove AM_SILENT_RULES from configure.ac. Alternatively, add --disable-silent-rules to %configure

- package applies pkg-config
=> Missing BR: pkg-config
(The fact pkg-config is implicitly being pulled in, is a random coincidence)

- Unnecessary "BR: libtool".
Please remove it, It's not needed.

Updates:

C++ extern guards added
unistd.h added (not sure why I'm not getting those warnings)
Documentation added to README and libbacklight.h
Argument passed to configure
BRs fixed

Thanks for the review! New versions of the files are at the same URL.

(In reply to comment #2)
> unistd.h added (not sure why I'm not getting those warnings)
I was building the package in f14 and f15-x86_64 mock-chroots on f14-x86_64
=> package received RPM_OPTS_FLAGS.

Likely you are building in an ordinary user's environment (The files inside of your srpm being owned by user mjg59 indicates this) without explict explicitly setting CFLAGS etc.

[NB: This is one of the situations AM_SILENT_RULES exposes its harmfulness: 
You don't know which cflags you are using, which directories are being passed to gcc etc.]


Further comments/findings:

1.  Still one moderately serious warning:
libbacklight.c:81:2: warning: implicit declaration of function 'strtol'

2. libbacklight.h lacks a "header inclusion guard",
 i.e. something similar
to this:
#ifndef LIBBACKLIGHT_H
#define LIBBACKLIGHT_H
<actual contents>
#endif

3. Still no legal information in libbacklight.h (no copyright/license).

4. libbacklight.h includes <pciaccess.h>
I don't see libbacklight.h uses any external symbol which could originate from <pciaccess.h> => makes me think this could be a bug.

However, should this #include be intentional, then libbacklight-devel would need "Requires: libpciaccess-devel" + this dependency be reflected to libbacklight.pc


Finally, a general remark: Please increment a package's rpm-revision each time you replace it in reviews - It tremendously helps reviewers. TIA.

> libbacklight.c:81:2: warning: implicit declaration of function 'strtol'

Fixed

> libbacklight.h lacks a "header inclusion guard"

Added

> Still no legal information in libbacklight.h (no copyright/license).

I don't think there's anything substantively copyrightable in the file, but the project copyright file included in %doc.

> libbacklight.h includes <pciaccess.h>

It's needed for struct pci_device, but the missing Requires was a bug.

Spec URL: http://www.codon.org.uk/~mjg59/tmp/libbacklight/libbacklight.spec
SRPM URL:
http://www.codon.org.uk/~mjg59/tmp/libbacklight/libbacklight-0.01-2.fc15.src.rpm

Thanks for the feedback!

(In reply to comment #4)

> > Still no legal information in libbacklight.h (no copyright/license).
> 
> I don't think there's anything substantively copyrightable in the file, but the
> project copyright file included in %doc.

Well, a file without explicit copyright is implicitly legally owned by its author. As only the author is legitimated to grant a license on a file, such files are legally unsafe to use by users. Whether a detached license file is sufficient for granting a license, is legally controversial. 
I.e. your works' users are only safe from being sued by you (rsp. this code's legal owner), when a piece of code contains copyright/license terms inside of its sources.

It's the reasoning why e.g. the FSF insists on explict copyright/license terms being included in each of their source files.

> > libbacklight.h includes <pciaccess.h>
> 
> It's needed for struct pci_device, but the missing Requires was a bug.
OK, then you'd have to reflect this dependency to libbacklight.pc, also (consider the case of libpciaccess having been installed to a non-standard directory).

I am not 100% sure what to do. As you only #include <pciaccess.h> and don't link against libpciaccess, "Requires: pciaccess" would go too far, because it pulls in -lpciaccess.

May-be "Requires.private: pciaccess" would be better - It would pull in -lpciaccess only for pkg-config --static), but wouldn't do so for dynamic linkage.

Ok, fixed both of those.

Spec URL: http://www.codon.org.uk/~mjg59/tmp/libbacklight/libbacklight.spec
SRPM URL:
http://www.codon.org.uk/~mjg59/tmp/libbacklight/libbacklight-0.01-3.fc15.src.rpm

I am sure future upstream tarball updates will carry an incremented version and the current tarball furtheron not be replaced ;)

md5sum: a04ae7354b0b0176326914e772dbdf11  libbacklight-0.01.tar.gz

# rpmlint /var/lib/mock/fedora-rawhide-x86_64/result/*rpm
libbacklight-devel.x86_64: W: no-documentation
4 packages and 0 specfiles checked; 0 errors, 1 warnings.

APPROVED

Matthew? What's going on with this review?

This package was approved 3 months ago, but seemingly nothing has happened since.
Shouldn't you reply within one week, I'll revoke my approval and close this review request as "failed".

Sorry, I got distracted with pushing the kernel side of this upstream and forgot about the review. I'll handle it this week.

Another 2 weeks have past. Still not CVS-request.

REVOKING approval, Closing STALLED SUBMITTER.

Feel free to resubmit your package, when you have more time.