Bug 1268010 - Review Request: libmooshika - helper library for RDMA
Review Request: libmooshika - helper library for RDMA
Status: NEW
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
:
Depends On:
Blocks: FE-NEEDSPONSOR
  Show dependency treegraph
 
Reported: 2015-10-01 10:43 EDT by Dominique Martinet
Modified: 2015-12-01 04:51 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed:
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 Dominique Martinet 2015-10-01 10:43:58 EDT
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.
Comment 1 Dave Love 2015-10-05 07:46:22 EDT
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).
Comment 2 Dominique Martinet 2015-10-05 08:12:28 EDT
(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 :/
Comment 3 Dave Love 2015-10-05 16:04:25 EDT
(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.
Comment 4 Orion Poplawski 2015-10-05 21:39:08 EDT
(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.
Comment 5 Upstream Release Monitoring 2015-10-07 10:59:51 EDT
martinetd's scratch build of libmooshika-1.0-4.g3bde2d1.fc22.src.rpm for epel7 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=11358285
Comment 6 Dominique Martinet 2015-10-07 11:28:45 EDT
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.
Comment 7 Michael Schwendt 2015-10-18 11:54:23 EDT
> 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().
Comment 9 Upstream Release Monitoring 2015-11-11 03:20:21 EST
martinetd's scratch build of libmooshika-1.0-1.fc22.src.rpm for f23 failed http://koji.fedoraproject.org/koji/taskinfo?taskID=11788334
Comment 10 Upstream Release Monitoring 2015-11-11 03:20:29 EST
martinetd's scratch build of libmooshika-1.0-1.fc22.src.rpm for f24 failed http://koji.fedoraproject.org/koji/taskinfo?taskID=11788301
Comment 11 Upstream Release Monitoring 2015-11-11 03:24:45 EST
martinetd's scratch build of libmooshika-1.0-1.fc22.src.rpm for el6-candidate failed http://koji.fedoraproject.org/koji/taskinfo?taskID=11788377
Comment 12 Upstream Release Monitoring 2015-11-11 03:25:14 EST
martinetd's scratch build of libmooshika-1.0-1.fc22.src.rpm for epel7-candidate failed http://koji.fedoraproject.org/koji/taskinfo?taskID=11788384
Comment 13 Upstream Release Monitoring 2015-11-11 04:06:02 EST
martinetd's scratch build of libmooshika-1.0-1.fc22.src.rpm for epel7-candidate completed http://koji.fedoraproject.org/koji/taskinfo?taskID=11788875
Comment 14 Dominique Martinet 2015-11-11 04:34:15 EST
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
Comment 15 Upstream Release Monitoring 2015-11-11 04:35:31 EST
martinetd's scratch build of libmooshika-1.0-1.fc22.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=11789146
Comment 16 Michael Schwendt 2015-12-01 04:51:02 EST
> 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.

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