https://bugzilla.redhat.com/show_bug.cgi?id=1333531 --- Comment #3 from Michal Schmidt <mschmidt@xxxxxxxxxx> --- 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. > Url: http://www.intel.com/ 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@xxxxxxxxx> - 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 _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx http://lists.fedoraproject.org/admin/lists/package-review@xxxxxxxxxxxxxxxxxxxxxxx