[openfabrics-ewg] OFED-1.1-rc2 review comments: openib.spec

[Doug Ledford]

(A lot of this is going to sound like I'm nit picking, but generally
speaking, upstream developers then to concern themselves with the actual
source build files and not rpm spec files.  You guys are stepping into
territory that the engineering people at Red Hat regulate fairly hard in
terms of style and best practices when you create spec files.  Almost
all of the things I'm going to mention here are things I had to change
before they would let me submit the openib-1.0 package for our product.
You obviously don't have to follow suit, but knowing what had to go may
help your future efforts in this area and OS vendor requirements line up
better.  Also, if you are wondering why I didn't bring some of this up
before, they didn't make me go through a formal package review until we
were talking about RHEL5beta packages, and some of this was new to me at
that time.  As a kernel engineer I don't normally have to deal with
packages, the kernel rpm maintainer does.)

You have both SOURCES/openib-1.1.tgz and SRPMS/openib-1.1-0.src.rpm that
contains openib-1.1.tgz.  You only need the tgz in one place.

Default rpm configuration for prefix and libdir is a violation of Linux
File Hierachy Standard.  You're acting as a distributor of an official
software package, you should have your binaries in /usr/bin (unless they
are needed for single user maintenance operation, then /bin)
and /usr/sbin or /sbin.  Even though the distribution may be built on
the customer's machine, building a prepackaged set of files is not the
same as a system admin checking out a svn repo and compiling things
himself and installing from the compile tree.  The /usr/local area is
generally reserved for that latter class of actions.  At most, you
should be installing under /opt/%{name} if you don't feel comfortable
installing to the normal system directories.  However, as a matter of
practicality, since various vendors (Red Hat, SuSE) *are* shipping
versions of this same code, you really *should* be installing to the
official FHS directories so that conflicting installations of this
software and vendor provided software will be detected by the package
management application (aka, you *want* your packages and Red Hat/SuSE
packages to conflict with each other or else you may find that random,
unexplained problems related to multiple versions of software on a
system starts to become an issue).

To comply with the FHS, system daemons should not be in /usr/bin, they
should be in /usr/sbin instead.  Opensmd and sldd.sh come to mind as
things that are primarily used in a daemon capacity and shouldn't really
be in user's path.  Any other applications that require elevated
privileges to operate should go there as well.

Spec files should only build for one arch at a time.  If you need to
build for another arch, your should rerun the build process using
--target <arch> as needed.  If you do this on, for example, x86_64 to
build i386, you get a complete set of i386 and x86_64 rpms, from which
you then install the x86_64 binaries, and both the x86_64 and i386 libs,
and the x86_64 devel packages.  At that point, it's all good.  This
implies that you will need to either A) control compilation from the %
build script in the rpm spec file (what I do) or B) pass the
RPM_BUILD_FLAGS to your build script and then pass them on to make so
that things like -m32 get picked up and the binaries get built as the
proper type.  Likely you will also need to pass the build flags to the
configure actions as well.

The package should build the desired result without being passed any
options (build systems often don't allow you to specify options to the
build process, ours doesn't anyway, so it has to build "out of the
box").

Although you may choose to leave it in your spec, the Vendor: flag is
really intended to be filled in by the people that build and distribute
the binaries.  As such, Red Hat's build system doesn't allow it to exist
because it adds it automatically.

This is a big one, and it's going to cause upgrade path issues.  You
guys *CAN'T* define a static release on all the packages inside
openib.spec independently.  Furthermore, you *CAN'T* have different
builds floating around with the same release numbers.  You have zero
ability to track what actual code someone is running right now with the
way you are doing package n-v-r numbers in your spec file.  If someone
builds the openib rpm, then changes the spec file and adds a patch, and
rebuilds the rpm, all the subpackages come out with the same name as the
first build, even if you passed an updated version option to rpmbuild.
That's ultra broken.

Your selected release numbering on some packages will break upgrades
later.  For example, the release on openib-diags is rc2.  If you are
going to put any rc or pre or beta tags in the release, it should always
be as 0.<rctag>.<buildnumber>.  That way the first build would be
0.rc2.1 and if you change it later and rebuild it would be 0.rc2.2 and
when you have a final release instead of a release candidate, you
eliminate the tag and update the release number to 1 and that preserves
the proper rpm upgrade behavior across all the different build variants.
Without doing this, when you finally release the final version of the
code, rpm won't upgrade to it because the match between rc2 and 1 won't
compare the way you might expect.

Since you have a single, large source rpm that builds lots of seemingly
unrelated subrpms when looking just at names, I would recommend you use
the openib name in more of the subpackage names.  For instance,
openib-perftest is more descriptive than just perftest, and less likely
to cause people to install it only to go "Oh, I don't have infiniband
hardware, never mind".  Likewise with tvflash, mstflint, and several
others.

Multiple rpms are missing proper Requires: definitions.  These can be
caught by working with clean machine installs.  For example, if you
install the opensm-devel package in order to build ibutils, you'll find
that the opensm headers directly include headers from libibcommon-devel
and libibumad-devel without bothering to specify a Requires:
libibcommon-devel, libibumad-devel in the opensm-devel package
definition.

When splitting binaries out from libraries (needed for multilib setups),
either the libs should require the binaries or the binaries the libs.
If there is no reason to have the libs without the binaries, then it's
typical to have the base package (say opensm) use a Requires:
opensm-libs = %{version}-%{release}.  This is to avoid upgrade skew
between libraries and associated binaries.  The same is done for -devel
packages so that the devel package and the libs are always in sync.  So,
as an example, this is the opensm definition in the spec file I used:

%package -n opensm
Version: 1.2.0
Summary: InfiniBand subnet manager and administration libraries
Requires: opensm-libs = %{version}-%{release}
Group: System Environment/Libraries
%description -n opensm
OpenSM provides an implementation for an InfiniBand Subnet Manager and
Administration. Such a software entity is required to run for in order
to initialize the InfiniBand hardware (at least one per each
InfiniBand subnet).

%package -n opensm-libs
Version: 1.2.0
Summary: InfiniBand subnet manager and administration libraries
Requires: openib
Group: System Environment/Libraries
%description -n opensm-libs
OpenSM provides an implementation for an InfiniBand Subnet Manager and
Administration. Such a software entity is required to run for in order
to initialize the InfiniBand hardware (at least one per each
InfiniBand subnet).

%package -n opensm-devel
Version: 1.2.0
Summary: Development files for OpenSM
Group: System Environment/Libraries
Requires: opensm-libs = %{version}-%{release}, libibcommon-devel, libibumad-devel
%description -n opensm-devel
Static libraries and header files for OpenSM.

Note: this is a perfect example of the sort of thing that is totally
broken by using static Release: numbers in the spec file, you could
easily get a devel package and libs using different actual ABIs that
way.

The spec file should have functioning %prep, %build, and %install
procedures that properly separate these three distinct steps.

You have multiple config files spread in different places in /etc.  For
our stuff, I was asked to clean that up, so they all got consolidated
under /etc/ofed so that the naming would be equally applicable to all
the component files that were there.

librdmacm and libibcm both used non-versioned library file names.  Since
that was flagged, I used the function version information in the
respective .map files to version the actual library files on the
filesystem, so libibcm is -version-info 4:0:0 and librdmacm is
-version-info 1:0:0.

As an example of why unversioned filenames for libraries is *bad*, the
ofed-1.0 distribution builds a libibcm.so and relies upon the
information in libibcm.map to version the symbols for the linker.  But,
due to this typo in libibcm/Makefile.am (which still exists in 1.1rc2):

if HAVE_LD_VERSION_SCRIPT
    ibcm_version_script = -Wl,--version-script=$(srcdir)/src/libibcm.map
    ^^^^
else

...

src_libibcm_la_LDFLAGS = -avoid-version $(ucm_version_script)
                                          ^^^

(someone made a cut and past error I think) you now have a libibcm.so
floating around with absolutely 0 symbol versioning and no filename
versioning as well.  Bad.

As an example of why having a monolithic install script that doesn't
split things into steps, the openib tarball preran aclocal and automake
in multiple of the user space library directories (actually, all but
libehca I think, so I special cased that one) meaning that patching the
Makefile.am above won't correct the final problem because changes to the
Makefile.am file are ignored.  So, what you have to do in order to fix
the bug without respinning the tarball (I'm supposed to keep a clean
upstream tarball, so I shouldn't be respinning it myself) is to run the
top level configure and then patch up the resulting Makefile instead.
Rpm is already designed to handle this type of thing and does it well,
versus using the monolithic install script each and every occurrence of
anything like this is a special case edit that has to be patched into
the script.

When adhering the the FHS, there is no appropriate place to put the
uninstall.sh script (although, more importantly, you should be using rpm
to control the install/uninstall of the files and this shouldn't be on
the fs at all).

libsysfs-2.0 and up does away with sysfs_read_attribute_value(), so if
you are going to continue to use sysfs on later installations, you need
to update code to use sysfs_open_attribute/sysfs_read_attribute instead
which exists both on 1.x and 2.0 applications.

As a side note: if you are going to make different kernels put a sysfs
attribute in different places (reference the librdmacm discussion on
list recently), you need to make the library check *both* places, not
just one.  You don't want the library tied to a specific kernel hack and
vice versa.  The library should work regardless of which kernel variant
it's on.  A better solution though would be to only have the file in one
place, and if it isn't there because of an older kernel, assume V1 and
continue since the later V2 stuff is in kernels that have the class/misc
available to the rdmacm kernel code, generally speaking.

OK, that pretty much covers it.  I know this was long.  It took me 3
days of back and forth with the package reviewer to get the initial
openib-1.0-3.el5 package approved.  FWIW, ibutils and openmpi breezed
through in comparison with mainly just making them FHS compliant.

-- 
Doug Ledford <dledford at redhat.com>
              GPG KeyID: CFBFF194
              http://people.redhat.com/dledford

Infiniband specific RPMs available at
              http://people.redhat.com/dledford/Infiniband
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
URL: <http://lists.openfabrics.org/pipermail/ewg/attachments/20060825/9f29c0f8/attachment.sig>