Spec URL: https://github.com/01org/opa-ff/releases/download/v1.0/fast-fabric.spec SRPM URL: https://github.com/01org/opa-ff/releases/download/v1.0/opa-10.0.1.0-2.src.rpm Description: The OPA Fast Fabric toolset comprises user-facing command line and TUI tools to enable fabric initialization, configuration, administration, and maintenance. Fedora Account System Username: sbreyer
Hello Scott, is this your first Fedora package? Please read the Fedora packaging guidelines: http://fedoraproject.org/wiki/Packaging:Guidelines For reference, here's how opa-ff is packaged in RHEL 7.2: https://git.centos.org/commit/rpms!opa-ff/f827710d59192fa7b32609b4439c785064a6efe2 See the spec file there. It's still far from perfect, but it has some packaging issues already fixed. I'll write down more specific comments later.
Hi Michal, Yes it's my first. I'll check the guidelines and take a look at the reference, thanks for the feedback. Scott
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> - 1.0.0-ifs > - Initial version The changelog entry does not match the actual package version.
Updated Spec and SRPM addressing review comments and recommendations: Spec URL: https://github.com/01org/opa-ff/releases/download/v1.1/opa-ff.spec SRPM URL: https://github.com/01org/opa-ff/releases/download/v1.1/opa-10.1.0.0-126.fc23.src.rpm
(In reply to Erik E. Kahn from comment #4) I see you made some changes, but most of the issues I mentioned in comment #3 are still present. Looking at the updated opa-ff.spec: > Name: opa Still inconsistent with spec file name. Or is the spec file to be called "opa.spec" when imported to Fedora? > Summary: Intel Omni-Path basic tools and libraries for fabric managment. Still with the typo and the dot. > # tarball created by: > # git clone https://github.com/01org/opa-ff.git > # cd opa-ff > # tar czf opa-ff.tar.gz --exclude-vcs . > Source: opa.tgz These steps to recreate the tarball are not deterministic. If I repeat them a year from now, I'd get a tarball with different contents. The steps should identify the exact git commit. > %description > This package contains the tools necessary to manage an Intel(R) Omni-Path > Architecture fabric. Please do not use the "(R)" symbol in %description. See http://fedoraproject.org/wiki/Packaging:Guidelines#Trademarks_in_Summary_or_Description > BuildRequires: ibacm-devel, %{?BuildRequires} What is the purpose of the %{?BuildRequires} macro? > Requires: opa-basic-tools Explicit Requires between subpackages of the same source package must be fully versioned and arch-specific. > %setup -q -c Still using -c. So... is there no chance of improving the tarball creation steps to have a top-level directory? > if [ -d OpenIb_Host ] Again, why? As a packager, you should know whether the tarball contains this directory or not. No such conditional should be necessary. The %install part still contains a long script. Oh well, it's unfortunate, but not a blocking issue. Still uses generated file lists. I strongly dislike them. It's not idiomatic and makes it hard to review what the packages contain. Still uses /etc/sysconfig (not a blocker, but a bad choice, IMO). Still installs files under /opt. BLOCKER. > %changelog > * Thu Jun 2 2016 Scott Breyer <scott.j.breyer> - 10.1.0-ifs > - Update to latest from build 10.1.0.0.145 (FF 10.1.0.0.126) The changelog entry again does not match the actual package version. And you deleted the previous changelog entry.
Beside Michal's comments about the spec file. Please address follow issues: 1 Unix and Dos format files. https://github.com/01org/opa-ff/issues/2 opa-ff$ find -type f | xargs -n 1 file | grep CRLF | awk -F: '{print $1}' | cat -n 2 Please remove executable permission bits for C source files. opa-ff$ find -type f -executable -name '*.[ch]' 3) Please apply this patch. It fixes a building failure. diff --git a/HSM_Lib/ib_library/fm_xml.c b/HSM_Lib/ib_library/fm_xml.c index 1ad794d..e3ea5e6 100644 --- a/HSM_Lib/ib_library/fm_xml.c +++ b/HSM_Lib/ib_library/fm_xml.c @@ -3388,7 +3388,7 @@ boolean addDefaultVirtualFabric(uint32_t fm, FMXmlCompositeConfig_t *config, VFX vfp->number_of_limited_members = 0; vfp->number_of_applications = 1; - snprintf(vfp->application[0].application, sizeof(vfp->application[0].application), app->name); + snprintf(vfp->application[0].application, sizeof(vfp->application[0].application), "%s", app->name); if (!addMap(&app_config->appMap, XML_QMAP_U64_CAST app)) { freeApplicationObject(app); 4) Please replace '#!/usr/bin/env expect' with '#!/usr/bin/expect -f'. https://github.com/01org/opa-ff/pull/1 5) hardcoded compiler flags sed -i -r -e 's/(release_C(C)?OPT_Flags\s*)=/\1?=/' Makerules/Target.LINUX.GNU.* Thanks
We've addressed the issue pointed out with /opt usage, along with other issues pointed out by the review: Spec URL: https://github.com/01org/opa-ff/releases/download/v1.2/opa.spec SRPM URL: https://github.com/01org/opa-ff/releases/download/v1.2/opa-10.1.0.0-126.fc23.src.rpm
Created attachment 1239419 [details] proposed standalone install script The spec file would be a lot simpler if the install steps were separated in a standalone ff_install.sh script, like the attached one (untested). The %install step in the spec would then be just one line: BUILDDIR=%{_builddir} DESTDIR=%{buildroot} LIBDIR=%{_libdir} ./ff_install.sh And the %files sections would be adjusted to not rely on generated file lists. Would you please consider adding such an install script?
(In reply to Michal Schmidt from comment #8) > Created attachment 1239419 [details] > proposed standalone install script > > The spec file would be a lot simpler if the install steps were separated in > a standalone ff_install.sh script, like the attached one (untested). > > The %install step in the spec would then be just one line: > BUILDDIR=%{_builddir} DESTDIR=%{buildroot} LIBDIR=%{_libdir} ./ff_install.sh > > And the %files sections would be adjusted to not rely on generated file > lists. > > Would you please consider adding such an install script? Hi Michal, Thank you for this suggestion. We will plan on working this into an upcoming update to our repositories. I apologize for the delay in this response. Scott
We've made changes to our spec file that should be in line with what you are asking. The patch is presently making its way through our internal review and test process.
New tag/sha has been pushed to v10_3_1 branch of 01org/opa-ff: tag v10.3.1.0.8: 282ce60
So I checked out the tag, looking for a spec file. $ find . -name '*.spec' ./Dsap/dsap.spec ./opasadb/opasadb.spec ./OpenIb_Host/mpi-apps.spec What are these spec files? Are they distinct packages that we need to add to Fedora (and RHEL) too? Where is the spec file for this review? I found a template opa-ff.spec.in. And there is a script to fill it in. Let's run it: $ ./update_opa-ff.spec.sh opa-ff.spec.in opa.spec That did not work. opa.spec is 100% identical to opa-ff.spec.in, so it's not a syntactically valid spec file ("error: line 13: Unknown tag: __RPM_DEBUG"). Looking into update_opa-ff.spec.sh... I see it assumes I'm using either RHEL or SLES. I am using neither, so let's hack it: diff --git a/update_opa-ff.spec.sh b/update_opa-ff.spec.sh index d1da2a2ef6..8cc3a594f7 100755 --- a/update_opa-ff.spec.sh +++ b/update_opa-ff.spec.sh @@ -37,7 +37,7 @@ then exit 1 fi -id=$(grep ^ID= /etc/os-release | cut -f2 -d\") +id=rhel #$(grep ^ID= /etc/os-release | cut -f2 -d\") versionid=$(grep ^VERSION_ID= /etc/os-release | cut -f2 -d\") from=$1 ...and try again: $ ./update_opa-ff.spec.sh opa-ff.spec.in opa.spec This is the result: Spec URL: https://michich.fedorapeople.org/opa-ff/opa.spec I see you added ff_install.sh and the spec file is now shorter. Thank you! Unfortunately it still uses generated file lists. In mentioned that as an issue in comments #3, #5, and #8. I don't know if you missed those comments, or you simply insist on the use of file lists. Please comment. There are several items that go against the Packaging Guidelines. What's worse is that some of them were already fixed in the previously reviewed spec file from comment #7 and now they're back, so we're regressing. > Summary: Intel Omni-Path basic tools and libraries for fabric managment. Typo. "managment" -> "management" There should be no dot at the end of the Summary. See http://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections > Group: System Environment/Libraries In comment #3 I wrote the Group tag was unnecessary. Since then the Fedora packaging guidelines got updated. The wording is now stronger: The [...] Group: tag, [...] SHOULD NOT be used. (http://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections) Please remove the Group tags. > License: GPLv2/BSD The correct syntax is: License: GPLv2 or BSD See https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Dual_Licensing_Scenarios > Url: http://www.intel.com/ Repeating from comment #3: The URL should point to a page that's more relevant to the package. For instance, opa-ff's page on github. The previously reviewed spec file had: Url: https://github.com/01org/opa-ff > Source: opa.tgz The previously reviewed spec file included a comment on how to reproduce the tarball. The comment is now gone! > %description > This package contains the tools necessary to manage an Intel(R) > Omni-Path Architecture fabric. The "(R)" symbol must not be used in %description. See http://fedoraproject.org/wiki/Packaging:Guidelines#Trademarks_in_Summary_or_Description > Requires: opa-basic-tools Explicit Requires between subpackages of the same source package should be fully versioned and arch-specific: Requires: opa-basic-tools%{?_isa} = %{version}-%{release} See http://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package > Requires: atlas What is the reason for requiring atlas explicitly? Does the software dlopen() lib[st]atlas.so.3? I did not find anything by grepping. > %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. Just a minor issue. > %preun fastfabric > cd /usr/lib/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. Could you explain the purpose of this scriptlet? And perhaps more generally, the intended purpose of the /usr/lib/opa/src/mpi_apps directory?
When creating a SRPM I got a warning: warning: bogus date in %changelog: Fri Feb 23 2017 Feb 23 was a Thursday. When building it I get a lot of errors: /usr/bin/perl: bad interpreter: No such file or directory This needs to be added: BuildRequires: perl
The package does not build in Rawhide due to API changes in OpenSSL 1.1. For example: src/fe_ssl.c: In function 'fe_ssl_x509_verify_callback': src/fe_ssl.c:116:62: error: dereferencing pointer to incomplete type 'X509_STORE_CTX {aka struct x509_store_ctx_st}' char *str = (char *)X509_verify_cert_error_string(ctx->error); I guess X509_STORE_CTX_get_error() needs to be used instead of accessing ctx's fields directly. This page might be helpful: https://wiki.openssl.org/index.php/1.1_API_Changes
>> Unfortunately it still uses generated file lists. In mentioned that as an issue in comments #3, #5, and #8. I don't know if you missed those comments, or you simply insist on the use of file lists. Please comment. << This spec file is producing several packages, so I think I need the generated file lists in order to specify which files go with which packages. I apologize for the reqressions and will address them.
(In reply to Scott Breyer from comment #15) > >> > Unfortunately it still uses generated file lists. In mentioned that as an > issue in comments #3, #5, and #8. I don't know if you missed those comments, > or you simply insist on the use of file lists. Please comment. > << > > This spec file is producing several packages, so I think I need the > generated file lists in order to specify which files go with which packages. Unless I misunderstand what you are saying here, the answer is no, you don't need generated file lists to deal with multiple packages. Static file lists work just fine: %files <set of file regexps> %files devel <set of files to go into -devel subpackage> etc.
I understand. Although it is a problem in that the file lists are bulky, and, due to the naming conventions of the files, it will be hard to separate then into regexps that make sense.
New tag/sha has been pushed to v10_3_1-spec-rework temporary branch of 01org/opa-ff for your review: tag v10.3.1.0.9-pre : e3d16de
This is an automatic check from review-stats script. This review request ticket hasn't been updated for some time, but it seems that the review is still being working out by you. If this is right, please respond to this comment clearing the NEEDINFO flag and try to reach out the submitter to proceed with the review. If you're not interested in reviewing this ticket anymore, please clear the fedora-review flag and reset the assignee, so that a new reviewer can take this ticket. Without any reply, this request will shortly be resetted.
This is an automatic action taken by review-stats script. The ticket reviewer failed to clear the NEEDINFO flag in a month. As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews we reset the status and the assignee of this ticket.
This is an automatic action taken by review-stats script. The ticket submitter failed to clear the NEEDINFO flag in a month. As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews we consider this ticket as DEADREVIEW and proceed to close it.