This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 670302 - Review Request: libbacklight - Linux backlight abstraction library
Review Request: libbacklight - Linux backlight abstraction library
Status: CLOSED DEFERRED
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Ralf Corsepius
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FE-DEADREVIEW
  Show dependency treegraph
 
Reported: 2011-01-17 15:14 EST by Matthew Garrett
Modified: 2013-01-10 02:52 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2011-05-10 04:06:17 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)

  None (edit)
Description Matthew Garrett 2011-01-17 15:14:09 EST
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-17 23:27:30 EST
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 09:54:29 EST
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-18 22:03:48 EST
(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-18 23:04:11 EST
> 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 00:11:11 EST
(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 01:14:07 EST
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 04:57:51 EDT
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 09:13:57 EDT
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 04:06:17 EDT
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.