Bug 246387

This builds fine for me; rpmlint complains about the lack of a changelog entry,
which you need.

The upstream URL seems to make no mention of this package.  Is it really the
correct URL?

Given that this is a git snapshot, is it really a good idea to ship static
libraries?  I'd imagine that it would be a bad idea to link statically against
in-development code.  (And if you really need the static library, please include
a comment in your spec as to why it's required.)

To be kind to the potential reviewers of your packages, please include the text
from the spec's Summary: in the Summary field of the ticket and include actual
links to the spec and srpm.

That can be added, but needs to be added by upstream.  The spec file is
autogenerated from the actual code repo, so any changes to the spec file that
aren't done in the upstream repo would be lost.  In the actual tarball is the
file libibcommon.spec.in that is used to generate the actual spec file.

Yes, www.openfabrics.org is the right URL.  The lack of mention of this
particular package is because they really don't talk a lot about the individual
packages on their web site, they tend to only talk about their largish (40MB+
src.rpm) Open Fabrics Enterprise Distribution, which includes this package and
about 15 others in one single src.rpm.  To see how that rpm builds out, look at
http://people.redhat.com/dledford/Infiniband/openib/1.2/1.el5/ and see for
yourself just how much crap they pack into one rpm.  I didn't think that was
suitable for Fedora, so I'm breaking it back out into individual rpms based upon
their git repos with properly crafted spec files (in terms of BuildRequires: and
such, little details they get to skip entirely by putting it all into one rpm)
and then pushing the changes upstream.  The maintainer of the repo that this
package comes from has accepted my changes in, but that doesn't mean he's gotten
around to making any official releases of this package separate from the larger
conglomerate package yet.  Hence why the URL doesn't say much about this
package, but only talks about their mondo package instead.

As for the git snapshot, I created a make.dist script for the person that
maintains this repo, and that script will spit out either a libibcommon-git.tgz
file or a libibcommon-%{version}.tgz file and tags the version in the repo and
does some other checking.  It's not my place to spit out that versioned, tagged,
official tarball, and since he doesn't have any up on the web site outside of
the mondo rpm, I created this with a git snapshot.  Once we've worked through
the review process and have things on track, I'm going to be helping the
upstream maintainer through the process of becoming a Fedora contributor and
then through the process of taking over the maintenance of this package.  He can
readily create the official tarball and official release package instead of a
git snapshot where as I can't.

As for the static library, that's a common request of the people that make use
of this code.  The customers I here from most are people running anywhere from
256 to 8000 node clusters with Infiniband hardware and they have been requesting
we make static libs available.

Thanks for the suggestions on the reviewing, I'll make sure and do those things
in the future.

(In reply to comment #2)
> That can be added, but needs to be added by upstream.

I assume you're referring to the changelog.  The problem is that the spec is
part of Fedora, not the upstream source.  It's maintained in Fedora CVS; someone
else might come along and fix a bug or bump the release and rebuild the package,
and at that point you'd end up with a change that's not upstream.

> Yes, www.openfabrics.org is the right URL.

OK, just checking.

> As for the git snapshot, I created a make.dist script for the person that
> maintains this repo,

Well, as long as you're willing to deal with the pain of dealing with
unversioned upstream source, there's no problem with it.

> As for the static library, that's a common request of the people that make use
> of this code.

It just seems like a really poor idea for anyone to link statically against
moving-target development code.  Still, it's OK as long as nothing in Fedora
actually links against those static libs.

(In reply to comment #3)
> (In reply to comment #2)
> > That can be added, but needs to be added by upstream.
> 
> I assume you're referring to the changelog.

Correct.

>  The problem is that the spec is
> part of Fedora, not the upstream source.

Yes, but the upstream maintainer is working on becoming a Fedora contributor and
would like to be the maintainer of the Fedora package for this library.  At that
point, any distinction between upstream and Fedora is moot.

In any case, I can add a placeholder changelog entry.

> > Yes, www.openfabrics.org is the right URL.
> 
> OK, just checking.
> 
> > As for the git snapshot, I created a make.dist script for the person that
> > maintains this repo,
> 
> Well, as long as you're willing to deal with the pain of dealing with
> unversioned upstream source, there's no problem with it.

It's really not that bad in this case.

> > As for the static library, that's a common request of the people that make use
> > of this code.
> 
> It just seems like a really poor idea for anyone to link statically against
> moving-target development code.  Still, it's OK as long as nothing in Fedora
> actually links against those static libs.

They do it.  I guess when a job takes weeks to complete on 1000s of nodes, they
*really* care about their performance.  They also do it precisely *because* the
code is still moving.  When they find a version they know works well, they may
statically link their longer running cluster apps so they know they won't crash
with the next library update, while letting shorter lived apps try things out. 
In any case, I don't use any of them in the various packages I'm building for
Fedora, they are just used by end users.

(In reply to comment #4)
> Yes, but the upstream maintainer is working on becoming a Fedora contributor 
> and would like to be the maintainer of the Fedora package for this library.  
> At that point, any distinction between upstream and Fedora is moot.

Unless there's some legal issue with the CLA or something like that, why not
have the upstream author contribute here directly and get sponsored and such as
part of getting this package in?

> In any case, I can add a placeholder changelog entry.

Well, it should be a proper changelog and should reflect the changes made in
this review, sure.

In any case, I think this package is close to being done.  Just give a link to
an updated srpm and I'll take a look.

(In reply to comment #5)
> (In reply to comment #4)
> > Yes, but the upstream maintainer is working on becoming a Fedora contributor 
> > and would like to be the maintainer of the Fedora package for this library.  
> > At that point, any distinction between upstream and Fedora is moot.
> 
> Unless there's some legal issue with the CLA or something like that, why not
> have the upstream author contribute here directly and get sponsored and such as
> part of getting this package in?

Because I'm waiting on this package (and a few others) before I can build
updated openmpi packages that support Infiniband.  I suspect if I waited for the
upstream people to do the whole process themselves, it would be next year before
it got done.  If I get it done, then I can do what needs done until they've
completed the process.  Even if they *never* complete the process, it's easier
for me to handle the individual packages than it is the big openib package
that's in RHEL.  So getting them in here so I can import them into RHEL is a
benefit to me.

> In any case, I think this package is close to being done.  Just give a link to
> an updated srpm and I'll take a look.

I didn't make any changes other than to add a changelog ;-)  But, they are on
the same place that they were before:

http://www.xsintricity.com/dledford/Package_Review/

There is an updated src.rpm from 20070703, and the spec files have been updated.

I talked with some other folks about this and the bottom line is that four of us
(Brian Pepple, Kevin Fenzi, Toshio Kuratomi and I) all agree that we have issues
with the intended method of maintenance of this package.  We have packages in
the distribution where the spec is maintained as part of the upstream source,
but with those packages the spec is actually pulled from Fedora into the
upstream source, which is the opposite direction from this package.

Ultimately, the following sentence from comment #2 is a deal-breaker for us:

The spec file is autogenerated from the actual code repo, so any changes to the
spec file that aren't done in the upstream repo would be lost.

Is it possible that there's a misunderstanding here?

As an act of good faith, let me go ahead and run through a full review so that
we can make progress in the event that the above issue is resolved.

rpmlint says:
W: libibcommon incoherent-version-in-changelog - 1.0.3-0.3.20070703git.fc8
  Your changelog entries need to be in the proper format, which includes the 
  version and release; see 
  http://fedoraproject.org/wiki/Packaging/Guidelines#Changelogs

W: libibcommon-devel no-documentation
W: libibcommon-static no-documentation
  These are OK.

I can't fetch the upstream source.  I don't even know how to fetch a tarball
from a git URL.  You will need to provide instructions for grabbing the source
from upstream; see http://fedoraproject.org/wiki/Packaging/SourceURL

Note that there are some who are firmly against ever running the autotools in a
spec; I happen to not be one of them, but that's really the kind of thing that
should be done by upstream as part of making the source snapshot.

It's not actually necessary to run ldconfig in the -devel package.

Nothing seems to own /usr/include/infiniband.  Actually, libibverbs-devel owns
it, but it's not in the dependency list.  See
http://fedoraproject.org/wiki/Packaging/Guidelines#FileAndDirectoryOwnership

Review:
X can't compare source files with upstream.
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* summary is OK.
* description is OK.
* dist tag is present.
* build root is OK.
* license field matches the actual license.
* license is open source-compatible.
* license text included in package.
* latest version is being packaged.
* BuildRequires are proper.
* compiler flags are appropriate.
* %clean is present.
* package builds in mock (development, x86_64).
* package installs properly
* debuginfo package looks complete.
X rpmlint has one valid complaint (changelog format).
* final provides and requires are sane:
  libibcommon-1.0.3-0.3.20070703git.fc8.x86_64.rpm
   libibcommon.so.1()(64bit)
   libibcommon.so.1(IBCOMMON_1.0)(64bit)
   libibcommon = 1.0.3-0.3.20070703git.fc8
  =
   /sbin/ldconfig
   libibcommon.so.1()(64bit)

  libibcommon-devel-1.0.3-0.3.20070703git.fc8.x86_64.rpm
   libibcommon-devel = 1.0.3-0.3.20070703git.fc8
  =
?  /sbin/ldconfig
   libibcommon = 1.0.3-0.3.20070703git.fc8
   libibcommon.so.1()(64bit)

  libibcommon-static-1.0.3-0.3.20070703git.fc8.x86_64.rpm
   libibcommon-static = 1.0.3-0.3.20070703git.fc8
  =
   libibcommon = 1.0.3-0.3.20070703git.fc8

* %check is not present; not test suite upstream.
* shared libraries present; ldconfig is called as necessary in the main package 
   and the unversioned .so file is in the -devel subpackage.
X nothing owns /usr/lib/infiniband
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* scriptlets are OK (ldconfig)
* code, not content.
* documentation is small, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* headers are in the -devel subpackage.
* no pkgconfig files.
* static libraries are in the -static subpackage.
* no libtool .la files.

(In reply to comment #7)
> I talked with some other folks about this and the bottom line is that four of us
> (Brian Pepple, Kevin Fenzi, Toshio Kuratomi and I) all agree that we have issues
> with the intended method of maintenance of this package.  We have packages in
> the distribution where the spec is maintained as part of the upstream source,
> but with those packages the spec is actually pulled from Fedora into the
> upstream source, which is the opposite direction from this package.

Well, that's fine if someone maintains an upstream code base that doesn't go
into anything other than Fedora, but what if upstream wants to maintain their
own package, and they maintain that package for multiple distributions?

Personally, the concept of the spec file flowing from Fedora to upstream is
backwards IMO.  I'm fine with maintaining a spec file apart from upstream,
especially since most upstream repos don't want to maintain a spec at all.  But,
if they have a spec, it's *their* spec.  We can use it or ignore it, but they
have no obligation to use ours.  And considering that Fedora is no longer the
only distro where people can contribute and maintain packages, it would be
arrogant to assume that our spec would suit all of their possible needs.

That said, I'm perfectly fine ignoring the upstream spec and maintaining our
own.  It's what I already do for the huge openib rpm that the OpenFabrics.org
people put out.  So, at least as long as I'm maintaining the package, I'll just
do it that way.  If the upstream maintainer does get himself vetted through the
contributor process, then I'll make sure he's aware of the issues regarding
preservation of a suitable spec file and other contributor's changes.


> Ultimately, the following sentence from comment #2 is a deal-breaker for us:
> 
> The spec file is autogenerated from the actual code repo, so any changes to the
> spec file that aren't done in the upstream repo would be lost.
> 
> Is it possible that there's a misunderstanding here?

Well, although it's rendered moot by my agreement to ignore the usptream spec,
I'll go ahead and explain this.  There's a minor misunderstanding.  My comment
was that any changes I made would be lost when any hand off to the upstream repo
maintainer happened.  That's just because his initial CVS co would be up to
date, so the first time he copied his generated spec file into CVS, the
subsequent check in would proceed without a hitch regardless of changes made. 
That's a problem that could be solved by a one time merge at the time of hand off.

What I left unsaid, but assumed, was that once he's checked things out, if
someone else modifies the spec file, than any attempt to just copy his generated
spec file into CVS followed by a check in would result in a failed up to date
check on the spec file, at which point he would have to check out the changes
made by other people and could merge those changes into not just the local spec
file but also the spec.in file in his repo (should they be appropriate for
that).  So, he won't be able to silently overwrite changes other people make. 
But, that doesn't necessarily mean he'll keep their changes either, just depends
on what they are.

> As an act of good faith, let me go ahead and run through a full review so that
> we can make progress in the event that the above issue is resolved.
> 
> rpmlint says:
> W: libibcommon incoherent-version-in-changelog - 1.0.3-0.3.20070703git.fc8
>   Your changelog entries need to be in the proper format, which includes the 
>   version and release; see 
>   http://fedoraproject.org/wiki/Packaging/Guidelines#Changelogs

I didn't bother with the git package just because the full release is so long. 
But, I built a new package against an actual released tarball.  The new package
and spec is at the same URL as before:

http://www.xsintricity.com/dledford/Package_Review/

> W: libibcommon-devel no-documentation
> W: libibcommon-static no-documentation
>   These are OK.
> 
> I can't fetch the upstream source.  I don't even know how to fetch a tarball
> from a git URL.  You will need to provide instructions for grabbing the source
> from upstream; see http://fedoraproject.org/wiki/Packaging/SourceURL

New URL/Source in the package.  As I mentioned before, this particular git repo
actually spits out 5 different release tarballs.  Only 3 of them from this repo
are present at the URL.  I've asked the maintainer to go ahead and put up
tarballs for the other two from any previous stable release in the git repo so I
can use them for those two additional packages as well.

> Note that there are some who are firmly against ever running the autotools in a
> spec; I happen to not be one of them, but that's really the kind of thing that
> should be done by upstream as part of making the source snapshot.

When using a tarball, as in the new package, it is done by upstream and dropped
from the spec file.

> It's not actually necessary to run ldconfig in the -devel package.

Dropped.

> Nothing seems to own /usr/include/infiniband.  Actually, libibverbs-devel owns
> it, but it's not in the dependency list.  See
> http://fedoraproject.org/wiki/Packaging/Guidelines#FileAndDirectoryOwnership

Yep.  This is correct.  Libibverbs is the core layer if you will for the
infiniband stack.  No infiniband using app can be built without it (well, they
could, but it would require writing an actual hardware driver to do so).  So any
app that wants to link against this library and do useful things must also link
against libibverbs in order to have a transport over which to send the output of
this library.  Therefore, there's an indirect requirement on libibverbs from
this package, but it's through the apps that use this library.  This library
itself doesn't need anything from libibverbs.  They do, however share the
include directory.  But, since apps can be written without this library *much*
easier than they can be written without libibverbs, we make libibverbs the
directory owner.

> Review:
> X can't compare source files with upstream.

Fixed.

> * package meets naming and versioning guidelines.
> * specfile is properly named, is cleanly written and uses macros consistently.
> * summary is OK.
> * description is OK.
> * dist tag is present.
> * build root is OK.
> * license field matches the actual license.
> * license is open source-compatible.
> * license text included in package.
> * latest version is being packaged.
> * BuildRequires are proper.
> * compiler flags are appropriate.
> * %clean is present.
> * package builds in mock (development, x86_64).
> * package installs properly
> * debuginfo package looks complete.
> X rpmlint has one valid complaint (changelog format).

Fixed.

> * final provides and requires are sane:
>   libibcommon-1.0.3-0.3.20070703git.fc8.x86_64.rpm
>    libibcommon.so.1()(64bit)
>    libibcommon.so.1(IBCOMMON_1.0)(64bit)
>    libibcommon = 1.0.3-0.3.20070703git.fc8
>   =
>    /sbin/ldconfig
>    libibcommon.so.1()(64bit)
> 
>   libibcommon-devel-1.0.3-0.3.20070703git.fc8.x86_64.rpm
>    libibcommon-devel = 1.0.3-0.3.20070703git.fc8
>   =
> ?  /sbin/ldconfig
>    libibcommon = 1.0.3-0.3.20070703git.fc8
>    libibcommon.so.1()(64bit)
> 
>   libibcommon-static-1.0.3-0.3.20070703git.fc8.x86_64.rpm
>    libibcommon-static = 1.0.3-0.3.20070703git.fc8
>   =
>    libibcommon = 1.0.3-0.3.20070703git.fc8
> 
> * %check is not present; not test suite upstream.
> * shared libraries present; ldconfig is called as necessary in the main package 
>    and the unversioned .so file is in the -devel subpackage.
> X nothing owns /usr/lib/infiniband

I think you mean /usr/include/infiniband, and if that's the case see above.

> * doesn't own any directories it shouldn't.
> * no duplicates in %files.
> * file permissions are appropriate.
> * scriptlets are OK (ldconfig)
> * code, not content.
> * documentation is small, so no -docs subpackage is necessary.
> * %docs are not necessary for the proper functioning of the package.
> * headers are in the -devel subpackage.
> * no pkgconfig files.
> * static libraries are in the -static subpackage.
> * no libtool .la files.

Ping.  Any updates on this?

This is not a valid value for the License Tag:
License: GPL/BSD
See:
http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#head-f21ae23bf2f278444e2c385463cfa74a502396b8


This is afaik not needed:
Requires(post): /sbin/ldconfig
Requires(postun): /sbin/ldconfig

Because this already creates the Requires:
%post -p /sbin/ldconfig
%postun -p /sbin/ldconfig


I am not sure, is the -static subpackage useful without the -devel package? If
not, then the -static package should depend on the -devel package and not on the
main package.


%install must begin with "rm -rf $RPM_BUILD_ROOT"


This
%{_includedir}/infiniband/*.h
should be
%{_includedir}/infiniband/
to include the directory.

Upstream used to put out a 50MB rpm that spit out something like 30 different
sub-rpms.  After massive brow beating on my part, they split everything up into
individual rpms.  I've had the process of getting them into fedora on the back
burner as I sort through the individual rpms and whip them into shape.  So,
originally this package depended on that huge package, and now it depends on
some of the split off packages.  Since this needs to change to accommodate, and
since the others aren't all done and in yet, this is languishing.  It might be
easiest to just close out all the review requests that were cloned from 246390
and let me reopen new review requests when the dust has settled.

*** This bug has been marked as a duplicate of bug 460927 ***