https://bugzilla.redhat.com/show_bug.cgi?id=1324590 Michal Schmidt <mschmidt@xxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Assignee|nobody@xxxxxxxxxxxxxxxxx |mschmidt@xxxxxxxxxx Flags| |fedora-review? --- Comment #2 from Michal Schmidt <mschmidt@xxxxxxxxxx> --- (In reply to paul.j.reger from comment #0) > Spec URL: https://github.com/01org/opa-psm2/releases/tag/10_1 > SRPM URL: https://github.com/01org/opa-psm2/releases/tag/10_1 To facilitate the review process (notably the 'fedora-review' helper tool), the two URLs should point directly to the spec file and SRPM, not to a download page. > Description: The PSM Messaging API, or PSM API, is the low-level > user-level communications interface for the Intel(R) OPA > family of products. PSM users are enabled with mechanisms > necessary to implement higher level communications > interfaces in parallel environments. Just curious: Is "PSM" an acronym? "Parallel ...something.. Messaging"? > Fedora Account System Username: pjreger Are you already a member of the 'packager' group in Fedora? Looking at the spec file: > # > # This file is provided under a dual BSD/GPLv2 license. When using or > # redistributing this file, you may do so under either license. > # > # GPL LICENSE SUMMARY > [...] > # BSD LICENSE > [...] > # Copyright (c) 2014-2015 Intel Corporation. All rights reserved. > # It's OK to have a copyright and license header in a spec file in Fedora, though it is uncommon. Most packagers rely on the default (MIT) license specified by the Fedora Project Contributor Agreement (FPCA): https://fedoraproject.org/wiki/Licensing:Main?rd=Licensing#License_of_Fedora_SPEC_Files > Summary: Intel PSM Libraries > Name: hfi1-psm > Version: 0.7 > Release: 13 > License: GPLv2 Seems it should say more precisely: BSD or GPLv2 https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Dual_Licensing_Scenarios > Group: System Environment/Libraries The Group tag is unnecessary. http://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections > URL: http://www.intel.com/ Please use a URL that's more specific to hfi1-psm. Maybe URL: https://github.com/01org/opa-psm2/ > Source0: %{name}-%{version}-%{release}.tar.gz Could you please use a full URL in the Source tag? http://fedoraproject.org/wiki/Packaging:SourceURL > Prefix: /usr Please drop this tag. http://fedoraproject.org/wiki/Packaging:Guidelines#Relocatable_packages > BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root Don't specify BuildRoot. http://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections > Requires(post): /sbin/ldconfig > Requires(postun): /sbin/ldconfig It should not be necessary to specify those since you are using the "%post -p /sbin/ldconfig" form to call ldconfig. In this case rpmbuild is supposed to detect the dependency automatically. http://fedoraproject.org/wiki/Packaging:Scriptlets#Shared_libraries > ExclusiveArch: x86_64 A short comment explaining the reason for ExclusiveArch would be nice. > %if 0%{?rhel}%{?rhl}%{?fedora} > Requires: libuuid > %else > Requires: libuuid1 > %endif The binary package will automatically get a dependency on "libuuid.so.1()(64bit)", so there is no need for an explicit Requires. http://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires > BuildRequires: libuuid-devel > Conflicts: opa-libs What is the purpose of this Conflicts tag? What is "opa-libs"? It's not a package in Fedora. http://fedoraproject.org/wiki/Packaging:Guidelines#Conflicts > Obsoletes: hfi-psm > Obsoletes: hfi-psm-debuginfo Unversioned Obsoletes are dangerous. What is the purpose of these Obsoletes? There never was a "hfi-psm" package in Fedora, was it? > %package devel > Summary: Development files for Intel PSM > Group: System Environment/Development See a previous comment for Group tag. > Requires: %{name} = %{version}-%{release} Better add %{?_isa} too, even though the package is exclusive to one arch. http://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package > Requires(post): /sbin/ldconfig > Requires(postun): /sbin/ldconfig I think only the main package needs to run ldconfig. > Requires: libuuid-devel > Conflicts: opa-devel > Obsoletes: hfi-psm-devel See previous comments about Conflicts and Obsoletes. > %package compat > Summary: Development files for Intel PSM Better adjust the Summary for -compat to make it different from -devel. > Group: System Environment/Development > Requires: %{name} = %{version}-%{release} > Requires(post): /sbin/ldconfig > Requires(postun): /sbin/ldconfig > Obsoletes: hfi-psm-compat See previous comments. > %description > The PSM Messaging API, or PSM API, is the low-level > user-level communications interface for the Intel(R) OPA > family of products. PSM users are enabled with mechanisms > necessary to implement higher level communications > interfaces in parallel environments. > > %description devel > Development files for the libpsm2 library This may be a little confusing, because "libpsm2" does not appear in the main package's name or description. > %description compat > Support for MPIs linked with PSM versions < 2 > > %prep > %setup -q -n hfi1-psm-%{version}-%{release} > > %build > %{__make} Personally I don't see the point of using this kind of a macro as opposed to just calling "make". More importantly, you need to make sure to follow the compiler flags specified by the system rpm configuration. http://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags > %install > rm -rf $RPM_BUILD_ROOT Don't clean the buildroot explicitly. http://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections > mkdir -p $RPM_BUILD_ROOT > export DESTDIR=$RPM_BUILD_ROOT Are these two lines really necessary? > %{__make} DESTDIR=$RPM_BUILD_ROOT install > > %clean > rm -rf $RPM_BUILD_ROOT %clean section is unnecessary. http://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections > %post -p /sbin/ldconfig > %postun -p /sbin/ldconfig > %post devel -p /sbin/ldconfig > %postun devel -p /sbin/ldconfig Already mentioned I don't think -devel needs to run ldconfig. > %files > %defattr(-,root,root,-) %defattr is not needed. http://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions > /usr/lib64/libpsm2.so.2.1 > /usr/lib64/libpsm2.so.2 Should use %{_libdir}. > /usr/lib/udev/rules.d/40-psm.rules Should use %_udevrulesdir (and hence "BuildRequires: systemd", because that's where the macro is defined). > %files devel > %defattr(-,root,root,-) > /usr/lib64/libpsm2.so > /usr/include/psm2.h > /usr/include/psm2_mq.h > /usr/include/psm2_am.h Use %{_includedir} > # The following files were part of the devel-noship and moved to devel: > /usr/include/hfi1diag/ptl_ips/ipserror.h > /usr/include/hfi1diag/linux-x86_64/bit_ops.h > /usr/include/hfi1diag/linux-x86_64/sysdep.h > /usr/include/hfi1diag/opa_udebug.h > /usr/include/hfi1diag/opa_debug.h > /usr/include/hfi1diag/opa_intf.h > /usr/include/hfi1diag/opa_user.h > /usr/include/hfi1diag/opa_service.h > /usr/include/hfi1diag/opa_common.h > /usr/include/hfi1diag/opa_byteorder.h > /usr/include/hfi1diag/hfi1_deprecated.h Looks like you forgot to own the "%{_includedir}/hfi1diag" directory itself. > %files compat > %defattr(-,root,root,-) > /usr/lib64/psm2-compat/libpsm_infinipath.so.1 > /usr/lib/udev/rules.d/40-psm-compat.rules > /etc/modprobe.d/hfi1-psm-compat.conf /usr/lib/modprobe.d is preferred to /etc. /etc is administrator's territory. I don't think there's a macro for that, unfortunately. So I guess this'll have to do: %{_prefix}/lib/modprobe.d/hfi1-psm-compat.conf > /usr/sbin/hfi1-psm-compat.cmds Use %{_sbindir} > %changelog > * Tue Apr 05 2016 -0700- > - Upstream PSM2 source code for Fedora. The changelog is not formatted properly. http://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs -- 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