Spec URL: https://www.openfabrics.org/downloads/libi40iw/ SRPM URL: https://www.openfabrics.org/downloads/libi40iw/ Description: libi40iw provides a device-specific RDMA userspace library for Intel Ethernet Connection X722 for use with the libibverbs library. Fedora Account System Username: tatyana Git URL: http://git.openfabrics.org/?p=~tnikolova/libi40iw/.git
Okay, I've done an initial pass through the spec, and a few things of note to take care of: 1) There's mixed use of both $RPM_BUILD_ROOT and %{buildroot}. These two are equivalent, but it's preferred that you only use one or the other consistently through the spec. 2) The COPYING file these days needs to be listed in the spec file under %license, rather than as %doc. 3) Formatting on the changelog entry is slightly off, there should be a space between the initial "*" and the date entry, and the version at the tail end of the line should be version-release, i.e. "0.5.223-1". 4) Fedora guidelines complain about unversioned library files dropped straight into %libdir, though putting a .a file into a -devel-static package seems to be the norm for all rdma hardware-specific libibverbs-based driver libs like this, so I think we can leave that as-is. 5) the Source: url to the tarball is a bit off, it says ./downloads/i40iw/., but it should be ./downloads/libi40iw/. instead. 6) the file dropped under libibverbs.d/ should probably be marked at %config, as you can edit the file to alter behavior, and don't want that overwritten by a package update. I've looked at another libibverbs driver or two, and they're done as %config as well. 7) The spec says license GPL/BSD, and looking at the various sources, that seems accurate, as I see both GPLv2 and GPLv3, and various BSD license text in the source. However, the COPYING file looks to only mention GPLv2. May need multiple license files here to cover all cases. Also, rpmlint complains that "GPL/BSD" isn't a valid value for the License: field. Looking at https://fedoraproject.org/wiki/Packaging:LicensingGuidelines I think "GPLv2+ and BSD" or "GPLv2+ or BSD" might be appropriate. 8) BuildRequires on libibverbs-devel are not versioned, which may well be just fine, but I'd like to double-check that there isn't some minimal version required for proper libi40iw support. (I seem to recall the hfi1 bits requiring libibverbs 1.3.0 or later, thus why I ask). 9) BuildRoot doesn't actually need to be explicitly defined, and the preference of late is actually that we let RPM internals figure it out, so the BuildRoot: ... line could actually be removed from the spec. Not a hard requirement though, particularly if looking to support much older versions of RPM in older distributions with the same spec. Most of this is pretty trivial to fix up, but the one that really needs the most effort is probably the license bit, to make sure we can get through legal with the proper licensing documented here.
Some further reading on the licensing issue and a bit of back-channel discussion, and what I now believe to be the thing to do for #7 is "License: GPLv2 or BSD", rename COPYING to LICENSE-GPL and include a new LICENSE-BSD file so the text of both licenses is easily distributable with the binaries. The only GPLv3+ files are config/config.*, which aren't part of the distributed binaries, and License: is only in reference to distributed binaries, so no reference to GPLv3+ is required here.
Just let me know when there's an updated package I can look over, I think we should be pretty close once the initial review pass notes are addressed.
(In reply to Jarod Wilson from comment #4) > Just let me know when there's an updated package I can look over, I think we > should be pretty close once the initial review pass notes are addressed. The package is updated. The new version libi40iw-0.5.224.tar.gz is available at the URL provided above. All the changes can be viewed in the public git repository.
Using fedora-review's template, with notes added after anything marked as !/fail: Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed Issues: ======= - Development (unversioned) .so files in -devel subpackage, if present. Note: Unversioned so-files directly in %_libdir. See: http://fedoraproject.org/wiki/Packaging/Guidelines#DevelPackages Waiving this issue, as this is the norm for libibverbs plugins. ===== MUST items ===== C/C++: [x]: Package does not contain kernel modules. [x]: Package contains no static executables. [x]: Header files in -devel subpackage, if present. [x]: Package does not contain any libtool archives (.la) [x]: Rpath absent or only used for internal libs. Generic: [x]: Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [x]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "MIT/X11 (BSD like)", "GPL (v2 or later)", "Unknown or generated". 16 files have unknown license. Detailed output of licensecheck in /home/jwilson/review-libi40iw/licensecheck.txt [x]: License file installed when any subpackage combination is installed. [x]: If the package is under multiple licenses, the licensing breakdown must be documented in the spec. [x]: %build honors applicable compiler flags or justifies otherwise. [x]: Package contains no bundled libraries without FPC exception. [!]: Changelog in prescribed format. There should be a blank line between changelog entries [!]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. Note: rm -rf %{buildroot} present but not required Present in the spec, which is apparently no longer okay. This is actually news to me. [x]: Sources contain only permissible code or content. [!]: %config files are marked noreplace or the reason is justified. Note: No (noreplace) in %config /etc/libibverbs.d/i40iw.driver I missed that this maybe should be marked %config(noreplace), but will also accept justification for why it shouldn't be. If it isn't, i40iw.driver modified by the user becomes i40iw.driver.rpmorig, I believe. [x]: Each %files section contains %defattr if rpm < 4.4 Note: %defattr present but not needed [-]: Package contains desktop file if it is a GUI application. [-]: Development files must be in a -devel package [x]: Package uses nothing in %doc for runtime. [x]: Package consistently uses macros (instead of hard-coded directory names). [x]: Package is named according to the Package Naming Guidelines. [x]: Package does not generate any conflict. [x]: Package obeys FHS, except libexecdir and /usr/target. [-]: If the package is a rename of another package, proper Obsoletes and Provides are present. [x]: Requires correct, justified where necessary. [x]: Spec file is legible and written in American English. [-]: Package contains systemd file(s) if in need. [x]: Useful -debuginfo package or justification otherwise. [x]: Package is not known to require an ExcludeArch tag. [-]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 10240 bytes in 1 files. [!]: Package complies to the Packaging Guidelines Just a few little things to fix up as noted in this comment. [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [x]: Package installs properly. [x]: Rpmlint is run on all rpms the build produces. Note: There are rpmlint messages (see attachment). [x]: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %license. [x]: Package requires other packages for directories it uses. [x]: Package must own all directories that it creates. [x]: Package does not own files or directories owned by other packages. [x]: All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines. [x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT [x]: Macros in Summary, %description expandable at SRPM build time. [x]: Dist tag is present. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Package use %makeinstall only when make install DESTDIR=... doesn't work. [x]: Package is named using only allowed ASCII characters. [x]: No %config files under /usr. [x]: Package does not use a name that already exists. [x]: Package is not relocatable. [x]: Sources used to build the package match the upstream source, as provided in the spec URL. [x]: Spec file name must match the spec package %{name}, in the format %{name}.spec. [x]: Static libraries in -static or -devel subpackage, providing -devel if present. Note: Package has .a files: libi40iw-devel-static. [x]: File names are valid UTF-8. [x]: Packages must not store files under /srv, /opt or /usr/local ===== SHOULD items ===== Generic: [!]: Buildroot is not present Note: Buildroot: present but not needed Fine to leave it if so desired. [!]: Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) Note: %clean present but not required Fine to leave it if so desired. [x]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [x]: Final provides and requires are sane (see attachments). [!]: Fully versioned dependency in subpackages if applicable. Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in libi40iw-devel-static , libi40iw-debuginfo I'm not entirely familiar with the %{?_isa} bit, but this would be a trivial addition, and won't negatively impact any case where _isa isn't defined, thanks to the ? prefix. [-]: Package functions as described. I don't actually have the hardware to test with myself, but I assume it does work. :) [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [-]: Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [x]: Package should compile and build into binary rpms on all supported architectures. [-]: %check is present and all tests pass. [x]: Packages should try to preserve timestamps of original installed files. [x]: Reviewer should test that the package builds in mock. [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [x]: Uses parallel make %{?_smp_mflags} macro. [x]: Sources can be downloaded from URI in Source: tag [x]: SourceX is a working URL. [x]: Spec use %global instead of %define unless justified. ===== EXTRA items ===== Generic: [x]: Rpmlint is run on debuginfo package(s). Note: No rpmlint messages. [x]: Rpmlint is run on all installed packages. Note: There are rpmlint messages (see attachment). [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. [x]: Spec file according to URL is the same as in SRPM.
[!]: %config files are marked noreplace or the reason is justified. Note: No (noreplace) in %config /etc/libibverbs.d/i40iw.driver The file /etc/libibverbs.d/i40iw.driver contains the driver name to be used by ib verbs and is not intended to be edited by the user. This file stays the same in different library versions (since the driver name is the same). If the file is somehow modified by the user, it can cause issues and it is preferred to be overwritten by the next libi40iw install. We are working on fixes for the other items on the list.
The package is updated. The new version libi40iw-0.5.225.tar.gz is available at the URL provided above. All the changes can be viewed in the public git repository.
(In reply to tatyana from comment #8) > The package is updated. The new version libi40iw-0.5.225.tar.gz is available > at the URL provided above. All the changes can be viewed in the public git > repository. Hm. The prior version built in mock just fine when I ran fedora-review, but this latest version fails with the following error: (CDPATH="${ZSH_VERSION+.}:" && cd . && /bin/sh /builddir/build/BUILD/libi40iw-0.5.225/config/missing autoheader) /builddir/build/BUILD/libi40iw-0.5.225/config/missing: line 81: autoheader: command not found WARNING: 'autoheader' is missing on your system. You should only need it if you modified 'acconfig.h' or 'configure.ac' or m4 files included by 'configure.ac'. The 'autoheader' program is part of the GNU Autoconf package: <http://www.gnu.org/software/autoconf/> It also requires GNU m4 and Perl in order to run: <http://www.gnu.org/software/m4/> <http://www.perl.org/> Makefile:404: recipe for target 'config.h.in' failed make: *** [config.h.in] Error 127
Assuming the auto-generated files got messed up, I rebuilt the package. I didn't change the version and there aren't source changes. Could you let me know if the issue above is resolved.
(In reply to tatyana from comment #10) > Assuming the auto-generated files got messed up, I rebuilt the package. I > didn't change the version and there aren't source changes. Could you let me > know if the issue above is resolved. Yes, thank you, I'm getting through the build just fine now, more detailed review notes forthcoming.
Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed Issues: ======= - Development (unversioned) .so files in -devel subpackage, if present. Note: Unversioned so-files directly in %_libdir. See: http://fedoraproject.org/wiki/Packaging/Guidelines#DevelPackages ===== MUST items ===== C/C++: [x]: Package does not contain kernel modules. [x]: Package contains no static executables. [x]: Header files in -devel subpackage, if present. [x]: Package does not contain any libtool archives (.la) [x]: Rpath absent or only used for internal libs. Generic: [x]: Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [x]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "MIT/X11 (BSD like)", "GPL (v2 or later)", "Unknown or generated". 16 files have unknown license. Detailed output of licensecheck in /home/jwilson/review-libi40iw/licensecheck.txt [x]: License file installed when any subpackage combination is installed. [x]: If the package is under multiple licenses, the licensing breakdown must be documented in the spec. [x]: %build honors applicable compiler flags or justifies otherwise. [x]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [x]: Sources contain only permissible code or content. [x]: %config files are marked noreplace or the reason is justified. Note: No (noreplace) in %config /etc/libibverbs.d/i40iw.driver [x]: Each %files section contains %defattr if rpm < 4.4 Note: %defattr present but not needed [-]: Package contains desktop file if it is a GUI application. [x]: Development files must be in a -devel package [x]: Package uses nothing in %doc for runtime. [x]: Package consistently uses macros (instead of hard-coded directory names). [x]: Package is named according to the Package Naming Guidelines. [x]: Package does not generate any conflict. [x]: Package obeys FHS, except libexecdir and /usr/target. [-]: If the package is a rename of another package, proper Obsoletes and Provides are present. [x]: Requires correct, justified where necessary. [x]: Spec file is legible and written in American English. [-]: Package contains systemd file(s) if in need. [x]: Useful -debuginfo package or justification otherwise. [x]: Package is not known to require an ExcludeArch tag. [-]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 10240 bytes in 1 files. [x]: Package complies to the Packaging Guidelines [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [x]: Package installs properly. [x]: Rpmlint is run on all rpms the build produces. Note: There are rpmlint messages (see attachment). [x]: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %license. [x]: Package requires other packages for directories it uses. [x]: Package must own all directories that it creates. [x]: Package does not own files or directories owned by other packages. [x]: All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines. [x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [x]: Macros in Summary, %description expandable at SRPM build time. [x]: Dist tag is present. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Package use %makeinstall only when make install DESTDIR=... doesn't work. [x]: Package is named using only allowed ASCII characters. [x]: No %config files under /usr. [x]: Package does not use a name that already exists. [x]: Package is not relocatable. [x]: Sources used to build the package match the upstream source, as provided in the spec URL. [x]: Spec file name must match the spec package %{name}, in the format %{name}.spec. [x]: Static libraries in -static or -devel subpackage, providing -devel if present. Note: Package has .a files: libi40iw-devel-static. [x]: File names are valid UTF-8. [x]: Packages must not store files under /srv, /opt or /usr/local ===== SHOULD items ===== Generic: [x]: Buildroot is not present Note: Buildroot: present but not needed [x]: Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) Note: %clean present but not required [x]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [x]: Final provides and requires are sane (see attachments). [x]: Fully versioned dependency in subpackages if applicable. Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in libi40iw-debuginfo [?]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [-]: Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [?]: Package should compile and build into binary rpms on all supported architectures. [-]: %check is present and all tests pass. [x]: Packages should try to preserve timestamps of original installed files. [x]: Reviewer should test that the package builds in mock. [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [x]: Uses parallel make %{?_smp_mflags} macro. [x]: Sources can be downloaded from URI in Source: tag [x]: SourceX is a working URL. [x]: Spec use %global instead of %define unless justified. ===== EXTRA items ===== Generic: [x]: Rpmlint is run on debuginfo package(s). Note: No rpmlint messages. [x]: Rpmlint is run on all installed packages. Note: There are rpmlint messages (see attachment). [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. [x]: Spec file according to URL is the same as in SRPM. Basically, I think we're good now, and I just need to sort out some administrative things on the Fedora side to get the last bits done.
It's my non-RH login that actually has Fedora Package Review Approval and Sponsorship privs, so setting fedora-review flag to + now. Do you have an account in the Fedora Account System yet? Once you do, I can officially sponsor you as a new Fedora packager (if you're not already?). https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group
I have an account in the Fedora Account System. Let me know if more actions on my part are required.
Unfortunately it's not yet good package. > %defattr(-,root,root,-) MUST be dropped > %clean > rm -rf %{buildroot} MUST be dropped > make %{?_smp_mflags} COULD be replaced iwth %make_build > make DESTDIR=%{buildroot} install COULD be replace with %make_install > %setup -q -n %{name}-%{version} COULD be changed to %setup -q or to %autosetup > BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n) MUST be dropped > Group: System Environment/Libraries > Group: System Environment/Libraries COULD be dropped > %package devel-static It's actually not devel-static, but just static. Please adjust.
(In reply to Igor Gnatenko from comment #15) > Unfortunately it's not yet good package. > > > %defattr(-,root,root,-) > MUST be dropped This isn't a "MUST" from my reading of things. > > %clean > > rm -rf %{buildroot} > MUST be dropped This is under "SHOULD" in the packaging guidelines, not "MUST". > > make %{?_smp_mflags} > COULD be replaced iwth %make_build > > > make DESTDIR=%{buildroot} install > COULD be replace with %make_install Potayto, potahto. > > %setup -q -n %{name}-%{version} > COULD be changed to %setup -q or to %autosetup I did miss this one, could easily be just %setup -q, since '%{name}-%{version}' is already the default name things are expected to be in, but I wouldn't consider this a blocking issue. Easily fixed in the next bump. > > BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n) > MUST be dropped Also a "SHOULD", not a "MUST". > > Group: System Environment/Libraries > > Group: System Environment/Libraries > COULD be dropped > > > %package devel-static > It's actually not devel-static, but just static. Please adjust. Take a look at every other libibverbs hardware driver (libocrdma, libcxgb4, libmlx5, etc), this is how all of them look. This is following precedent.
(In reply to tatyana from comment #14) > I have an account in the Fedora Account System. Let me know if more actions > on my part are required. What is your FAS username? I'm struggling to find it at the moment. :)
My username is tatyana
(In reply to tatyana from comment #18) > My username is tatyana Ah, wasn't in the packager group yet, and I was apparently only looking within that group. I've added you to the group and sponsored you now. I believe the next step is: https://fedoraproject.org/wiki/Join_the_package_collection_maintainers?rd=PackageMaintainers/Join#Add_Package_to_Source_Code_Management_.28SCM.29_system_and_Set_Owner Should be able to request a new package there, with you as the maintainer. Please set me (FAS account name jwilson) as a co-maintainer.
I submitted a new package request: •user: tatyana request package: libi40iw-0.5.225-1.el7.src.rpm on branch master I am not sure if branch master was the right one.
Apparently, I need to fix the ticket number - should I send another package request. Is there a way to edit the first one?
(In reply to tatyana from comment #21) > Apparently, I need to fix the ticket number - should I send another package > request. Is there a way to edit the first one? Just gave it a try myself, I think the issue was the package name: •user: jwilson request package: libi40iw on branch master •user: jwilson request package: libi40iw on branch f24 •user: jwilson request package: libi40iw on branch f23 I don't see anything about fixing a ticket number yet anyway...
Yeah, that too is an issue. I am trying to submit a new package request, but when I enter the ticket number = bugzilla id = 1350029 and click sync, I get "Review not approved by the assignee of the ticket jarodwilson"
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/libi40iw
Now the sync is working. Which collection should I choose?
Ok, I see the package request has been approved. Thank you.
Ah, the problem was that the bug was still assigned to my RH login instead of the one I have on file with the Fedora package database. I'm updating some permissions on the package now, so you can build for Fedora 24 (just released a week or two ago, iirc) and Fedora 23 as well.
ping?
I thought that this package has been released and no further action was required. I don't know who needs to update the bug status.
(In reply to tatyana from comment #29) > I thought that this package has been released and no further action was > required. I don't know who needs to update the bug status. http://koji.fedoraproject.org/koji/packageinfo?packageID=22609 I don't see any builds of this package in Fedora. So you need to: 1. import spec and oter stuff into git repo 2. build in koji 3. submit updates in bodhi You can always ask someone who sponsored you into packagers group as it's his duty to help.
Thank you for the information. Just to clarify this package was released as part of RedHat 7.3. There is another libi40iw bug report (id=1381746) which targets RehHat 7.4. We are still working on preparing patches for the new libi40iw package. Are you referring to a new package for RedHat 7.4?
(In reply to tatyana from comment #31) > Thank you for the information. > > Just to clarify this package was released as part of RedHat 7.3. There is > another libi40iw bug report (id=1381746) which targets RehHat 7.4. We are > still working on preparing patches for the new libi40iw package. Are you > referring to a new package for RedHat 7.4? No, this is parallel to Red Hat Enterprise Linux work, libi40iw should also be built for Fedora. This particular bug was to get the package into Fedora, where it needs to be accepted before it goes into RHEL, we're just missing the final step where it's actually built in Fedora.
So, this package was never imported in Fedora. Tatyana, are you still interested in importing and building for Fedora, or should we properly retire it?
I've received no response, so I've filed a request to releng to orphan the package: https://pagure.io/releng/issue/10205
Package orphaned, closing as DEADREVIEW