Bug 1315870 - Review Request: libhfi1 - verbs userspace driver for Intel HFIs
Review Request: libhfi1 - verbs userspace driver for Intel HFIs
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Neil Horman
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 1171868 1315609 1273171
  Show dependency treegraph
 
Reported: 2016-03-08 15:23 EST by dennis.dalessandro
Modified: 2016-09-12 12:51 EDT (History)
9 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2016-09-12 12:29:52 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
nhorman: fedora‑review+
dennis.dalessandro: needinfo-


Attachments (Terms of Use)
spec review patch (1.12 KB, patch)
2016-03-09 21:16 EST, Honggang LI
no flags Details | Diff
spec review patch (1.17 KB, patch)
2016-03-10 01:45 EST, Honggang LI
no flags Details | Diff

  None (edit)
Description dennis.dalessandro 2016-03-08 15:23:58 EST
Spec URL: https://github.com/01org/opa-libhfi1verbs/releases/download/10_1/libhfi1verbs.spec
SRPM URL: https://github.com/01org/opa-libhfi1verbs/releases/download/10_1/libhfi1verbs-0.5-16.fc23.src.rpm

Description: 
libhfi1verbs provides a device-specific userspace driver for Intel Host
Fabric interface cards.  This driver is designed for use with the
libibverbs library.

Fedora Account System Username: ddalessa

This is my first package submitted for Fedora and as such will be needing a sponsor. I am the upstream maintainer for this package. Here is a link to to the koji build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=13277993
Comment 1 Honggang LI 2016-03-09 21:14:41 EST
(In reply to dennis.dalessandro from comment #0)
> Spec URL:
> https://github.com/01org/opa-libhfi1verbs/releases/download/10_1/
> libhfi1verbs.spec

Please see the attachment and update the spec file and the package. You also need update '%changelog' section.

http://people.redhat.com/honli/.bcc4d86c9ce430ffc0c40ba4b877b7c6/libmlx5.spec

Please take this one as example. Thanks
Comment 2 Honggang LI 2016-03-09 21:16 EST
Created attachment 1134688 [details]
spec review patch
Comment 3 Honggang LI 2016-03-10 01:45 EST
Created attachment 1134700 [details]
spec review patch
Comment 4 dennis.dalessandro 2016-03-10 15:58:52 EST
(In reply to Honggang LI from comment #3)
> Created attachment 1134700 [details]
> spec review patch

Thanks for the feedback. I can make those changes. One question/concern, about the valgrind requirement and the "configure --with-valgrind". I'm not sure if this has performance implications or not. Is this a hard requirement? I'm checking into its impact regardless.
Comment 5 Honggang LI 2016-03-10 19:57:14 EST
(In reply to dennis.dalessandro from comment #4)
> Is this a hard requirement? I'm checking into its impact regardless.

Yes, it is a hard requirement for RHEL. So, it is better to build with valgrind for fedora too. We test other InfiniBand user-space drivers, for example, libmlx4. The performance is good over our RDMA cluster. However, we don't have huge/large cluster for testing.
Comment 6 Honggang LI 2016-03-14 10:51:35 EDT
(In reply to dennis.dalessandro from comment #0)
> Spec URL:
> https://github.com/01org/opa-libhfi1verbs/releases/download/10_1/
> libhfi1verbs.spec
> SRPM URL:
> https://github.com/01org/opa-libhfi1verbs/releases/download/10_1/
> libhfi1verbs-0.5-16.fc23.src.rpm

Hi, Dennis
 It seems those URLs are no longer valid. Could you please check them?
Comment 7 dennis.dalessandro 2016-03-14 10:56:53 EDT
(In reply to Honggang LI from comment #6)
> (In reply to dennis.dalessandro from comment #0)
> > Spec URL:
> > https://github.com/01org/opa-libhfi1verbs/releases/download/10_1/
> > libhfi1verbs.spec
> > SRPM URL:
> > https://github.com/01org/opa-libhfi1verbs/releases/download/10_1/
> > libhfi1verbs-0.5-16.fc23.src.rpm
> 
> Hi, Dennis
>  It seems those URLs are no longer valid. Could you please check them?

Hi Honggang, yep, I am going to post new revisions there. I got side tracked looking into the valgrind thing. Should I re-post the old version until I'm ready to replace it?
Comment 8 Honggang LI 2016-03-14 11:01:22 EDT
Please re-post the old version then someone else can review those files. And I also need those files to run fedora-review as suggested by someone in IRC channel #fedora-devel.
Comment 9 dennis.dalessandro 2016-03-14 11:07:40 EDT
(In reply to Honggang LI from comment #8)
> Please re-post the old version then someone else can review those files. And
> I also need those files to run fedora-review as suggested by someone in IRC
> channel #fedora-devel.

Ok, done.

Spec URL: https://github.com/01org/opa-libhfi1verbs/releases/download/10_1/libhfi1verbs.spec

SRPM URL: https://github.com/01org/opa-libhfi1verbs/releases/download/10_1/libhfi1verbs-0.5-16.fc23.src.rpm
Comment 10 Honggang LI 2016-03-28 03:32:47 EDT
Hello, Dennis
 Ping? Any update?
Comment 12 Honggang LI 2016-03-29 07:05:37 EDT
Hello, Dennis

http://people.redhat.com/honli/.b187328c893321f3baa62cc5ef46f5d1/

I uploaded my review result into this http dir. Please fix the obsoleted m4s macros in upstream git repo and recreate the spec and src.

http://people.redhat.com/honli/.b187328c893321f3baa62cc5ef46f5d1/review.txt
=============================================
Generic:
[!]: Package should not use obsolete m4 macros
     Note: Some obsoleted macros found, see the attachment.
     See: https://fedorahosted.org/FedoraReview/wiki/AutoTools
..................
AutoTools: Obsoleted m4s found
------------------------------
  AM_PROG_LIBTOOL found in: libhfi1-0.5/configure.in:63
  AM_CONFIG_HEADER found in: libhfi1-0.5/configure.in:61

=============================================
What I modified based on your spec.
1) rename libhfi1verbs as libhfi1, as we had import it into rhel-7.2 with the name libhfi1. If Fedora and RHEL-7 have different names for this package. It will be an trouble.
https://bugzilla.redhat.com/show_bug.cgi?id=1251634

2) replace '%define' with '%global'

3) BuildRequires:  libibverbs-devel >= 1.2.0
We had update libibverbs to latest upstream v1.2.0 for Fedora.

4) replace '-devel' package with '-static'
a) There is a *.a file, it should be packaged with -static package, not -devel package.
b) All other verbs user space drivers have a static package instead a -devel package.

5) make %{?_smp_mflags}
It seems there is a typo in your spec file.

6) As this package is licensed by multiple licenses, so add comment in spec file as required.

7) Add changelog as I said in comment #1. Please check your name.


thanks
Comment 13 Neil Horman 2016-03-31 10:25:39 EDT
My review re-inforces Honggangs, pelase make those corrections, submit a new spec/srpm when complete, and inform us here of the update with the new location
Comment 14 Honggang LI 2016-04-08 01:54:19 EDT
Hello, Dennis
 Ping? Any update?

Thanks
Comment 15 dennis.dalessandro 2016-04-11 09:11:46 EDT
(In reply to Honggang LI from comment #14)
> Hello, Dennis
>  Ping? Any update?
> 
> Thanks

I was out of the office last week on travel. Will get to this as soon as I dig out.
Comment 16 dennis.dalessandro 2016-04-15 12:15:27 EDT
(In reply to Honggang LI from comment #12)
> 3) BuildRequires:  libibverbs-devel >= 1.2.0
> We had update libibverbs to latest upstream v1.2.0 for Fedora.

Do we really need to update this? There still may be systems that want to build this RPM but have not upgraded to 1.2.0, not sure how we would handle that.
Comment 17 Neil Horman 2016-04-15 12:19:17 EDT
Its a BuildRequires.  If someone is building the rpm they can adhere to the levels Fedora requires. If they aren't using fedora, then they can modify the spec file.
Comment 18 dennis.dalessandro 2016-04-15 12:50:12 EDT
So I'm asking is it a hard requirement to use the newer package? If so, why? is there a specific problem with the older version (which would be good to know for other reasons), or is a general policy that the latest/greatest is required for Fedora packages?
Comment 19 dennis.dalessandro 2016-04-15 14:11:25 EDT
(In reply to Honggang LI from comment #12)
> 6) As this package is licensed by multiple licenses, so add comment in spec
> file as required.

Not sure what you mean here? The spec file has the GPL and BSD license text in the comments at the beginning.
Comment 20 Honggang LI 2016-04-17 22:04:20 EDT
(In reply to dennis.dalessandro from comment #18)
> So I'm asking is it a hard requirement to use the newer package? If so, why?
> is there a specific problem with the older version (which would be good to
> know for other reasons), or is a general policy that the latest/greatest is
> required for Fedora packages?

It works with older libibverbs. However, we update 'BuildRequires:  libibverbs-devel >= 1.2.0' for *all* RDMA user space drivers to prevent build the drivers against old libibverbs in Fedora koji system. As Fedora always asks for latest upstream release to be packaged up, the latest libibverbs will be installed. Building packages with latest required will ensure packages built and run with the same version required packages.
Comment 21 Honggang LI 2016-04-17 22:16:00 EDT
(In reply to dennis.dalessandro from comment #19)
> (In reply to Honggang LI from comment #12)
> > 6) As this package is licensed by multiple licenses, so add comment in spec
> > file as required.
> 
> Not sure what you mean here? The spec file has the GPL and BSD license text
> in the comments at the beginning.

As the software is dual licensed, it is OK to ignore my comment.

https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Dual_Licensing_Scenarios
Comment 22 Neil Horman 2016-04-18 09:30:09 EDT
Concur with comment 20, we need the latest package from upstream

However, the license does need to get fixed.  As per the licensing guidelines:
https://fedoraproject.org/wiki/Packaging:LicensingGuidelines?rd=Packaging/LicensingGuidelines

The license field in the spec applies to the binaries in the resultant packages

Just because the source has multiple license options, doesn't mean you just pass that through to the binary.  The dual license option is for use when you have multiple binaries in a given binary rpm, some of which have license X, some of which have license Y. If they all are being distributed under the same license, then you either need to:

1) Pick a single license and update the spec file as such
or
2) Change the or in the License field to an And

See:
https://fedoraproject.org/wiki/Packaging:LicensingGuidelines?rd=Packaging/LicensingGuidelines#Multiple_Licensing_Scenarios

for details
Comment 23 dennis.dalessandro 2016-04-18 09:51:32 EDT
(In reply to Neil Horman from comment #22)
> Just because the source has multiple license options, doesn't mean you just
> pass that through to the binary.  The dual license option is for use when
> you have multiple binaries in a given binary rpm, some of which have license
> X, some of which have license Y. If they all are being distributed under the
> same license, then you either need to:

That's not the way I read it. I think you are describing the "Multiple license" option, not "Dual license". All binaries have the same license (GPL/BSD). "Multiple" license is when there are different licenses for different binaries. That is not the case here.

I think Honggang is correct in comment 21. Please confirm?
Comment 24 Neil Horman 2016-05-05 08:07:20 EDT
You're right, my bad, the license is ok, the other BuildRequires issues and such need to get fixed though
Comment 25 dennis.dalessandro 2016-05-05 08:19:02 EDT
Ok sounds like we are on the same page then. I've made the other changes and have it out for review/testing on our end. Should be posted as a new version here soon.
Comment 26 dennis.dalessandro 2016-05-12 09:45:36 EDT
I think I got the requested changes covered, please take a look and let me know any further review comments. Here are the Koji test build, spec, and srpm links.

http://koji.fedoraproject.org/koji/taskinfo?taskID=14025958

https://github.com/01org/opa-libhfi1verbs/releases/download/10_1_2/libhfi1-0.5-23.el7.src.rpm

https://github.com/01org/opa-libhfi1verbs/releases/download/10_1_2/libhfi1.spec
Comment 27 Neil Horman 2016-07-06 09:39:43 EDT
sorry, I completely missed this, I'll review it right away
Comment 28 Neil Horman 2016-07-06 10:00:30 EDT
It looks like at some point in your update you removed the _smp_mflags macro:

[!]: Uses parallel make %{?_smp_mflags} macro.

Its not a huge deal, I can let that slide, but please fix it when you check it in.

Also, something just occured to me.  Is this verb library named libhfi1 because its meant to supersede libhfi?  If so, you might consider renaming it upstream, and just using the epoch tag in the spec to make sure the upgrade path is right.  Doing so would let you avoid the obsoletes tag requirements, as you we could update the libhfiverbs package that exists.

The question to really answer is "is libhfiverbs still under active development"?  If it is, then you proably want what you have.  If not, and if libfhi1 is compatible with libhfi in terms of hardware, then you may want to consider renaming the package upstream.

Let me know what you want to do.  If you are happy with the way it currently is, then I can approve this as it is.
Comment 29 John Fleck 2016-07-12 15:55:11 EDT
libhfi1 is the new name of the package, we renamed it to match the RHEL name.
libhfi1verbs was the old name.
There was never a libhfi.
We'll look into the epoch tag.

Did this answer your question?
Comment 30 Neil Horman 2016-07-13 09:21:46 EDT
ok, then you have what you want, thanks.

APROVED.
Comment 31 dennis.dalessandro 2016-07-26 11:19:41 EDT
Hi Neil,

What info is being requested from me at this point? So you marked it approved, now am I waiting to get a sponsor? Does the FE_NEEDSPONSOR in the blocks section accomplish that or is there something else I should do to move that along.
Comment 32 Neil Horman 2016-08-17 09:26:39 EDT
whats your fedora user name, I can sponsor you in.
Comment 33 dennis.dalessandro 2016-08-25 14:14:56 EDT
Just now seen this. I have been out for a couple weeks. My fedora user name is ddalessa.
Comment 34 Neil Horman 2016-08-29 13:01:24 EDT
done.
Comment 35 dennis.dalessandro 2016-08-29 13:16:35 EDT
Thank Neil. So looks like the next steps for me are to follow:

https://fedoraproject.org/wiki/Join_the_package_collection_maintainers?rd=PackageMaintainers/Join#Add_Package_to_Source_Code_Management_.28SCM.29_system_and_Set_Owner

What other info do you need from me at this point?
Comment 36 Neil Horman 2016-08-29 13:40:09 EDT
Nothing, you can proceed with the documentation at the link you have above
Comment 37 dennis.dalessandro 2016-09-06 10:04:48 EDT
(In reply to Neil Horman from comment #28)
> It looks like at some point in your update you removed the _smp_mflags macro:
> 
> [!]: Uses parallel make %{?_smp_mflags} macro.
> 
> Its not a huge deal, I can let that slide, but please fix it when you check
> it in.
> 

In comment 12 it said this was a typo. So I just removed it and things seemed to work OK.
Comment 38 Gwyn Ciesla 2016-09-06 10:09:06 EDT
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/libhfi1
Comment 39 Neil Horman 2016-09-06 10:54:56 EDT
good, so you can import the package, build it and close this bug
Comment 40 dennis.dalessandro 2016-09-12 10:28:33 EDT
Just an update: I've done the steps on the above page up to and including the git push. However the "fedpkg build" is just hanging. Asking around here for hints at if there is something I need to do, proxy related or what not, to get it to work.
Comment 41 dennis.dalessandro 2016-09-12 12:29:52 EDT
Was able to get fedpkg build to work.

http://koji.fedoraproject.org/koji/taskinfo?taskID=15602698

Marking as CLOSED but not sure what it should be closed as, taking a guess, please update if needed.
Comment 42 Parag AN(पराग) 2016-09-12 12:51:39 EDT
A bug blocking FE-NEEDSPONSOR gets closed that mean this review reporter must have been sponsored already. Hence, Removing FE-NEEDSPONSOR from this review bug.

Note You need to log in before you can comment on or make changes to this bug.