Spec URL: http://www.openfabrics.org/downloads/mlx4/fedora/libmlx4.spec SRPM URL: http://www.openfabrics.org/downloads/mlx4/fedora/libmlx4-1.0-0.1.rc1.fc8.src.rpm Description: libmlx4 provides a device-specific userspace driver for Mellanox ConnectX HCAs for use with the libibverbs library. Note that libibverbs is already part of Fedora, as is the libmthca userspace driver package. I packaged libmlx4 almost identically to the existing libmthca package. [This package is of 1.0-rc1, but the 1.0 final release is expected within two weeks; I am entering this bug now to start the review process, but the package I actually upload will be of the final 1.0 release]
The real 1.0 release of libmlx4 has come out, and I updated my package: Spec URL: http://www.openfabrics.org/downloads/mlx4/fedora/libmlx4.spec SRPM URL: http://www.openfabrics.org/downloads/mlx4/fedora/libmlx4-1.0-1.fc8.src.rpm
I guess all of the other reviewers are stared away by esoteric things like hardware we couldn't hope to afford, but I am fearless. Or am I insane? I keep forgetting. Builds OK for me on rawhide; rpmlint says: libmlx4.x86_64: W: conffile-without-noreplace-flag /etc/libibverbs.d/mlx4.driver If it's a config, you probably don't want an rpm update wiping out end-user customization, so you should use %config(noreplace). The difference is whether rpm creates a .rpmnew file instead of moving the old version to .rpmsave. libmlx4-devel-static.x86_64: W: no-documentation Not a problem. Generally, the package containing the static library should be named "-static". However, if this would leave the -devel package empty, you can put the library in the -devel package and have it provide -static. See the "Packaging Static Libraries" section of http://fedoraproject.org/wiki/Packaging/Guidelines. I know this package is a little odd (unversioned so, no headers to compile against) but I think the static library bits in the guidelines still cover this situation well enough. Since you install a shared library, you need to call ldconfig: %post -p /sbin/ldconfig %postun -p /sbin/ldconfig Checklist: * source files match upstream: ced3d0d5ac965e5d9c1230ecb98a6fb644906b6cdf25c117fabbdce0e0be2974 libmlx4-1.0.tar.gz X package does not follow the naming guidelines for static library packages. * 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 (rawhide, x86_64). * package installs properly * debuginfo package looks complete. X rpmlint has a valid complaint. * Ignoring the -static package issue, final provides and requires are sane: libmlx4-1.0-1.fc9.x86_64.rpm config(libmlx4) = 1.0-1.fc9 libmlx4-rdmav2.so()(64bit) libmlx4 = 1.0-1.fc9 = config(libmlx4) = 1.0-1.fc9 libibverbs.so.1()(64bit) libibverbs.so.1(IBVERBS_1.0)(64bit) libibverbs.so.1(IBVERBS_1.1)(64bit) libmlx4-devel-static-1.0-1.fc9.x86_64.rpm libmlx4-devel-static = 1.0-1.fc9 = libmlx4 = 1.0-1.fc9 * %check is not present; no test suite upstream. I haven't a clue how to test this, and I don't have the hardware anyway. X a shared library is installed, but ldconfig is not run. * owns the directories it creates. * doesn't own any directories it shouldn't. * no duplicates in %files. * file permissions are appropriate. X no scriptlets present (but there should be) * code, not content. * documentation is small, so no -doc subpackage is necessary. * %docs are not necessary for the proper functioning of the package. * no headers. * no pkgconfig files. X static libraries should be in the -devel package. * no libtool .la files.
Thanks for the review! A few comments/questions: > libmlx4.x86_64: W: conffile-without-noreplace-flag > /etc/libibverbs.d/mlx4.driver > If it's a config, you probably don't want an rpm update wiping out end-user > customization, so you should use %config(noreplace). The difference is whether > rpm creates a .rpmnew file instead of moving the old version to .rpmsave. the mlx4.driver file is not exactly a config file in the sense that there is no useful change that a user can make right now; all it contains is a single line that tells the libibverbs library to look for an "mlx4" plugin. However if I don't mark it as a conffile then rpmlint complains about non-config files in /etc. On the other hand, if some future version does introduce new info in the mlx4.driver file, I think the best thing for a user would be to update the file, so I left out the "noreplace" marking. I don't have a strong opinion about the right thing here; although I did think about this and make a semi-informed choice, just let me know if you want me to change how the mlx4.driver file is handled, and I'll update my package. > Generally, the package containing the static library should be named "-static". > However, if this would leave the -devel package empty, you can put the library > in the -devel package and have it provide -static. Make sense, I will make the change and add the provides tag. > Since you install a shared library, you need to call ldconfig: > %post -p /sbin/ldconfig > %postun -p /sbin/ldconfig The same issue came up when my libmthca package was reviewed (bug 169744). The package does contain a .so file, but it is not something that is linked to applications; rather, libibverbs uses dlopen() to load this plugin. So I don't think running ldconfig is necessary or desirable for this package. I will prepare a package with the -devel-static -> -devel change and post it. Let me know if my explanations for the other points make sense.
Note that the non-conffile-in-etc complaint is not absolute; there are configuration things in /etc that aren't user configurables, like the files in /etc/ld.so.conf.d. This seems to be in the same vein, and from your description it would be appropriate for that file to not be marked as %config. The real problem comes when files are marked %config but not %config(noreplace) as that's explicitly saying that we expect the user to modify the file but we'll also happly nuke their changes on package upgrades and force them to hunt down the .rpmsave file and pull their changes back in. Hence plain %config should be used only very rarely. About .so files used as plugins: If nothing ever links to them, they probably should not live in the standard library directory. Is it possible for these files to live elsewhere?
> Note that the non-conffile-in-etc complaint is not absolute OK, I will just remove the %config marking. > About .so files used as plugins: If nothing ever links to them, they probably > should not live in the standard library directory. Is it possible for these > files to live elsewhere? It's not that easy for the .so plugins to be out of the standard library path. libibverbs uses dlopen() without a full path to open the plugin. This means that the standard library path is used to find the .so. This has the advantage that we don't have to reinvent all the library path configuration stuff; for example the user can use LD_LIBRARY_PATH to point to a different version of a plugin if desired, or optimized builds can be put in /lib/tls/i686/sse2, etc. I guess the main point is that the upstream libibverbs will only search the standard library path, and the Fedora package keeps this behavior. So there's not really any way to change this without redoing the Fedora libibverbs and libmthca package, diverging from upstream, and causing pain for users who want to install unmodified upstream plugins (there are several others not yet packaged for Fedora, eg libcxgb3, libipathverbs, libehca).
I uploaded the updated package: * Sun Jan 27 2008 Roland Dreier <rdreier> - 1.0-2 - Spec file cleanups, based on Fedora review: don't mark libmlx4.driver as a config file, since it is not user modifiable, and change the name of the -devel-static package to plain -devel, since it would be empty without the static library. Spec URL: http://www.openfabrics.org/downloads/mlx4/fedora/libmlx4.spec SRPM URL: http://www.openfabrics.org/downloads/mlx4/fedora/libmlx4-1.0-2.fc8.src.rpm The only outstanding issue is the plugin .so as discussed above, but I don't think there is a better way to handle this.
This picked up an additional rpmlint complaint, as expected: libmlx4.x86_64: W: non-conffile-in-etc /etc/libibverbs.d/mlx4.driver but that's OK. About the .so file, it is somewhat suboptimal, but this wouldn't be the first package in the distro to do this and you do have a reasonable point about being able to have specifically optimized versions without duplicating all of that logic from the loader. So I guess there's not much to do. Outside of that, everything looks good to me. APPROVED
New Package CVS Request ======================= Package Name: libmlx4 Short Description: device-specific userspace driver for Mellanox ConnectX HCAs for use with the libibverbs library Owners: rolandd Branches: F-7 F-8 InitialCC: Cvsextras Commits: yes
cvs done.
Successfully built.