[Bug 246387] Review Request: libibcommon - OpenFabrics Alliance InfiniBand management common library

Wed Jul 4 21:13:01 UTC 2007

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Review Request: libibcommon - OpenFabrics Alliance InfiniBand management common library

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=246387

------- Additional Comments From dledford at redhat.com  2007-07-04 17:12 EST -------
(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.

-- 
Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.