[ofa-general] Re: Input on the new OFED package
Doug Ledford
dledford at redhat.com
Thu Oct 25 15:50:11 PDT 2007
On Thu, 2007-10-25 at 18:27 +0200, Tziporet Koren wrote:
> Hi Doug,
> Since we updated OFED install and user space packages according to your
> request I would like to get input to see if its ease your integration
> and if you have more input to give us.
>
> Thanks,
> Tziporet
It's definitely better being all split up, but I'm still going to have
to replace at least some of the spec files wholesale (well, currently
all of them). I'll pick on the ibutils spec file as my example and I'm
cc:ing this to the list so I don't need to do this over and over again.
[ I'm not pasting the big copyright notice ]
For both RHEL and Fedora, the spec file is generally considered Red Hat
property and it is always under GPL regardless of the license on the
software package itself. We usually both write and maintain our spec
files ourselves, so this is OK. But, that means that if you guys want
me to use your spec files, then I can't import them with this
multi-license copyright. What's more, even if they get imported, they
will be imported once and only once. From that point on, they will be
maintained internally. This is so that when the next update rolls
around, we don't loose the internal history of various builds we did
between upstream releases. So, that being said, you guys are free to
ignore the rest of this email if you don't care if I ever import your
spec files. However, even if I don't import your spec files, the things
I point out are good spec file practice *anyway*, so you might care just
for reasons of spec file quality.
# Disable debugging
%define __check_files %{nil}
# Disable brp-lib64-linux
%ifarch x86_64 ia64
%define __arch_install_post %{nil}
%endif
Why? If you don't have a good reason to disable debug packages or the
__arch_install_post, then don't.
%{!?_prefix: %define _prefix /usr}
%{_prefix} is never not defined, so this is a useless statement.
%{!?configure_options: %define configure_options %{nil}}
%define build_ibmgtsim %(if ( echo %{configure_options} | grep
"enable-ibmgtsim"
> /dev/null ); then echo -n '1'; else echo -n '0'; fi)
This is a horrible construct. If you really want the ability to pass in
a configure option to rpmbuild to control this, then define a specific
option to pass and do something like:
%{?build_ibmgtsim: %define ibmgtsim --enable-ibmgtsim}
and then in the actual call to ./configure pass an option of
%{?ibmgtsim}
That will save a shell/grep spawn and is much cleaner.
Name: %{?_name:%{_name}}%{!?_name:ibutils}
It's never valid to play games with the name of the package. If you
were to change the name to, say, ibutils-gen2, then you would need to
add a Conflicts: ibutils or Obsoletes: ibutils to the spec file to avoid
rpm conflicts. Since the spec file needs a history of all the previous
names you are obsoleting, random name changes aren't really possible nor
should they be done lightly.
License: GPL/BSD
Although this is a specific requirement of Fedora and not other distros,
it can't hurt to do for other distros as it clearly identifies and
differentiates the different license scenarios at a glance.
The license tag should specify all the possible valid license scenarios,
and there is a list of accepted abbreviations for the various licenses
that exist. In the case of this package, it is available under either
the GPL or BSD license (which is distinctly different from packages that
have some files under one license and others under another, in which
case both licenses apply all the time). The proper license tag depends
on the exact GPL and BSD licenses in use. For example, if the GPL
license in use is version 2 or later, then the abbreviation would be
GPLv2+. If the BSD option is the original BSD license that has an
advertising clause, it would be BSD with advertising. The situation of
the multiple license, aka whether your choice of one applies or all
apply simultaneously, is differentiated by the use of the words and/or
between clause abbreviations. So the correct license for this package
would be (assuming I have the right BSD type, it could be another BSD
type):
License: GPLv2+ or BSD with advertising
See http://fedoraproject.org/wiki/Licensing for a complete list of the
license abbreviations to date.
Url: http://openib.org/downloads/%{name}-%{version}.tar.gz
Source:
http://www.openfabrics.org/downloads/ibutils-1.2-0.4.ofed20070930.tar.gz
The URL should point not to the tarball itself, but to the project web
page. Regardless though, you need to update the URL to openfabrics.org
if that's your download site.
# Requires: opensm
It's correct that you should not need to specify a Requires: line for
this package. However, what you *should* have in there is a
BuildRequires: that specifies the -devel packages you need in order to
build this package. The Requires: portion is mostly automatically
generated by rpm by looking up the ldd output of the binaries you
package up.
%setup -n %{name}-%{version}
Shouldn't need the -n option if the tarball is packaged correctly.
###
### build
###
%build
OK, if someone can't figure out that %build is the build section and
needs a big comment header, they need real help. The spec file is not a
regular source code file. That comment will actually get embedded into
the %prep script because everything between the %prep and the next
section identifier in the spec file is considered part of the %prep
script.
%configure %{configure_options} %{ppc64_configure_options}
--enable-ibmgtsim
OK, so you went through the trouble to make the spec file horribly ugly
with that option parsing stuff for enabling the ibmgtsim, then just
hardcoded enabling it? Let's also jump back and take a look at
something:
# Add ppc64 64 bit compile flages
%ifarch ppc64
%define ppc64_configure_options CFLAGS='-m64 -g -O2' CPPFLAGS='-m64 -g
-O2' LDFL
AGS='-m64 -g -O2 -L/usr/lib64/'
%else
%define ppc64_configure_options %{nil}
%endif
In this you are completely wiping out the default CFLAGS et. al. It
would be far preferable for you to append -m64 to the existing CFLAGS
than it is to reset them entirely. However, the %build script is run as
a standalone script, so if you attempt to set the actual CFLAGS
environment variable earlier in the spec file, it won't get passed on to
the %build script. Part of the %build macro itself includes startup
scripting that sets the default CFLAGS environment variable. So, you
would really want something like:
%build
%ifarch ppc64
CFLAGS="$$CFLAGS -m64"
%configure CFLAGS="$$CFLAGS"
(you need the double $ in front of CFLAGS to get past the rpm option
parsing and down to shell environment variable parsing)
# W/A for libtool issue: change libdir in all *.la files to point to
${RPM_BUILD
_ROOT}/${libdir}
# This W/A should be removed in post install section
if [ -d ${RPM_BUILD_ROOT}/%{_prefix} ]; then
LA_FILES=$(find ${RPM_BUILD_ROOT}/%{_prefix} -type f -name '*.la')
for la_file in ${LA_FILES}
do
case ${la_file##*/} in
libibumad.la|libosmcomp.la|libopensm.la|libosmvendor.la)
perl -ni -e "s@(libdir=).*@\$1'${RPM_BUILD_ROOT}%{_libdir}'@;
print" $
{la_file}
perl -ni -e "s@ %{_libdir}@\ ${RPM_BUILD_ROOT}%{_libdir}@g;
print" ${l
a_file}
;;
esac
done
fi
No, this isn't a workaround for libtool, it's a work around for building
against opensm-libs that haven't been installed yet. Will absolutely
get yanked from anything we do here. Use the BuildRequires: line in the
spec file and install the opensm-devel package before building this one.
install -d $RPM_BUILD_ROOT/etc/profile.d
cat > $RPM_BUILD_ROOT/etc/profile.d/ibutils.sh << EOF
if ! echo \${PATH} | grep -q %{_prefix}/bin ; then
PATH=\${PATH}:%{_prefix}/bin
fi
EOF
cat > $RPM_BUILD_ROOT/etc/profile.d/ibutils.csh << EOF
if ( "\${path}" !~ *%{_prefix}/bin* ) then
set path = ( \$path %{_prefix}/bin )
endif
EOF
Writing files from within the spec file is generally frowned upon, but
overlooked on small enough files. However, just below this section you
have this:
case %{_prefix} in
/usr | /usr/)
;;
*)
echo "/etc/ld.so.conf.d/ibutils.conf" >> ibutils-files
;;
esac
Since the two shell environment files are not necessary when the _prefix
is /usr, realistically, both the shell files and the ibutils.conf file
should be under the case statement above and all three files should be
added to ibutils-files if they are actually needed. You should also
only write the files under the case statement above so you don't have
random files in the build root that aren't needed.
%clean
#Remove installed driver after rpm build finished
rm -rf $RPM_BUILD_DIR/%{name}-%{version}
rm -rf $RPM_BUILD_ROOT
###
### pre section
###
%pre
As mentioned before, this comment is now part of the %clean script.
%pre
###
### post section
###
%post
And this comment is now the entire %pre script that is stored in the
actual rpm database after package installation. If you aren't using a
section, such as %pre, then don't include it at all.
%post
if [ $1 = 1 ]; then # 1 : This package is being installed for the first
time
/sbin/ldconfig
fi
Don't check to see if this is the first install in order to run
ldconfig, run ldconfig unconditionally. It's entirely possible that an
upgrade from ibutils-1.2 to ibutils-1.3 could update the minor version
of the installed library and then ldconfig would have a stale cache.
# %{_libdir}/ibibdmcom.a
%{_libdir}/libibdm.a
Why some .a files and not others? Of course, if you are going to
include a devel environment it should really be in a separate -devel sub
package, and .a files should generally be in a separate -static package
(not -devel-static BTW, devel is implied with -static), but that still
leaves the issue of why some and not others.
%{_prefix}/include/ibdm
Should be %{_includedir}/ibdm
%{_libdir}/ibis1.2
%{_libdir}/ibdm1.2
%{_libdir}/ibdiagnet1.2
%{_libdir}/ibdiagpath1.2
%{_libdir}/ibdiagui1.2
%{_prefix}/include/ibdm
These are all directories that are new. In order to declare that a
package owns the directory itself, and not just all the files under that
directory, you also need a %dir statement for each of these. This
results in rpm trying to remove the directory itself if you remove the
package. So you end up with:
%dir %{_libdir}/ibis1.2
%{_libdir}/ibis1.2
for each of the above.
More of the same issues in the remaining file list.
OK, that covers this spec file. And in case you think I'm just being
nit picky, these are the exact sorts of things that I had to fix before
release engineering would let me submit the original openib package into
our build tree, and fixing all of this stuff each and every release is
why I don't use your spec files and instead just update my own spec file
to use the new sources (well, that and history loss), which is of course
why I really want downloadable tarballs as opposed to trying to munge
tarballs out of your distribution. At least with this release, if
nothing else, I can pull separate package tarballs out of the srpms in
the OFED tarball, which is *much* better than in the past with the big
ofa_user stuff. But, I still really need a download link to put in the
spec files or else I get yelled at.
--
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/general/attachments/20071025/718d41d2/attachment.sig>
More information about the general
mailing list