Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 670302 - Review Request: libbacklight - Linux backlight abstraction library
Summary: Review Request: libbacklight - Linux backlight abstraction library
Keywords:
Status: CLOSED DEFERRED
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Ralf Corsepius
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: FE-DEADREVIEW
TreeView+ depends on / blocked
 
Reported: 2011-01-17 20:14 UTC by Matthew Garrett
Modified: 2013-01-10 07:52 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-05-10 08:06:17 UTC


Attachments (Terms of Use)

Description Matthew Garrett 2011-01-17 20:14:09 UTC
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-1.fc15.src.rpm
Description: libbacklight provides an abstraction for Linux backlight control, making it easier for applications to identify the preferred backlight device for a
given piece of graphics hardware.

Comment 1 Ralf Corsepius 2011-01-18 04:27:30 UTC
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.

Comment 2 Matthew Garrett 2011-01-18 14:54:29 UTC
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.

Comment 3 Ralf Corsepius 2011-01-19 03:03:48 UTC
(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.

Comment 4 Matthew Garrett 2011-01-19 04:04:11 UTC
> 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!

Comment 5 Ralf Corsepius 2011-01-19 05:11:11 UTC
(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.

Comment 7 Ralf Corsepius 2011-01-20 06:14:07 UTC
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

Comment 8 Ralf Corsepius 2011-04-25 08:57:51 UTC
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".

Comment 9 Matthew Garrett 2011-04-25 13:13:57 UTC
Sorry, I got distracted with pushing the kernel side of this upstream and forgot about the review. I'll handle it this week.

Comment 10 Ralf Corsepius 2011-05-10 08:06:17 UTC
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.


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