https://bugzilla.redhat.com/show_bug.cgi?id=1333531
--- Comment #3 from Michal Schmidt <mschmidt(a)redhat.com> ---
I'll comment on some issues I spotted.
Note that some of the issues appear multiple times in the spec file, but I'll
point them out only the first time I spot them.
Name: opa
The review title says "opa-ff", the spec file's name says
"fast-fabric",
the Name tag says "opa". These need to be consistent.
My preferred choice would be "opa-ff", because that's what we called the
source
package in RHEL 7.2.
Version: 10.0.1.0
Release: 2
%{?dist} tag is missing in Release.
Summary: Intel Omni-Path basic tools and libraries for fabric
managment.
Typo. "managment" -> "management"
There should be no dot at the end of Summary.
Group: System Environment/Libraries
Group tag is unnecessary.
License: GPLv2/BSD
Is this dual-licensing of the whole work (where the recipient may to choose to
follow GPLv2 or BSD), or does it mean that parts of the whole are licensed
under GPLv2 and other parts under BSD?
Depending on which case it is, the slash has to be replaced with "and" or
"or"
as per the Licensing Guidelines.
The URL should point to a page that's more relevant to the package. For
instance,
opa-ff's page on github.
Source: opa.tgz
Do you (in your role as an upstream maintainer of opa-ff) publish official
opa-ff release tarballs?
If yes:
The Source tag should be a full URL to the tarball.
Otherwise:
You (in your role as the Fedora packager of opa-ff) need to add a comment
describing reproducible steps how to create the tarball.
BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u}
-n)
Remove the BuildRoot specification. It's been superfluous since Fedora 12-ish /
RHEL6.
ExclusiveArch: x86_64
A one-line comment explaining the reason for using ExclusiveArch would be nice.
%if 0%{?suse_version} >= 1110
%debug_package
%endif
Conditional blocks for other distros should not be used in Fedora spec files.
(RHEL/EPEL conditionals are tolerated, but personally I don't like those
either. There's a reason spec files are kept in git branches per Fedora/EPEL
release.)
%description
Basic package
Not a good package description.
> %package basic-tools
> Summary: Managment level tools and scripts.
Group: System Environment/Libraries
> AutoReq: no
The Packaging Guidelines have a section on "Filtering Auto-Generated Requires",
which must be followed instead of using AutoReq.
Requires: rdma
%if 0%{?rhel} || 0%{?fedora}
Requires(post): expat, libibmad, libibumad, libibverbs, expect, tcl, openssl
You don't have a %post scriptlet. You should not need "Requires(post)".
BuildRequires: expat-devel, gcc-c++, openssl-devel, ncurses-devel,
tcl-devel, libibumad-devel, libibverbs-devel, libibmad-devel
BuildRequires are per source package, not per subpackage.
> %else
> Requires(post): libexpat1, libibmad5, libibumad, libibverbs1, openssl
> BuildRequires: libexpat-devel, gcc-c++, openssl-devel, ncurses-devel, tcl-devel,
libibumad-devel, libibverbs-devel, libibmad-devel
> %endif
>
> %description basic-tools
> Contains basic tools for fabric managment necessary on all compute nodes.
>
> %package fastfabric
> Summary: Management level tools and scripts.
Group: System Environment/Libraries
> AutoReq: no
> Requires: opa-basic-tools
Explicit Requires between subpackages of the same source package must be fully
versioned and arch-specific.
%if 0%{?rhel}
Requires: atlas
%endif
This looks fishy. Why is this dependency RHEL-specific?
> %description fastfabric
> Contains tools for managing fabric on a managment node.
>
> %package address-resolution
> Summary: Contains Address Resolution manager
Group: System Environment/Libraries
> AutoReq: no
> BuildRequires: ibacm-devel, %{?BuildRequires}
> Requires: opa-basic-tools
>
> %description address-resolution
> This is to be filled out more concisely later.
>
> %prep
> #rm -rf %{_builddir}/*
> #tar xzf %_sourcedir/%name.tgz
> %setup -q -c
You're using "-c" here as a workaround for the fact that the tarball does
not
contain a single top-level directory. Better improve the tarball style.
%build
if [ -d OpenIb_Host ]
As a packager, you should know whether the tarball contains this directory or
not. No such conditional should be necessary.
then
cd OpenIb_Host && ./ff_build.sh %{_builddir} $FF_BUILD_ARGS
else
./ff_build.sh %{_builddir} $FF_BUILD_ARGS
fi
%install
[... cut 167 lines ...]
Most of the knowledge about installing the built files does not belong in
the spec file. As the upstream developer of opa-ff, would you please consider
moving the install steps into your build scripts / Makefiles / whatever it is
that opa-ff uses for building?
(Think about how "make", "make install" work in most open source
programs.)
%clean
rm -rf $RPM_BUILD_ROOT
The %clean section should be removed.
%post address-resolution -p /sbin/ldconfig
%postun address-resolution -p /sbin/ldconfig
%preun fastfabric
cd /opt/opa/src/mpi_apps >/dev/null 2>&1
make -k clean >/dev/null 2>&1 || : # suppress all errors and return codes from
the make clean.
Very unusual to perform this kind of action in a scriptlet.
%files basic-tools -f %{_builddir}/basic_file.list
I strongly dislike this use of a generated file list. It's not idiomatic and
makes it hard to review what the packages contain.
%defattr(-,root,root,-)
%defattr is unnecessary.
> %files fastfabric -f %{_builddir}/ff_file.list
%defattr(-,root,root,-)
>
%{_sysconfdir}/sysconfig/opa/opamon.si.conf
A conf file without %config(noreplace). Is this intentional?
/etc/sysconfig is a Fedora-ism. In the interest of minimizing cross-distro
differences, would you (in your opa-ff upstream developer role) please consider
avoiding it? Why not just /etc/opa?
%config(noreplace) %{_sysconfdir}/sysconfig/opa/opafastfabric.conf
%config(noreplace) %{_sysconfdir}/sysconfig/opa/opamon.conf
%config(noreplace) %{_sysconfdir}/sysconfig/opa/allhosts
%config(noreplace) %{_sysconfdir}/sysconfig/opa/chassis
%config(noreplace) %{_sysconfdir}/sysconfig/opa/esm_chassis
%config(noreplace) %{_sysconfdir}/sysconfig/opa/hosts
%config(noreplace) %{_sysconfdir}/sysconfig/opa/ports
%config(noreplace) %{_sysconfdir}/sysconfig/opa/switches
%config(noreplace) %{_sysconfdir}/sysconfig/opa/opaff.xml
%config(noreplace) /opt/opa/tools/osid_wrapper
Fedora packages must not install files under /opt.
Also, a %config file under any other location than /etc is wrong.
> %{_sbindir}/opafmconfigcheck
> %{_sbindir}/opafmconfigdiff
>
>
> %files address-resolution
%defattr(-,root,root,-)
> #Everything under the bin
directory belongs exclusively to opasadb at this time.
I wonder what "opasadb" is.
%{_bindir}/*
%{_libdir}/*
%{_includedir}/*
/usr/share/man/man1/opa_osd_dump.1*
/usr/share/man/man1/opa_osd_exercise.1*
/usr/share/man/man1/opa_osd_perf.1*
/usr/share/man/man1/opa_osd_query.1*
Use %{_mandir}.
%config(noreplace) %{_sysconfdir}/rdma/dsap.conf
%changelog
* Fri Oct 10 2014 Erik E. Kahn <erik.kahn(a)intel.com> - 1.0.0-ifs
- Initial version
The changelog entry does not match the actual package version.
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component