Bug 889505
| Summary: | Review Request: libkqueue - Userspace implementation of the kqueue event notification mechanism | ||
|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | Eric Radman <eradman> |
| Component: | Package Review | Assignee: | Nobody's working on this, feel free to take it <nobody> |
| Status: | CLOSED WONTFIX | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
| Severity: | medium | Docs Contact: | |
| Priority: | unspecified | ||
| Version: | rawhide | CC: | adimania, eradman, fedora-package-review, jch, leamas.alec, mail, metherid, misc, notting, package-review, pwouters |
| Target Milestone: | --- | ||
| Target Release: | --- | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Whiteboard: | |||
| Fixed In Version: | Doc Type: | Bug Fix | |
| Doc Text: | Story Points: | --- | |
| Clone Of: | 684446 | Environment: | |
| Last Closed: | 2013-10-21 13:54:34 UTC | Type: | --- |
| Regression: | --- | Mount Type: | --- |
| Documentation: | --- | CRM: | |
| Verified Versions: | Category: | --- | |
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
| Cloudforms Team: | --- | Target Upstream Version: | |
| Embargoed: | |||
|
Description
Eric Radman
2012-12-21 14:10:46 UTC
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 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 Yup, but you need to give the login you used :) I'll get the hang of this! FAS login is 'eradman' (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}. Further above I've mentioned a few issues with the package. Please respond to that. 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. * https://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags * https://fedoraproject.org/wiki/Packaging:Guidelines#Devel_Packages * https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package * https://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries * https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag * https://fedoraproject.org/wiki/Packaging:Guidelines#.25clean "Happy New Year!" since this is probably my last bugzilla comment in 2012. ;-) 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
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 > 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 ? 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 #12 from Michael Schwendt <mschwendt> --- > 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. 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? 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 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 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.
(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. 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 Ping. *** Bug 684446 has been marked as a duplicate of this bug. *** |