Bug 889505 - Review Request: libkqueue - Userspace implementation of the kqueue event notification mechanism
Review Request: libkqueue - Userspace implementation of the kqueue event noti...
Status: CLOSED WONTFIX
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
: 684446 (view as bug list)
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-12-21 09:10 EST by Eric Radman
Modified: 2014-01-16 04:21 EST (History)
11 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: 684446
Environment:
Last Closed: 2013-10-21 09:54:34 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Eric Radman 2012-12-21 09:10:46 EST
+++ This bug was initially created as a clone of Bug #684446 +++

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

--- Additional comment from Rahul Sundaram on 2011-03-12 14:04:50 EST ---


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

--- Additional comment from Rahul Sundaram on 2011-03-14 03:36:02 EDT ---


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

--- Additional comment from Aditya Patawari on 2011-04-01 11:40:10 EDT ---

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

--- Additional comment from Rahul Sundaram on 2011-04-02 15:10:35 EDT ---


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.

--- Additional comment from Michael Schwendt on 2011-05-03 04:08:42 EDT ---

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

--- Additional comment from John Haxby on 2012-02-04 15:46:36 EST ---

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.

--- Additional comment from John Haxby on 2012-02-04 15:50:23 EST ---

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.

--- Additional comment from John Haxby on 2012-02-04 15:51:21 EST ---

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?

--- Additional comment from Mario Blättermann on 2012-09-17 15:37:35 EDT ---

@Aditya, you didn't response here for more than a year. Are you still interested in to keep this review request alive?

--- Additional comment from Paul Wouters on 2012-09-28 19:00:26 EDT ---

I'm interested to get this package in, as I need it as a dependancy :)

--- Additional comment from Mario Blättermann on 2012-10-08 14:05:29 EDT ---

(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 1 Michael Scherer 2012-12-23 04:52:41 EST
This package lack the link to a the url of the spec and srpm, so I am arking it as NotReady for now.

It also lack the submitter FAS login
Comment 2 Eric Radman 2012-12-27 14:39:41 EST
I published the spec ans srpm here:

http://static.eradman.com/src/libkqueue-1.0.1-1.src.rpm
http://static.eradman.com/src/libkqueue.spec

I also registered with the Fedora Project
Comment 3 Michael Scherer 2012-12-27 15:27:26 EST
Yup, but you need to give the login you used :)
Comment 4 Eric Radman 2012-12-27 17:02:03 EST
I'll get the hang of this! FAS login is 'eradman'
Comment 5 Eric Radman 2012-12-28 08:03:39 EST
(In reply to comment #2)

I updated these source files slightly with a fix to libkqueue-devel so that pkg-config returns paths based on %{_prefix}.
Comment 6 Michael Schwendt 2012-12-28 17:10:28 EST
Further above I've mentioned a few issues with the package. Please respond to that.
Comment 7 Eric Radman 2012-12-29 22:24:53 EST
Okay, further corrections applied:

- Updated the .spec to create separate -spec and -devel packages
- Install manpages using .* instead of .gz
- Run ldconfig directly from %post and %postrun

rpmlint raises a number of warnings that I'll work through.

Also, rpmbuild is creating an empty -debuginfo package. I don't yet know why this is happening.
Comment 9 Eric Radman 2013-01-01 00:45:20 EST
Happy 2013! I think this package is starting to shape up

- devel and static subpackages depend on the base package
- Fix up the description a bit
- Install .so.* files with executable permissions
- Use %{_configure} for tests as well as the main program

Note the rpmlint complains that the subpackages don't include documentation, but they include the base package which contains the man pages.

Comments are welcome
Comment 10 Michael Schwendt 2013-01-01 05:59:48 EST
A bit of feedback on the reviewer comments would be very welcome, too, and could result in faster progress and even better guiding/hints. For example, one reviewer has told you not to package the .a and .la file, another pointed out that the package would violate the static library guidelines, but you've not commented on that yet.

Many things in the packaging guidelines are not in there just for fun, but with the goal of avoiding packaging pitfalls.


> %package static

_Why_ is the static lib being built?

[Regardless of that question, a -static subpackage would not need to depend on the base package (a run-time shared library package), but the -devel package.]


> rpmbuild is creating an empty -debuginfo package. I don't yet know why this
> is happening.

Perhaps the "redhat-rpm-config" package is not installed. One can get it also as a dependency when installing the "fedora-packager" package. Or the test-build is not done with Mock (or the Fedora Build System "koji" if you follow the https://fedoraproject.org/wiki/Join_the_package_collection_maintainers process steps), or the built code applies its own insufficient compiler flags, or it strips the built code.


> License:    MIT and BSD

https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text

That's one of the early items on the following list:
https://fedoraproject.org/wiki/Packaging:ReviewGuidelines


> %{_configure} --prefix=%{_prefix}

The %{_configure} macro expands to just "./configure", so using the macro doesn't add any value. Better would be to add a comment to the spec file that explains _why_ "./configure" is used. Is it a custom-made configure script and probably incompatible with the more commonly used %{configure} macro? Check out:

  $ rpm --eval %_configure
  $ rpm --eval %configure


> make install DESTDIR=$RPM_BUILD_ROOT LIBDIR=$RPM_BUILD_ROOT%{_libdir}
> MANDIR=$RPM_BUILD_ROOT%{_mandir} INCLUDEDIR=$RPM_BUILD_ROOT%{_includedir}

Does it use DESTDIR at all? As a prefix for the "install" target? If so, that raises the question why to override the other variables, too? Btw, there's the %{make_install} macro these days, which saves some typing in the spec file.


> the base package which contains the man pages.

That's the wrong package for them. They are development related. Section 2 (system calls).


> %{_includedir}/kqueue/sys/event.h

The "unowned" directory issue is still present.


> %{_libdir}/libkqueue.so

This file is included in the wrong package.


> %{_libdir}/libkqueue.la

The previously linked packaging guidelines say something about these files.


> %{_libdir}/pkgconfig/libkqueue.pc

%{_libdir} is not substituted correctly in this file. It's hardcoded to /usr/lib here for x86_64
Comment 11 Eric Radman 2013-01-12 09:08:12 EST
> one reviewer has told you not to package the .a and .la file, another
> pointed out that the package would violate the static library
> guidelines, but you've not commented on that yet.

Fair point, I should have asking why he said this. Better to ask than to
assume. And to your larger point, it wouldn't have hurt to start with a
short list of questions about previous comments.

I understand that the Fedora project does not want applications to be
built with static libraries, but it didn't occur to me that users
are not permitted to build their own utilities using cc -static.

> Many things in the packaging guidelines are not in there just for fun,
> but with the goal of avoiding packaging pitfalls.

My tendency was to look at existing RPMs, but they're not always good
templates to follow.

> _Why_ is the static lib being built?

Because it's useful. In some cases it's difficult to run utilities that
are built on DSOs.

The author of libkqueue agreed that we don't need to package the .a
file, so I removed the static package.

>> License:    MIT and BSD

> https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text
>
> That's one of the early items on the following list:
> https://fedoraproject.org/wiki/Packaging:ReviewGuidelines

I added a COPYING file to the source and included it with %doc

> > %{_configure} --prefix=%{_prefix}
>
> The %{_configure} macro expands to just "./configure", so using the
> macro doesn't add any value. Better would be to add a comment to the
> spec file that explains _why_ "./configure" is used.

%{configure} breaks the build by setting --target 
to 'x86_64-redhat-linux-gnu' instead of 'linux', which changes the
behavior of `uname`.  

The configure script needs to know the operating system type in order to
build he right files (e.g src/solaris, src/linux)

>> make install DESTDIR=$RPM_BUILD_ROOT LIBDIR=$RPM_BUILD_ROOT%{_libdir}
>> MANDIR=$RPM_BUILD_ROOT%{_mandir} INCLUDEDIR=$RPM_BUILD_ROOT%{_includedir}
>
>Does it use DESTDIR at all? As a prefix for the "install" target? If so, that
>raises the question why to override the other variables, too? Btw, there's the
>%{make_install} macro these days, which saves some typing in the spec file.

I think DESTDIR is used indirectly because the other variables (MANDIR, 
INCLUDEDIR) are based on it. You're right, we don't have to override 
these other variables. The only exception is LIBDIR which is set to
$DESTDIR/usr/lib instead of $DESTDIR/usr/lib64. I'm not sure how to 
improve this.

> > the base package which contains the man pages.
>
> That's the wrong package for them. They are development related.
> Section 2 (system calls).

ok, I moved the man page the devel package

> > %{_includedir}/kqueue/sys/event.h
>
> The "unowned" directory issue is still present.

Is the right way to do it?

%dir %{_includedir}/kqueue
%dir %{_includedir}/kqueue/sys

> > %{_libdir}/libkqueue.so
>
> This file is included in the wrong package.

Ok, moved libkqueue.so to then devel package

> > %{_libdir}/pkgconfig/libkqueue.pc
>
> %{_libdir} is not substituted correctly in this file. It's hardcoded
> to /usr/lib %here for x86_64

Are you saying that we should hard-code the path to /usr/lib/pkgconfig ?
Comment 12 Michael Schwendt 2013-01-13 16:15:16 EST
Note that I haven't reviewed the latest update except that it still doesn't use the global CFLAGS, but overrides them with own ones.

Just some answers:

> I understand that the Fedora project does not want applications to be
> built with static libraries, but it didn't occur to me that users
> are not permitted to build their own utilities using cc -static.

The same users, who would simply install an own special build of libkqueue into /usr/local then? ;)

I don't intend to argue a lot about this. It's just that the packaging guidelines say

| In general, packagers are strongly encouraged not to ship static libs
| unless a compelling reason exists.

and actually most libraries are not shipped as static builds, because neither the packager nor the developer would find a _compelling_ reason why to insist on shipping the static build.

Libtool archives (.la files) are a different matter, and shipping them in the -static subpackage would have been wrong, btw, since they are not specific to static linking.


> > %{_configure} --prefix=%{_prefix}

> %{configure} breaks the build by setting --target 

Yes, that's why I suggested mentioning the rationale in the spec file. Which is what you've done in the latest update. Good.

The real question was why use %{_configure} and not just ./configure? The macro doesn't add any value, does it? You're free to keep using it, I'm just curious. Not that you think you must use it or such.


> The only exception is LIBDIR which is set to $DESTDIR/usr/lib instead
> of $DESTDIR/usr/lib64. I'm not sure how to improve this.

What about passing --libdir=%{_libdir} to the configure script? The script seems to handle that. As pointed out before, the wrong /usr/lib gets substituted into the built files. libkqueue.pc for example.


> Is the right way to do it?
>
> %dir %{_includedir}/kqueue
> %dir %{_includedir}/kqueue/sys

Yes, for this package, yes.  For other packages and where you want to include entire directory trees, there are other solutions mentioned in the guidelines.
Comment 13 Eric Radman 2013-01-21 04:01:22 EST
> --- Comment #12 from Michael Schwendt <mschwendt@gmail.com> ---
> Note that I haven't reviewed the latest update except that it still doesn't
> use the global CFLAGS, but overrides them with own ones.

Indeed this is always bad behavior, the following change to config.inc
corrects this

cflags="$cflags -fpic"
ldflags="$ldflags"

> > I understand that the Fedora project does not want applications to be
> > built with static libraries, but it didn't occur to me that users
> > are not permitted to build their own utilities using cc -static.
> 
> The same users, who would simply install an own special build of libkqueue
> into /usr/local then? ;)

True! Making this point up front would avoid a lot of confusion. None of
the arguments I've read so far put it so simply.

> why use %{_configure} and not just ./configure? The macro doesn't add any
> value, does it? You're free to keep using it, I'm just curious.
> Not that you think you must use it or such.

Only because someone bothered to define the macro, I assume for some
good reason. Other than that it's a mystery to me.

> > The only exception is LIBDIR which is set to $DESTDIR/usr/lib instead
> > of $DESTDIR/usr/lib64. I'm not sure how to improve this.
> 
> What about passing --libdir=%{_libdir} to the configure script? The script
> seems to handle that. As pointed out before, the wrong /usr/lib gets
> substituted into the built files. libkqueue.pc for example.

That worked perfectly, thanks.
Comment 14 Eric Radman 2013-04-01 11:03:42 EDT
I've tested the latest revisions on Fedora 17 and Red Hat 6.4

http://static.eradman.com/src/libkqueue-1.0.1-1.src.rpm
http://static.eradman.com/src/libkqueue.spec
http://static.eradman.com/src/libkqueue-1.0.1.tar.gz

Any further comments?
Comment 15 Eric Radman 2013-04-03 17:19:17 EDT
It was suggested that I bump the package version and add a changelog entry for each public revision. 

http://static.eradman.com/src/libkqueue-1.0.1-5.src.rpm
Comment 16 Alec Leamas 2013-04-04 07:08:58 EDT
In comment #13 you set cflags to "$cflags -fpic". How will this ensure that %{optflags} is applied?

Stated otherwise: if you check the build logs and compares it with the output from 'rpm --eval %optflags', can you verify that %optflags is applied? See also [1]

Patching or adding a COPYING file isn't kosher, only the copyright owner can do that.  You should file a bug upstream about the missing COPYING, leaving source as-is until upstream fixes this. See also [2].

The normal way to handle the other stuff is to use the upstream sources and create patches as needed. However, instead of using the upstream source from  http://mark.heily.com/project/libkqueue you have created an own archive, effectively forking the project. Forking isn't necessary evil, but requires an explanation beyond the review remarks discussed so far. So: why have you forked the upstream project? 

[1] https://fedoraproject.org/wiki/Common_Rpmlint_issues#debuginfo-without-sources
[2] https://fedoraproject.org/wiki/Common_Rpmlint_issues#incorrect-fsf-address
Comment 17 Eric Radman 2013-04-04 09:55:29 EDT
I finally get it, these flags are added at configure time, but I wasn't using the standard %configure macro.

CFLAGS="%{optflags} -fpic" %{_configure}

Also shortened the summary a bit. rpmlint is finnally free of warnings on Fedora 17. New source:

http://static.eradman.com/src/libkqueue-1.0.1-6.src.rpm

> Patching or adding a COPYING file isn't kosher

I'll ask the author if he wants to create this file in the official SVN repo. His license isn't ambiguous, COPYING contains the exact text already included in the project.

> why have you forked the upstream project?

Primarily to maintain quick turnaround and minimize friction since Mark's current work is focused on a 2.0 release. I'll write him and ask if he'd like to create an official 1.0 maintenance branch with the changes I've made.
Comment 18 Alec Leamas 2013-04-04 10:16:44 EDT
(In reply to comment #17)
[cut]

> 
> > why have you forked the upstream project?
> 
> Primarily to maintain quick turnaround and minimize friction since Mark's
> current work is focused on a 2.0 release. I'll write him and ask if he'd
> like to create an official 1.0 maintenance branch with the changes I've made.

Basically, this motivation isn't enough IMHO. I suggest that you go back to using the upstream sources. If you still need to fix config.inc you can use sed -i to patch it in place. If there are other changes, add a patch and/or new sources. The current situation will make maintenance hard and I would not let it pass if I was reviewing it.

Of course, you should try to talk to upstream about merging the patches. However, the first step is to create those patches, not to create a new tarball.

BTW, when building for reviews rawhide is the preferred platform. Use mock, and this is no problem. F17 is to old today.
Comment 19 Eric Radman 2013-05-09 00:18:33 EDT
Instead of patching libkqueue-1.x, Mark Heily allowed me to integrate the RPM build changes into the 2.0 release, which I've done

http://downloads.sourceforge.net/libkqueue/libkqueue-2.0.1.tar.gz
http://sourceforge.net/p/libkqueue/code/HEAD/tree/tags/REL_2_0_1/libkqueue.spec?format=raw
http://static.eradman.com/src/libkqueue-2.0.1-1.src.rpm

rpmlint shows one warning for the .spec file, which I'm not sure how to solve (I'm now testing on rawhide instead of Fedora 17)

libkqueue.src:10: W: unversioned-explicit-provides libkqueue.so.0
Comment 20 Eric Radman 2013-06-04 16:49:19 EDT
Ping.
Comment 21 Parag AN(पराग) 2013-07-22 03:19:03 EDT
*** Bug 684446 has been marked as a duplicate of this bug. ***

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