Hide Forgot
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
(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
Created attachment 1134688 [details] spec review patch
Created attachment 1134700 [details] spec review patch
(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.
(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.
(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?
(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?
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.
(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
Hello, Dennis Ping? Any update?
(In reply to Honggang LI from comment #10) > Hello, Dennis > Ping? Any update? SRPM: https://github.com/01org/opa-libhfi1verbs/releases/download/10_1_1/libhfi1verbs-0.5-17.fc23.src.rpm SPEC: https://github.com/01org/opa-libhfi1verbs/releases/download/10_1_1/libhfi1verbs.spec Koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=13484725
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
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
Hello, Dennis Ping? Any update? Thanks
(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.
(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.
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.
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?
(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.
(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.
(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
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
(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?
You're right, my bad, the license is ok, the other BuildRequires issues and such need to get fixed though
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.
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
sorry, I completely missed this, I'll review it right away
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.
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?
ok, then you have what you want, thanks. APROVED.
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.
whats your fedora user name, I can sponsor you in.
Just now seen this. I have been out for a couple weeks. My fedora user name is ddalessa.
done.
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?
Nothing, you can proceed with the documentation at the link you have above
(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.
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/libhfi1
good, so you can import the package, build it and close this bug
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.
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.
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.