Bug 684446 - Review Request: libkqueue - A userspace implementation of the kqueue kernel event notification mechanism
Summary: Review Request: libkqueue - A userspace implementation of the kqueue kernel e...
Keywords:
Status: CLOSED DUPLICATE of bug 889505
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Paul Wouters
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-03-12 17:09 UTC by Aditya Patawari
Modified: 2013-07-22 07:19 UTC (History)
8 users (show)

Fixed In Version:
Clone Of:
: 889505 (view as bug list)
Environment:
Last Closed: 2013-07-22 07:19:03 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)
Updated .spec file (1.41 KB, text/plain)
2012-02-04 20:46 UTC, John Haxby
no flags Details
Compilation errors patch (964 bytes, patch)
2012-02-04 20:50 UTC, John Haxby
no flags Details | Diff

Description Aditya Patawari 2011-03-12 17:09:08 UTC
SPEC URL: http://adimania.fedorapeople.org/libkqueue.spec

SRPM URL: http://adimania.fedorapeople.org/libkqueue-1.0.1-1.src.rpm

Description: libkqueue is a userspace implementation of the kqueue(2) kernel event notification mechanism. Initial efforts are focused on porting to the Linux 2.6 kernel. libkqueue acts as a translator between the kevent structure and the native kernel facilities of the host machine.

Koji Scratch Build URL: http://koji.fedoraproject.org/koji/taskinfo?taskID=2906645

Comment 1 Rahul Sundaram 2011-03-12 19:04:50 UTC
Not a full review yet but here is some initial feedback:

" Initial efforts are focused on porting to the 
Linux 2.6 kernel"

* Drop this from the description.  
* You don't need to define the buildroot or have a %clean section

http://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag
http://fedoraproject.org/wiki/Packaging:Guidelines#.25clean

* Does %setup really require passing name and version to it? 
* You need to use %configure macro and not use ./configure
* Must use %defattr(-,root,root,-)
* Should not package .la file  

http://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries

* Header files and pkgconfig files belong in a -devel package
* You should use a %check and run the in-built test suite

Comment 2 Rahul Sundaram 2011-03-14 07:36:02 UTC
src/common/tree.h and include/sys/event.h are under the BSD license (2 clause) and hence the license tag must be MIT and BSD

Comment 3 Aditya Patawari 2011-04-01 15:40:10 UTC
The bugs have been fixed and devel package has been created.
The package has been updated to latest version.

Spec: http://adimania.fedorapeople.org/specs/libkqueue.spec
SRPM: http://adimania.fedorapeople.org/src.rpms/libkqueue-1.0.2-1.src.rpm

Koji Build URL: http://koji.fedoraproject.org/koji/taskinfo?taskID=2966742

Comment 4 Rahul Sundaram 2011-04-02 19:10:35 UTC
Run rpmlint on the spec file,  srpm and binary rpms and try to fix any warnings and errors.  Files which should be in -devel subpackage are not.  You shouldn't package libtool archive (.la) and static library (.a).  Fix the summary as well.

Comment 5 Michael Schwendt 2011-05-03 08:08:42 UTC
The package would violate the static library packaging guidelines anyway. If shared and static lib are present, the static lib would need to be moved into a libkqueue-static subpackage:
https://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries


> %package devel

https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package
https://fedoraproject.org/wiki/Packaging:Guidelines#Devel_Packages


> %{_mandir}/man2/kqueue.2.gz
> %{_mandir}/man2/kevent.2.gz

Manual pages are compressed on-the-fly by rpmbuild with a compression that may change, so including with wildcard .* instead of .gz is the way to go.


> %files devel
> %defattr(-,root,root,-)
> %{_includedir}/kqueue/sys/event.h

https://fedoraproject.org/wiki/Packaging:UnownedDirectories


> %post
> /sbin/ldconfig
> 
> %postun
> /sbin/ldconfig

The better form here is to not run ldconfig from within a /bin/sh script, but directly:

%post -p /sbin/ldconfig

%postun -p /sbin/ldconfig

Comment 6 John Haxby 2012-02-04 20:46:36 UTC
Created attachment 559432 [details]
Updated .spec file

This updated .spec file addresses most of the comments above.   There is one remaining issue: the debuginfo rpm contains no files: I think this is because libkqueue.so is actually a symbolic link to libkqueue.so.0.0.

The .spec file includes new links for the home page and source and this .spec file is for 1.0.4.   The next attachment is a patch file to address two compiler warnings that were treated as errors.

Comment 7 John Haxby 2012-02-04 20:50:23 UTC
Created attachment 559433 [details]
Compilation errors patch

This patch addresses two compilation warnings.

One is for an ignored read(2) result that is simply passed up to the caller: all the callers I could find do check the result.

The other warning is for an ignored write(2) in a signal handler.  There is nothing much you can do with this (as it's in a signal handler) so I just wrapped it in an assert; it should never fail anyway.

Comment 8 John Haxby 2012-02-04 20:51:21 UTC
I don't suppose this is going to make it into Fedora 17 is it?  Along with the other libraries that make up Apple's Grand Central Dispatch?

Comment 9 Mario Blättermann 2012-09-17 19:37:35 UTC
@Aditya, you didn't response here for more than a year. Are you still interested in to keep this review request alive?

Comment 10 Paul Wouters 2012-09-28 23:00:26 UTC
I'm interested to get this package in, as I need it as a dependancy :)

Comment 11 Mario Blättermann 2012-10-08 18:05:29 UTC
(In reply to comment #10)
> I'm interested to get this package in, as I need it as a dependancy :)

Paul, if Aditya doesn't response within a reasonable time frame, feel free to open a new review request for libkqueue and mark the current one as a duplicate. I'll do the review then.

Comment 12 Parag AN(पराग) 2013-07-22 07:19:03 UTC
I will close this as I see we already have fresh review request for this package.

*** This bug has been marked as a duplicate of bug 889505 ***


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