https://bugzilla.redhat.com/show_bug.cgi?id=1268010
Bug ID: 1268010 Summary: Review Request: libmooshika - helper library for RDMA Product: Fedora Version: rawhide Component: Package Review Severity: medium Assignee: nobody@fedoraproject.org Reporter: dominique.martinet@cea.fr QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org
Spec URL: http://asmadeus.notk.org/CEA/libmooshika-1.0-2.g374a0dd.spec SRPM URL: http://asmadeus.notk.org/CEA/libmooshika-1.0-2.g374a0dd.fc22.src.rpm Description: libmooshika is a helper for rdma connection handling, used primarily by nfs-ganesha for 9P. Fedora Account System Username: martinetd
This is my first package and it would appear I need a sponsor, I'll gladly fix things in the spec file (in particular I couldn't find anything about including the git commit in the package release - a few other packages do this but most do not, just tell me if it's a nuisance) This version passes koji builds for f21-24 and epel7, on all architectures built by default: http://koji.fedoraproject.org/koji/tasks?owner=martinetd&state=all
I am the primary upstream maintainer of the library, working on nfs-ganesha as well, a few people from the gluster community know me.
https://bugzilla.redhat.com/show_bug.cgi?id=1268010
Dominique Martinet dominique.martinet@cea.fr changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |177841 (FE-NEEDSPONSOR)
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=177841 [Bug 177841] Tracker: Review requests from new Fedora packagers who need a sponsor
https://bugzilla.redhat.com/show_bug.cgi?id=1268010
Dave Love d.love@liverpool.ac.uk changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |d.love@liverpool.ac.uk
--- Comment #1 from Dave Love d.love@liverpool.ac.uk --- Unfortunately I can't review this properly as I can't run ganesha so can't test it. However, here are some quick comments on the spec file.
* I don't know if you want to support it, but the Group tag is only needed for EPEL5;
* You normally shouldn't add explicit Requires for dynamic libraries except typically for devel packages depending on the non-devel version -- the packaging will usually do that automatically. For the devel package the Requires should normally be like
%package devel Requires: %{name}-libs%{?_isa} = %{version}-%{release} ...
* make should have an argument %{?_smp_mflags} in the %build section, or a comment that it's not smp-safe;
* Don't package .la files (and normally not .a). Under %files, You can use instead
%exclude %{_libdir}/*a
but you need .so in the devel package, not the main one;
* You need to add
%post -p /sbin/ldconfig %postun -p /sbin/ldconfig
Sorry not to give references to the packaging guide for each item, but I hope that helps, and it's easy to fix. Don't be put off! rpmlint should warn about some of it, and it's worth running fedora-review on the package (if you can -- I have trouble on an EPEL6 system).
https://bugzilla.redhat.com/show_bug.cgi?id=1268010
--- Comment #2 from Dominique Martinet dominique.martinet@cea.fr --- (In reply to Dave Love from comment #1)
- I don't know if you want to support it, but the Group tag is only needed for EPEL5;
Hmm I'll admit I didn't even try to build on EPEL5, will remove it.
You normally shouldn't add explicit Requires for dynamic libraries except typically for devel packages depending on the non-devel version -- the packaging will usually do that automatically. For the devel package the Requires should normally be like
%package devel Requires: %{name}-libs%{?_isa} = %{version}-%{release} ...
Right, will fix. I think I do need to keep the devel package requirements on other -devel packages, though, right? As far as I can tell (rpm -qpi --requires <package>) it could not guess these, and I couldn't find anything in the guidelines about this.
- make should have an argument %{?_smp_mflags} in the %build section, or a comment that it's not smp-safe;
Ok.
- Don't package .la files (and normally not .a). Under %files, You can use instead %exclude %{_libdir}/*a
Hmm I wonder why I added the .la there, if anything it should have been in the -devel... But right rhel systems don't even have it, will remove.
For the .a though I've pretty much always seen it package in the -devel, but I see the packaging guideline now and it makes sense so I guess I'll strip it off as well.
but you need .so in the devel package, not the main one;
Yes, sorry.
You need to add
%post -p /sbin/ldconfig %postun -p /sbin/ldconfig
Ok
Sorry not to give references to the packaging guide for each item, but I hope that helps, and it's easy to fix. Don't be put off! rpmlint should warn about some of it, and it's worth running fedora-review on the package (if you can -- I have trouble on an EPEL6 system).
No problem, thanks for the review! I'll fix all of that and try fedora-review as soon as I get my laptop back home (might be a few days, will update the ticket with new URLs anyway) Our development servers are behind a kerberized proxy and the fedora tools can't seem to work with that, unfortunately :/
https://bugzilla.redhat.com/show_bug.cgi?id=1268010
--- Comment #3 from Dave Love d.love@liverpool.ac.uk --- (In reply to Dominique Martinet from comment #2)
Hmm I'll admit I didn't even try to build on EPEL5, will remove it.
Group does no harm, but some reviewers object to it being there.
Right, will fix. I think I do need to keep the devel package requirements on other -devel packages, though, right? As far as I can tell (rpm -qpi --requires <package>) it could not guess these, and I couldn't find anything in the guidelines about this.
Yes, I looked without success, and it's quite inconsistent in packages generally. I'd have thought the dependencies should be added, but it's difficult to verify them (and I don't know why rpmbuild doesn't try to follow #includes).
Hmm I wonder why I added the .la there, if anything it should have been in the -devel... But right rhel systems don't even have it, will remove.
That's something that is in the guidelines. (I sympathize with "I wonder why I ..."!)
No problem, thanks for the review!
It's only an informal one. I hope someone (Red Hat storage?) can test it, as required for a proper review, and sponsor you. They probably expect to see more packaging and/or informal reviews of other things, first.
If you have any more to contribute, even if it's not ready or appropriate for review, I'd recommend setting up a copr repo. Most of what I install on our cluster goes into mine.
I'll fix all of that and try fedora-review as soon as I get my laptop back home (might be a few days, will update the ticket with new URLs anyway) Our development servers are behind a kerberized proxy and the fedora tools can't seem to work with that, unfortunately :/
I don't know what it needs that would be blocked, but you can run fedora-review on local package source, which is what I meant. I've only managed to do that in a rawhide vagrant box, though, which is more painful than I'd like.
https://bugzilla.redhat.com/show_bug.cgi?id=1268010
Orion Poplawski orion@cora.nwra.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |orion@cora.nwra.com
--- Comment #4 from Orion Poplawski orion@cora.nwra.com --- (In reply to Dave Love from comment #3)
(In reply to Dominique Martinet from comment #2)
I think I do need to keep the devel package requirements on other -devel packages, though, right? As far as I can tell (rpm -qpi --requires <package>) it could not guess these, and I couldn't find anything in the guidelines about this.
Yes, I looked without success, and it's quite inconsistent in packages generally. I'd have thought the dependencies should be added, but it's difficult to verify them (and I don't know why rpmbuild doesn't try to follow #includes).
Yes, -devel packages should require the other devel packages needed to build with it. It's a commonly missed aspect. I suspect it's just a bit too messy to try to generate the proper provides/requires automatically.
https://bugzilla.redhat.com/show_bug.cgi?id=1268010
--- Comment #5 from Upstream Release Monitoring upstream-release-monitoring@fedoraproject.org --- martinetd's scratch build of libmooshika-1.0-4.g3bde2d1.fc22.src.rpm for epel7 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=11358285
https://bugzilla.redhat.com/show_bug.cgi?id=1268010
--- Comment #6 from Dominique Martinet dominique.martinet@cea.fr --- Thanks for confirming.
Here are the new specfile and src.rpm: http://asmadeus.notk.org/CEA/libmooshika-1.0-4.g3bde2d1.spec http://asmadeus.notk.org/CEA/libmooshika-1.0-4.g3bde2d1.fc22.src.rpm
I tried fedora-review which gave a few more feedbacks (not required %clean section, obsolete autoconf flag for libtool...), it's pretty nice. Hadn't noticed the local testing parameter the other day, you're right it should work for me.
(also noticed the el6 i686 package doesn't seem to build, looks like a system header difference from what I could see. I'm probably not going to try pushing it to epel6 so will ignore that for now unless that's a problem.)
I'll try to get attention from the redhat storage people I know to see if I can get it tested. Not sure if many of them have IB adapters available, let's see how it goes.
https://bugzilla.redhat.com/show_bug.cgi?id=1268010
--- Comment #7 from Michael Schwendt (Fedora Packager Sponsors Group) bugs.michael@gmx.net ---
Hmm I wonder why I added the .la there, if anything it should have been in the -devel...
No, that would break the purpose of those files, since libtool .la files also cover run-time linking, e.g. via lt_dlopen().
https://bugzilla.redhat.com/show_bug.cgi?id=1268010
--- Comment #8 from Michael Schwendt (Fedora Packager Sponsors Group) bugs.michael@gmx.net --- Oh, as I just saw this:
http://asmadeus.notk.org/CEA/libmooshika-1.0-4.g3bde2d1.spec
https://fedoraproject.org/wiki/Packaging:Guidelines#Version_and_Release
https://bugzilla.redhat.com/show_bug.cgi?id=1268010
--- Comment #9 from Upstream Release Monitoring upstream-release-monitoring@fedoraproject.org --- martinetd's scratch build of libmooshika-1.0-1.fc22.src.rpm for f23 failed http://koji.fedoraproject.org/koji/taskinfo?taskID=11788334
https://bugzilla.redhat.com/show_bug.cgi?id=1268010
--- Comment #10 from Upstream Release Monitoring upstream-release-monitoring@fedoraproject.org --- martinetd's scratch build of libmooshika-1.0-1.fc22.src.rpm for f24 failed http://koji.fedoraproject.org/koji/taskinfo?taskID=11788301
https://bugzilla.redhat.com/show_bug.cgi?id=1268010
--- Comment #11 from Upstream Release Monitoring upstream-release-monitoring@fedoraproject.org --- martinetd's scratch build of libmooshika-1.0-1.fc22.src.rpm for el6-candidate failed http://koji.fedoraproject.org/koji/taskinfo?taskID=11788377
https://bugzilla.redhat.com/show_bug.cgi?id=1268010
--- Comment #12 from Upstream Release Monitoring upstream-release-monitoring@fedoraproject.org --- martinetd's scratch build of libmooshika-1.0-1.fc22.src.rpm for epel7-candidate failed http://koji.fedoraproject.org/koji/taskinfo?taskID=11788384
https://bugzilla.redhat.com/show_bug.cgi?id=1268010
--- Comment #13 from Upstream Release Monitoring upstream-release-monitoring@fedoraproject.org --- martinetd's scratch build of libmooshika-1.0-1.fc22.src.rpm for epel7-candidate completed http://koji.fedoraproject.org/koji/taskinfo?taskID=11788875
https://bugzilla.redhat.com/show_bug.cgi?id=1268010
--- Comment #14 from Dominique Martinet dominique.martinet@cea.fr --- Hmm, I knew I was in the middle of something so this isn't what I wanted to push but this is weird, the same srpm does mock build on my laptop... Ohwell, will teach me to make sure I have a clean tree at least. Correct ones: http://asmadeus.notk.org/CEA/libmooshika/libmooshika.spec http://asmadeus.notk.org/CEA/libmooshika/libmooshika-1.0-1.fc22.src.rpm
This is purely personal but I don't like the official guideline for git snapshot version so I'm going back to release (I did upstream release 1.0 for these packages so it isn't a problem), I wouldn't mind adding the date but just the date isn't enough (hence my using git describe, which will give strictly ordered release numbers)
Unrelated to the update, but regarding tests of this package, nfs-ganesha will not be packaged with RDMA enabled until we can bundle it separately (there's planned work for this upstream but not short-term), so I doubt this will get any momentum right now... But it's entierely possible to test the lib itself with the utils (-rcat and -rmitm subpackages) -- I doubt anyone has any use for rmitm, but rcat is meant to be similar to netcat in its purpose so it's easy enough to test if you have ibverbs-compatible equipment
I'll also be at supercomputing next week if anyone feels like talking about this or anything else :)
Thanks, Dominique
https://bugzilla.redhat.com/show_bug.cgi?id=1268010
--- Comment #15 from Upstream Release Monitoring upstream-release-monitoring@fedoraproject.org --- martinetd's scratch build of libmooshika-1.0-1.fc22.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=11789146
https://bugzilla.redhat.com/show_bug.cgi?id=1268010
--- Comment #16 from Michael Schwendt bugs.michael@gmx.net ---
This is purely personal but I don't like the official guideline for git snapshot version so I'm going back to release (I did upstream release 1.0 for these packages so it isn't a problem), I wouldn't mind adding the date but just the date isn't enough (hence my using git describe, which will give strictly ordered release numbers)
If you disagree with the guidelines, consider bringing it up on packaging@ mailing-list. Eventually you may want to package a snapshot, and then you would need to return to the topic anyway.
It's the release number that will give a strictly ordered sequence. The git hash is meaningless with regard to updates/upgrades.
Release: 0.%{X}.%{alphatag}%{?dist}
%{X} is the release number to increment for every update of the package. The leading 0. is only for pre-release versions of the package.
%{alphatag} is purely informational and may be as complex as 20110102git9e88d7e not adding much value, since inside the spec file you are supposed to add a comment anyway that would explain how to checkout exactly the same snapshot that has been packaged.
If %X is the same for multiple builds of the package, you've not increased it properly, and only then package tools would take into account the values right of it during version comparison.
%changelog
https://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs
Summary: The mooshika library (libmooshika)
This is an example of why package reviews (and creating guidelines) can be a pain. Repeating the package %{name} in %{summary} is really bad style. Can you think of cases when the summary would be displayed without displaying the package name anywhere next to it?
https://fedoraproject.org/wiki/Packaging:Guidelines#Summary_and_description
Source: %{name}-%{version}.tar.gz
https://fedoraproject.org/wiki/Packaging:SourceURL
It's a rather small and simple package. Consider pointing the fedora-review tool at this ticket: fedora-review -b 1268010 It will look for the latest package files (such as in the Spec URL and SRPM URL lines) and perform many checks on a local test-build. Stuff you should be interested in when doing self-reviews.
package-review@lists.fedoraproject.org
-
Red Hat Bugzilla