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.
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).
(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 :/
(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.
(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.
martinetd's scratch build of libmooshika-1.0-4.g3bde2d1.fc22.src.rpm for epel7 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=11358285
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.
> 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().
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
martinetd's scratch build of libmooshika-1.0-1.fc22.src.rpm for f23 failed http://koji.fedoraproject.org/koji/taskinfo?taskID=11788334
martinetd's scratch build of libmooshika-1.0-1.fc22.src.rpm for f24 failed http://koji.fedoraproject.org/koji/taskinfo?taskID=11788301
martinetd's scratch build of libmooshika-1.0-1.fc22.src.rpm for el6-candidate failed http://koji.fedoraproject.org/koji/taskinfo?taskID=11788377
martinetd's scratch build of libmooshika-1.0-1.fc22.src.rpm for epel7-candidate failed http://koji.fedoraproject.org/koji/taskinfo?taskID=11788384
martinetd's scratch build of libmooshika-1.0-1.fc22.src.rpm for epel7-candidate completed http://koji.fedoraproject.org/koji/taskinfo?taskID=11788875
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
martinetd's scratch build of libmooshika-1.0-1.fc22.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=11789146
> 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.
This is an automatic check from review-stats script. This review request ticket hasn't been updated for some time. We're sorry it is taking so long. If you're still interested in packaging this software into Fedora repositories, please respond to this comment clearing the NEEDINFO flag. You may want to update the specfile and the src.rpm to the latest version available and to propose a review swap on Fedora devel mailing list to increase chances to have your package reviewed. If this is your first package and you need a sponsor, you may want to post some informal reviews. Read more at https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group. Without any reply, this request will shortly be considered abandoned and will be closed. Thank you for your patience.
Closing this, since I haven't had time in 5 years I don't think I ever will (although I have since then released 1.0 so all the last discussions on git snapshots would be easier to fix now... oh well) Thanks Dave, Orion and Michael for the reviews though!