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=670302 Ralf Corsepius <rc040203@xxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |rc040203@xxxxxxxxxx --- Comment #1 from Ralf Corsepius <rc040203@xxxxxxxxxx> 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. -- 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. _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review