Bug 523523 - Review Request: clutter-gesture - Gesture Library for Clutter
Summary: Review Request: clutter-gesture - Gesture Library for Clutter
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Christian Krause
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: MeeGo1
TreeView+ depends on / blocked
 
Reported: 2009-09-15 19:34 UTC by Peter Robinson
Modified: 2009-12-29 03:22 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-12-29 03:22:33 UTC
Type: ---
Embargoed:
chkr: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Peter Robinson 2009-09-15 19:34:52 UTC
SPEC: http://pbrobinson.fedorapeople.org/clutter-gesture.spec
SRPM: http://pbrobinson.fedorapeople.org/clutter-gesture-0.0.2-1.fc11.src.rpm

Description:
This library allows clutter applications to be aware of gestures
and to easily attach some handlers to the gesture events. The library
supports both single and multi touch.

koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=1681277

Comment 1 Christian Krause 2009-11-08 21:46:11 UTC
Here is the full review of the package:

* rpmlint: ?
rpmlint SRPMS/clutter-gesture-0.0.2-1.fc12.src.rpm RPMS/i686/clutter-gesture-* SPECS/clutter-gesture.spec 
clutter-gesture-devel.i686: W: no-documentation
4 packages and 1 specfiles checked; 0 errors, 1 warnings.

Rpmlint looks clean - in general is a missing documentation acceptable for
devel packages. However, searching for clutter-gesture on moblin.org I've
found a document describing the clutter-gesture API:
http://moblin.org/sites/all/files/Clutter-Gesture-API-0.5.pdf
Depending on its "up-to-date status", would it be an option
to package this as the documentation for the devel package?

* naming: OK
- name matches upstream
- spec file name matches package name

* sources: OK
- md5sum: de0e3e5c01f0a328cc04a46198776ebe  clutter-gesture-0.0.2.tar.bz2
- sources matches upstream
- Source0 tag ok
- spectool -g  works

* URL tag: ?
clutter-gesture has its own project page:
http://moblin.org/projects/clutter-gesture
Would it make sense to use this one?

* License: TODO
- License LGPLv2+ acceptable
- License in spec file does not match the actual license:
In general it does, but one source file (engine/stroke.c) is actual under the MIT license:
https://fedoraproject.org/wiki/Licensing/MIT#Old_Style_with_legal_disclaimer_2
- So the license in the spec file should be "LGPLv2+ and MIT" (according to
https://fedoraproject.org/wiki/Packaging/LicensingGuidelines#Multiple_Licensing_Scenarios ). Please also add a comment about this issue to the spec file.
- License file packaged
- It would be morer clearer if upstream would provide a standard license file with a proper name like COPYING (and not "NEWS" ;-) ). According to the Review guidelines the packager is encouraged to query upstream to include it. However this will not block the review. ;-)

* spec file written in English and legible: OK

* compilation: TODO
- supports parallel build: OK
- RPM_OPT_FLAGS are correctly used: OK
- builds in koji for F13 and F12: OK
- uncommon configure flags: IMHO it shouldn't be necessary to add "--with-pic", since "--enable-dynamic" is the default anyway
- uncommon CFLAGS: auto*/libtool usually take care of compiling with the correct
parameters for shared libs
- For testing purposes I've removed "--with-pic" and the CFLAGS definition at all and according to the output during compilation the library is still compiled with the correct flags.

* BuildRequires: OK

* locales handling: OK (n/a)

* ldconfig in %post and %postun: OK

* package owns all directories that it creates: OK

* no files listed twice in %files: OK

* file permissions: OK
- %defattr used
- actual permissions in packages ok

* %clean section: OK

* macro usage: OK

* code vs. content: OK (only code)

* large documentation into subpackage: OK (n/a)

* header files in -devel subpackage: OK

* header files: TODO
- clutter-gesture.h includes "config.h" and there is no "config.h" in
/usr/include/clutter-gesture/
- this means, it is not possible to include the public API
- possible solutions:
a) header file should not include "config.h"
b) use an conditional #include directive like
#ifdef HAVE_CONFIG_H
#include "config.h"
#endif
or
c) have a "config.h" in the include directory (this should not be done with the config.h generated by configure)

* static libraries in -static package: OK (n/a)

* package containing *.pc files must "Requires: pkgconfig": OK

* *.so link in -devel package: OK

* devel package requires base package using fully versioned dependency: OK

* packages must not contain *.la files: OK

* GUI applications must provide *.desktop file: OK (n/a)

* packages must not own files/dirs already owned by other packages: OK

* rm -rf $RPM_BUILD_ROOT at the beginning of %install: OK

* all filenames UTF-8: OK

* functional test: ?
How can I test this package? The tests in tests/ just crash if I try
to execute them...

* debuginfo sub-package: OK
- non-empty


Summary
-------

nice to have:
- Package API documentation
- Let URL tag point to specific project page on moblin.org
- What would be a proper functional test?

blocking:
- Add MIT to license tag
- Fix public header files
- Remove unusual %configure flags / CFLAGS

Comment 2 Peter Robinson 2009-12-19 14:36:13 UTC
Fixed:
> - Let URL tag point to specific project page on moblin.org
> - Add MIT to license tag
> - Remove unusual %configure flags / CFLAGS  

Reported upstream. 
> - Package API documentation
> - Fix public header files

eom is the currently only supported test although there should be more support in Moblin 2.2
> - What would be a proper functional test?

Comment 3 Peter Robinson 2009-12-19 14:38:03 UTC
Updated SRPM: http://pbrobinson.fedorapeople.org/clutter-gesture-0.0.2-2.fc12.src.rpm

Spec location as before.

Comment 4 Christian Krause 2009-12-27 13:30:30 UTC
Thank you very much for the update.

(In reply to comment #2)
> Fixed:
> > - Let URL tag point to specific project page on moblin.org
> > - Add MIT to license tag
> > - Remove unusual %configure flags / CFLAGS  
> 
> Reported upstream. 
> > - Package API documentation
> > - Fix public header files

I've reviewed the new package and besides the public header / pkgconfig issue it looks fine now.

Have you already heard something from upstream?

However, since it is possible to compile eom against clutter-gesture (and the test programs work), the header issue does not block the review.

The remaining minor non-blocking issues are:

1. Package API documentation

2. Fix public header files / pkgconfig
Please note, that /usr/lib/pkgconfig/clutter-gesture.pc also contains a placeholder which was not replaced by the configure script:
 modlibexecdir=@modlibexecdir@
This must also be fixed.

-> APPROVED

Comment 5 Peter Robinson 2009-12-27 15:04:51 UTC
Thanks for the review

New Package CVS Request
=======================
Package Name: clutter-gesture
Short Description: Gesture Library for Clutter
Owners: pbrobinson
Branches: F-12
InitialCC:

Comment 6 Kevin Fenzi 2009-12-29 02:56:40 UTC
cvs done.

Comment 7 Peter Robinson 2009-12-29 03:22:33 UTC
Built and in rawhide


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