Bug 1298665 - Review Request: libvma - Dramatically improves performance of socket based applications
Review Request: libvma - Dramatically improves performance of socket based ap...
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
x86_64 Linux
unspecified Severity medium
: ---
: ---
Assigned To: Neil Horman
Fedora Extras Quality Assurance
https://github.com/Mellanox/libvma
:
Depends On:
Blocks: 1315609
  Show dependency treegraph
 
Reported: 2016-01-14 12:15 EST by Avner BenHanoch
Modified: 2016-07-29 05:43 EDT (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2016-03-28 07:41:46 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
nhorman: fedora‑review+


Attachments (Terms of Use)


External Trackers
Tracker ID Priority Status Summary Last Updated
Red Hat Bugzilla 1271624 None None None 2016-01-14 12:17 EST

  None (edit)
Description Avner BenHanoch 2016-01-14 12:15:46 EST
Spec URL: http://www.mellanox.com/downloads/Accelerator/libvma.spec
SRPM URL: http://www.mellanox.com/downloads/Accelerator/libvma-7.0.13-1.fc23.src.rpm
Description:
libvma is dynamically linked user space library for enhancing the performance of TCP and UDP networking-heavy applications over RDMA-capable NICs.
It allows application written over standard socket API to run with full network stack bypass from user-space which result in latency reduction and increased throughput and packet rate

Fedora Account System Username: avnerbh
Comment 1 Michal Schmidt 2016-01-15 09:53:01 EST
Hello Avner,
you're not yet in the "packager" group, so I'm marking this review request as needing a sponsor.
I will do a review.
Comment 2 Michal Schmidt 2016-01-15 10:24:42 EST
Just a few things I noticed (not yet a full review):
- You're using too many macros for my taste. Some of them are entirely unused.
- Some spec file tags are unnecessary these days (Group, BuildRoot, the %clean
  step, %defattr).
- I don't like messing with limits.conf in %post like that. If you really need
  to reconfigure the limits, consider shipping a fragment file in
  /etc/security/limits.d/
Comment 3 Ophir Munk 2016-02-03 02:01:53 EST
A new branch named "fedora" was created under https://github.com/Mellanox/libvma
Please switch to fedora branch by selecting the branch name on the upper left "branch:" button.

All packaging related development will be handled on this branch before being merged into the master branch.
Comment 4 Upstream Release Monitoring 2016-02-21 03:59:07 EST
alexva's scratch build of libvma-7.0.14-1.fc24.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=13075948
Comment 5 alexv 2016-02-23 12:57:12 EST
Hi Michal,

Following your feedback we’ve updated the libvma.spec file: http://www.mellanox.com/downloads/Accelerator/libvma.spec and created a new libvma source RPM file: http://www.mellanox.com/downloads/Accelerator/libvma-7.0.14-1.fc24.src.rpm 
Please review.

Thanks,
AlexV
Comment 6 Ophir Munk 2016-02-27 05:42:23 EST
Branch "fedora" under https://github.com/Mellanox/libvma
was merged into master branch. 
All packaging related code is under master branch
Comment 7 Michal Schmidt 2016-02-29 10:29:26 EST
> ExcludeArch: %{arm} %{ix86} s390x ppc64 ppc64le

Every use of ExcludeArch should have a comment explaining the reason.

> %description
> libvma is a LD_PRELOAD-able library that boosts performance
> of TCP and UDP traffic

Missing a dot to end the sentence.

> Part of Mellanox's enhanced services

Does it mean libvma works only with Mellanox hardware?
If yes, please rephrase it in a clearer way.
If it does not mean that, then does it mean anything at all?

> Allows application written over standard socket API
> to run over Infiniband/Ethernet from user space with full network stack bypass
> and get better throughput, latency and packets/sec rate.

It allows applications [...]

> %pre
> if [ `grep "^[^#]" /etc/security/limits.conf /etc/security/limits.d/* 2> /dev/null |grep memlock|grep unlimited | wc -l` -le 0 ]; then

Looks like it could be simplified to:
if ! grep -qs '^[^#].*memlock.*unlimited' /etc/security/limits.conf /etc/security/limits.d/*; then

Anyway, do you really need to emit the message?
It is unusual for a scriptlet in Fedora to emit any output whatsoever.

> %files
> %{_libdir}/%{name}*.so.*
> %{_libdir}/%{name}.so

You may want to add a comment here saying you need libvma.so in the main package
(as opposed to -devel) so that 'LD_PRELOAD=libvma.so <command>' works.

> %doc README.txt journal.txt VMA_VERSION

I see your upstream tarball contains a COPYING file, that you're not including in the package.
Ideally it would contain the full text of the GPLv2
(Would you please make that change upstream?
 Currently it has only a copy of the header as it appears in the source files.)
and be included in the package using the %license tag (not %doc).
Comment 8 alexv 2016-02-29 12:28:41 EST
(In reply to Michal Schmidt from comment #7)
> > ExcludeArch: %{arm} %{ix86} s390x ppc64 ppc64le
> 
> Every use of ExcludeArch should have a comment explaining the reason.
 
I will fix it.

> > %description
> > libvma is a LD_PRELOAD-able library that boosts performance
> > of TCP and UDP traffic
> 
> Missing a dot to end the sentence.
 
I will fix it.

> > Part of Mellanox's enhanced services
> 
> Does it mean libvma works only with Mellanox hardware?
> If yes, please rephrase it in a clearer way.
> If it does not mean that, then does it mean anything at all?
 
libvma can be supported by RDMA capable devices that support
IBV_QPT_RAW_PACKET QPs.
I will rephrase the description.

> > Allows application written over standard socket API
> > to run over Infiniband/Ethernet from user space with full network stack bypass
> > and get better throughput, latency and packets/sec rate.
> 
> It allows applications [...]
 
I will fix it.

> > %pre
> > if [ `grep "^[^#]" /etc/security/limits.conf /etc/security/limits.d/* 2> /dev/null |grep memlock|grep unlimited | wc -l` -le 0 ]; then
 
> Looks like it could be simplified to:
> if ! grep -qs '^[^#].*memlock.*unlimited' /etc/security/limits.conf
> /etc/security/limits.d/*; then
> 
> Anyway, do you really need to emit the message?
> It is unusual for a scriptlet in Fedora to emit any output whatsoever.
 
We print this message in order to notify about system configuration change (memlock limit). I agree that this is not crucial and can be removed.

> > %files
> > %{_libdir}/%{name}*.so.*
> > %{_libdir}/%{name}.so
>
> You may want to add a comment here saying you need libvma.so in the main
> package
> (as opposed to -devel) so that 'LD_PRELOAD=libvma.so <command>' works.

Good idea, I will add this comment.
 
> > %doc README.txt journal.txt VMA_VERSION
> 
> I see your upstream tarball contains a COPYING file, that you're not
> including in the package.
> Ideally it would contain the full text of the GPLv2
> (Would you please make that change upstream?
>  Currently it has only a copy of the header as it appears in the source
> files.)

I am checking if we can make this change.

> and be included in the package using the %license tag (not %doc).

I will add the COPYING file to the package under the %license tag.
Comment 9 Upstream Release Monitoring 2016-03-01 01:56:12 EST
alexva's scratch build of libvma-7.0.14-1.fc24.src.rpm for rawhide failed http://koji.fedoraproject.org/koji/taskinfo?taskID=13185983
Comment 10 Upstream Release Monitoring 2016-03-01 05:22:32 EST
alexva's scratch build of libvma-7.0.14-1.fc24.src.rpm for rawhide failed http://koji.fedoraproject.org/koji/taskinfo?taskID=13187585
Comment 11 Upstream Release Monitoring 2016-03-01 11:26:08 EST
alexva's scratch build of libvma-7.0.14-1.fc24.src.rpm for rawhide failed http://koji.fedoraproject.org/koji/taskinfo?taskID=13190047
Comment 12 Upstream Release Monitoring 2016-03-01 14:16:47 EST
alexva's scratch build of libvma-7.0.14-1.fc24.src.rpm for rawhide failed http://koji.fedoraproject.org/koji/taskinfo?taskID=13191401
Comment 13 Upstream Release Monitoring 2016-03-03 11:23:12 EST
alexva's scratch build of libvma-7.0.14-2.fc24.src.rpm for rawhide completed http://koji.fedoraproject.org/koji/taskinfo?taskID=13215203
Comment 14 alexv 2016-03-03 11:43:07 EST
Hi Michal,

I've updated the package:

* Wed Mar 2 2016 Alex Vainman <alexv@mellanox.com> - 7.0.14-2
- Added reasoning for archs exclusion
- Package description improvement
- Removal of the pre scriplet
- Added COPYING and LICENSE files to the package

Notice: a new packaged "LICENSE" file contains the full GPLv2 license text as you requested.

Spec URL: http://www.mellanox.com/downloads/Accelerator/libvma.spec
SRPM URL: http://www.mellanox.com/downloads/Accelerator/libvma-7.0.14-2.fc24.src.rpm
Comment 15 Michal Schmidt 2016-03-04 11:42:39 EST
[We're getting close to completion of this review, so I'm reassigning this
to Neil Horman who has package sponsor powers and tentatively agreed
to sponsor you. I remain on the CC list.]

I see you excluded most of the architectures, just because libvma was
not tested on them. I think your standards for inclusion are too high :-)
For example, very few Fedora packagers test their packages on i686 these
days, but they don't exclude that arch.
But whatever, I'll leave that choice to you.
Note that you misspelled "libvma" as "libmva" in the comments.


I realized the COPYING file (just like the headers in the source files)
is somewhat contradictory:
/*
 * Copyright (C) Mellanox Technologies Ltd. 2001-2013.  ALL RIGHTS RESERVED.
 *
 * This software product is a proprietary product of Mellanox Technologies Ltd.
 * (the "Company") and all right, title, and interest in and to the software product,
 * including all associated intellectual property rights, are and shall
 * remain exclusively with the Company.

The above paragraph makes it look like the source code was never meant
to leak outside the Company. Maybe I'm misunderstanding it.

 *
 * This software is made available under either the GPL v2 license or a commercial license.

OK, a relief!

To clear any doubts, would it be possible to apply the GPLv2 the way
the FSF recommends it?:
http://www.gnu.org/licenses/old-licenses/gpl-2.0.en.html#SEC4
Just make it "version 2 of the License"
without the "or (at your option) any later version".

 * If you wish to obtain a commercial license, please contact Mellanox at support@mellanox.com.
 */

OK, nothing wrong with a small ad. This line can stay.


In %build I recommend running make with V=1 so that the compiler command lines
are visible in build.log.


Here's edited output from the fedora-review tool:

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated


Issues:
=======
(fedora-review reported an installation error, which was a false positive.
It also complained about *.so in the main package, but we agreed this is
necessary in this case.)

===== 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]: ldconfig called in %post and %postun if required.
[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: "BSD (3 clause)", "BSD (2 clause)", "GPL (v3 or later)",
     "Unknown or generated". 277 files have unknown license. Detailed
     output of licensecheck in
     /home/michich/1298665-libvma/licensecheck.txt

 ### The "unknown license" is GPLv2 - the source files use a different
     license header than the one recommended by FSF.
     The GPLv3 files are src/vma/config_parser.[ch] - they are generated
     by GNU Bison and they permit the use in non-GPLv3 software by
     the usual Bison license exception.

[x]: License file installed when any subpackage combination is installed.
[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.
[!]: %config files are marked noreplace or the reason is justified.
     Note: No (noreplace) in %config (noreplace) /etc/security/limits.d/30
     -libvma-limits.conf

 ### I'm not sure if fedora-review is stricter than rpmbuild about
     the whitespace, but you'd better remove the space.

[x]: 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.
[x]: 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.
[x]: 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.

 ### Well, it does use ExcludeArch, but the use is explained. So OK.

[x]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 122880 bytes in 3 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]: 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]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[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 libvma-
     debuginfo
[?]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Scriptlets must be sane, if used.
[x]: 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]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Uses parallel make %{?_smp_mflags} macro.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[!]: Rpmlint is run on all installed packages.
     See: http://fedoraproject.org/wiki/Packaging/Guidelines#rpmlint
[!]: Package should not use obsolete m4 macros
     Note: Some obsoleted macros found, see the attachment.
     See: https://fedorahosted.org/FedoraReview/wiki/AutoTools
[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.


Rpmlint
-------
Checking: libvma-7.0.14-2.fc25.x86_64.rpm
          libvma-devel-7.0.14-2.fc25.x86_64.rpm
          libvma-utils-7.0.14-2.fc25.x86_64.rpm
          libvma-debuginfo-7.0.14-2.fc25.x86_64.rpm
          libvma-7.0.14-2.fc25.src.rpm
libvma.x86_64: W: devel-file-in-non-devel-package /usr/lib64/libvma.so
libvma-devel.x86_64: W: no-documentation
libvma-utils.x86_64: W: no-documentation
libvma-utils.x86_64: W: no-manual-page-for-binary vma_stats
5 packages and 0 specfiles checked; 0 errors, 4 warnings.




Requires
--------
libvma-devel (rpmlib, GLIBC filtered):
    libvma(x86-64)

libvma-debuginfo (rpmlib, GLIBC filtered):

libvma (rpmlib, GLIBC filtered):
    /sbin/ldconfig
    config(libvma)
    ld-linux-x86-64.so.2()(64bit)
    libc.so.6()(64bit)
    libdl.so.2()(64bit)
    libgcc_s.so.1()(64bit)
    libgcc_s.so.1(GCC_3.0)(64bit)
    libibverbs.so.1()(64bit)
    libibverbs.so.1(IBVERBS_1.0)(64bit)
    libibverbs.so.1(IBVERBS_1.1)(64bit)
    libm.so.6()(64bit)
    libnl-3.so.200()(64bit)
    libnl-3.so.200(libnl_3)(64bit)
    libnl-route-3.so.200()(64bit)
    libnl-route-3.so.200(libnl_3)(64bit)
    libpthread.so.0()(64bit)
    librdmacm.so.1()(64bit)
    librdmacm.so.1(RDMACM_1.0)(64bit)
    librt.so.1()(64bit)
    libstdc++.so.6()(64bit)
    libstdc++.so.6(CXXABI_1.3)(64bit)
    libstdc++.so.6(CXXABI_1.3.8)(64bit)
    libstdc++.so.6(CXXABI_1.3.9)(64bit)
    libvma.so.7()(64bit)
    pam
    rtld(GNU_HASH)

libvma-utils (rpmlib, GLIBC filtered):
    libc.so.6()(64bit)
    libdl.so.2()(64bit)
    libgcc_s.so.1()(64bit)
    libgcc_s.so.1(GCC_3.0)(64bit)
    libm.so.6()(64bit)
    libpthread.so.0()(64bit)
    librt.so.1()(64bit)
    libstdc++.so.6()(64bit)
    libstdc++.so.6(CXXABI_1.3)(64bit)
    libstdc++.so.6(CXXABI_1.3.9)(64bit)
    libvma(x86-64)
    rtld(GNU_HASH)



Provides
--------
libvma-devel:
    libvma-devel
    libvma-devel(x86-64)

libvma-debuginfo:
    libvma-debuginfo
    libvma-debuginfo(x86-64)

libvma:
    config(libvma)
    libvma
    libvma(x86-64)
    libvma.so.7()(64bit)

libvma-utils:
    libvma-utils
    libvma-utils(x86-64)



Unversioned so-files
--------------------
libvma: /usr/lib64/libvma.so

Source checksums
----------------
http://www.mellanox.com/downloads/Accelerator/libvma-7.0.14.tar.gz :
  CHECKSUM(SHA256) this package     : c13a6576e3d94cff9e5456d982623253fa5e07051b01de4223301f2adcb9cc59
  CHECKSUM(SHA256) upstream package : c13a6576e3d94cff9e5456d982623253fa5e07051b01de4223301f2adcb9cc59


AutoTools: Obsoleted m4s found
------------------------------
  AC_PROG_LIBTOOL found in: libvma-7.0.14/configure.ac:28


Generated by fedora-review 0.6.0 (3c5c9d7) last change: 2015-05-20
Command line :/usr/bin/fedora-review -b 1298665
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, Shell-api, C/C++
Disabled plugins: Java, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP, Ruby
Disabled flags: EXARCH, DISTTAG, EPEL5, BATCH, EPEL6
Comment 16 alexv 2016-03-04 15:16:43 EST
(In reply to Michal Schmidt from comment #15)
> [We're getting close to completion of this review, so I'm reassigning this
> to Neil Horman who has package sponsor powers and tentatively agreed
> to sponsor you. I remain on the CC list.]
> 
> I see you excluded most of the architectures, just because libvma was
> not tested on them. I think your standards for inclusion are too high :-)
> For example, very few Fedora packagers test their packages on i686 these
> days, but they don't exclude that arch.
> But whatever, I'll leave that choice to you.
I see, we will reconsider this.
> Note that you misspelled "libvma" as "libmva" in the comments.
> 
I will fix the spelling.
> 
> I realized the COPYING file (just like the headers in the source files)
> is somewhat contradictory:
> /*
>  * Copyright (C) Mellanox Technologies Ltd. 2001-2013.  ALL RIGHTS RESERVED.
>  *
>  * This software product is a proprietary product of Mellanox Technologies
> Ltd.
>  * (the "Company") and all right, title, and interest in and to the software
> product,
>  * including all associated intellectual property rights, are and shall
>  * remain exclusively with the Company.
> 
> The above paragraph makes it look like the source code was never meant
> to leak outside the Company. Maybe I'm misunderstanding it.
> 
>  *
>  * This software is made available under either the GPL v2 license or a
> commercial license.
> 
> OK, a relief!
> 
> To clear any doubts, would it be possible to apply the GPLv2 the way
> the FSF recommends it?:
> http://www.gnu.org/licenses/old-licenses/gpl-2.0.en.html#SEC4
> Just make it "version 2 of the License"
> without the "or (at your option) any later version".
> 
I am checking if we can make this change.
>  * If you wish to obtain a commercial license, please contact Mellanox at
> support@mellanox.com.
>  */
> 
> OK, nothing wrong with a small ad. This line can stay.
> 
> 
> In %build I recommend running make with V=1 so that the compiler command
> lines
> are visible in build.log.
>
OK, I will add it. 
> 
> Here's edited output from the fedora-review tool:
> 
> Package Review
> ==============
> 
> Legend:
> [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
> 
> 
> Issues:
> =======
> (fedora-review reported an installation error, which was a false positive.
> It also complained about *.so in the main package, but we agreed this is
> necessary in this case.)
> 
> ===== 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]: ldconfig called in %post and %postun if required.
> [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: "BSD (3 clause)", "BSD (2 clause)", "GPL (v3 or later)",
>      "Unknown or generated". 277 files have unknown license. Detailed
>      output of licensecheck in
>      /home/michich/1298665-libvma/licensecheck.txt
> 
>  ### The "unknown license" is GPLv2 - the source files use a different
>      license header than the one recommended by FSF.
>      The GPLv3 files are src/vma/config_parser.[ch] - they are generated
>      by GNU Bison and they permit the use in non-GPLv3 software by
>      the usual Bison license exception.
> 
> [x]: License file installed when any subpackage combination is installed.
> [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.
> [!]: %config files are marked noreplace or the reason is justified.
>      Note: No (noreplace) in %config (noreplace) /etc/security/limits.d/30
>      -libvma-limits.conf
> 
>  ### I'm not sure if fedora-review is stricter than rpmbuild about
>      the whitespace, but you'd better remove the space.
> 
OK, I will fix this.
> [x]: 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.
> [x]: 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.
> [x]: 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.
> 
>  ### Well, it does use ExcludeArch, but the use is explained. So OK.
> 
> [x]: Large documentation must go in a -doc subpackage. Large could be size
>      (~1MB) or number of files.
>      Note: Documentation size is 122880 bytes in 3 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]: 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]: File names are valid UTF-8.
> [x]: Packages must not store files under /srv, /opt or /usr/local
> 
> ===== SHOULD items =====
> 
> Generic:
> [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 libvma-
>      debuginfo
> [?]: Package functions as described.
> [x]: Latest version is packaged.
> [x]: Package does not include license text files separate from upstream.
> [x]: Scriptlets must be sane, if used.
> [x]: 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]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
> [x]: Sources can be downloaded from URI in Source: tag
> [x]: Reviewer should test that the package builds in mock.
> [x]: Buildroot is not present
> [x]: Package has no %clean section with rm -rf %{buildroot} (or
>      $RPM_BUILD_ROOT)
> [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
> [x]: Uses parallel make %{?_smp_mflags} macro.
> [x]: SourceX is a working URL.
> [x]: Spec use %global instead of %define unless justified.
> 
> ===== EXTRA items =====
> 
> Generic:
> [!]: Rpmlint is run on all installed packages.
>      See: http://fedoraproject.org/wiki/Packaging/Guidelines#rpmlint
> [!]: Package should not use obsolete m4 macros
>      Note: Some obsoleted macros found, see the attachment.
>      See: https://fedorahosted.org/FedoraReview/wiki/AutoTools
> [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.
> 
> 
> Rpmlint
> -------
> Checking: libvma-7.0.14-2.fc25.x86_64.rpm
>           libvma-devel-7.0.14-2.fc25.x86_64.rpm
>           libvma-utils-7.0.14-2.fc25.x86_64.rpm
>           libvma-debuginfo-7.0.14-2.fc25.x86_64.rpm
>           libvma-7.0.14-2.fc25.src.rpm
> libvma.x86_64: W: devel-file-in-non-devel-package /usr/lib64/libvma.so
> libvma-devel.x86_64: W: no-documentation
> libvma-utils.x86_64: W: no-documentation
> libvma-utils.x86_64: W: no-manual-page-for-binary vma_stats
> 5 packages and 0 specfiles checked; 0 errors, 4 warnings.
> 
> 
> 
> 
> Requires
> --------
> libvma-devel (rpmlib, GLIBC filtered):
>     libvma(x86-64)
> 
> libvma-debuginfo (rpmlib, GLIBC filtered):
> 
> libvma (rpmlib, GLIBC filtered):
>     /sbin/ldconfig
>     config(libvma)
>     ld-linux-x86-64.so.2()(64bit)
>     libc.so.6()(64bit)
>     libdl.so.2()(64bit)
>     libgcc_s.so.1()(64bit)
>     libgcc_s.so.1(GCC_3.0)(64bit)
>     libibverbs.so.1()(64bit)
>     libibverbs.so.1(IBVERBS_1.0)(64bit)
>     libibverbs.so.1(IBVERBS_1.1)(64bit)
>     libm.so.6()(64bit)
>     libnl-3.so.200()(64bit)
>     libnl-3.so.200(libnl_3)(64bit)
>     libnl-route-3.so.200()(64bit)
>     libnl-route-3.so.200(libnl_3)(64bit)
>     libpthread.so.0()(64bit)
>     librdmacm.so.1()(64bit)
>     librdmacm.so.1(RDMACM_1.0)(64bit)
>     librt.so.1()(64bit)
>     libstdc++.so.6()(64bit)
>     libstdc++.so.6(CXXABI_1.3)(64bit)
>     libstdc++.so.6(CXXABI_1.3.8)(64bit)
>     libstdc++.so.6(CXXABI_1.3.9)(64bit)
>     libvma.so.7()(64bit)
>     pam
>     rtld(GNU_HASH)
> 
> libvma-utils (rpmlib, GLIBC filtered):
>     libc.so.6()(64bit)
>     libdl.so.2()(64bit)
>     libgcc_s.so.1()(64bit)
>     libgcc_s.so.1(GCC_3.0)(64bit)
>     libm.so.6()(64bit)
>     libpthread.so.0()(64bit)
>     librt.so.1()(64bit)
>     libstdc++.so.6()(64bit)
>     libstdc++.so.6(CXXABI_1.3)(64bit)
>     libstdc++.so.6(CXXABI_1.3.9)(64bit)
>     libvma(x86-64)
>     rtld(GNU_HASH)
> 
> 
> 
> Provides
> --------
> libvma-devel:
>     libvma-devel
>     libvma-devel(x86-64)
> 
> libvma-debuginfo:
>     libvma-debuginfo
>     libvma-debuginfo(x86-64)
> 
> libvma:
>     config(libvma)
>     libvma
>     libvma(x86-64)
>     libvma.so.7()(64bit)
> 
> libvma-utils:
>     libvma-utils
>     libvma-utils(x86-64)
> 
> 
> 
> Unversioned so-files
> --------------------
> libvma: /usr/lib64/libvma.so
> 
> Source checksums
> ----------------
> http://www.mellanox.com/downloads/Accelerator/libvma-7.0.14.tar.gz :
>   CHECKSUM(SHA256) this package     :
> c13a6576e3d94cff9e5456d982623253fa5e07051b01de4223301f2adcb9cc59
>   CHECKSUM(SHA256) upstream package :
> c13a6576e3d94cff9e5456d982623253fa5e07051b01de4223301f2adcb9cc59
> 
> 
> AutoTools: Obsoleted m4s found
> ------------------------------
>   AC_PROG_LIBTOOL found in: libvma-7.0.14/configure.ac:28
> 
> 
> Generated by fedora-review 0.6.0 (3c5c9d7) last change: 2015-05-20
> Command line :/usr/bin/fedora-review -b 1298665
> Buildroot used: fedora-rawhide-x86_64
> Active plugins: Generic, Shell-api, C/C++
> Disabled plugins: Java, Python, fonts, SugarActivity, Ocaml, Perl, Haskell,
> R, PHP, Ruby
> Disabled flags: EXARCH, DISTTAG, EPEL5, BATCH, EPEL6
Comment 17 alexv 2016-03-06 10:28:41 EST
>[We're getting close to completion of this review, so I'm reassigning this
>to Neil Horman who has package sponsor powers and tentatively agreed
>to sponsor you. I remain on the CC list.]
Thanks Michal!

Neil, nice to e-meet you :)
Please let me know if I can do anything in order to promote this task.

>I realized the COPYING file (just like the headers in the source files)
>is somewhat contradictory:
>/*
> * Copyright (C) Mellanox Technologies Ltd. 2001-2013.  ALL RIGHTS RESERVED.
> *
> * This software product is a proprietary product of Mellanox Technologies Ltd.
> * (the "Company") and all right, title, and interest in and to the software product,
> * including all associated intellectual property rights, are and shall
> * remain exclusively with the Company.
>
>The above paragraph makes it look like the source code was never meant
>to leak outside the Company. Maybe I'm misunderstanding it.
>
> *
> * This software is made available under either the GPL v2 license or a commercial license.
>
>OK, a relief!
>
>To clear any doubts, would it be possible to apply the GPLv2 the way
>the FSF recommends it?:
>http://www.gnu.org/licenses/old-licenses/gpl-2.0.en.html#SEC4
>Just make it "version 2 of the License"
>without the "or (at your option) any later version".
I’ve checked this matter with our legal department.
Since libvma package is dual licensed: GPLv2 or proprietary license, we think that the wording in the COPYING and in the sources files is correct and accurate.
Notice that this approach is common in the industry, see for example the license of MYSQL: http://www.mysql.com/about/legal/licensing/oem/

PS: I will send an updated package soon.

Thanks,
AlexV
Comment 19 Neil Horman 2016-03-07 16:39:55 EST
alex, I appreciate you looking into it, but I don't believe that the legal dual licensing is going to be acceptable for fedora packaging (or Red Hat for that matter).  I'm going to ask Red Hat Legal to clarify this but it seems to me that, while dual licensing is certainly a legal approach to take to open source code, both licenses must be open source compatible for us to pacakge the code in Fedora or RHEL.  That is to say, you may have a package that contains both BSD and GPLv2 licensed code, and thats fine, as long as you clearly delineate which code is licensed in which way.

However, you have a dual license situation in which code may be licensed under GPLv2 or some proprietary license.  While the former is ok, the latter is not distributable, and you have several files in here that are very ambiguous, as they do not specify which license they are under.  If they are all 100% GPLv2 licensed, then thats great, but its not a dual license situation then, its a single license, and the proprietary language needs to be removed.  If there proprietary code here, then thats a problem of a different sort.
Comment 20 Michal Schmidt 2016-03-08 07:51:52 EST
(In reply to Neil Horman from comment #19)
> alex, I appreciate you looking into it, but I don't believe that the legal
> dual licensing is going to be acceptable for fedora packaging (or Red Hat
> for that matter).  I'm going to ask Red Hat Legal to clarify this but it
> seems to me that, while dual licensing is certainly a legal approach to take
> to open source code, both licenses must be open source compatible for us to
> pacakge the code in Fedora or RHEL.

Neil,
I don't think the dual license (GPLv2 or proprietary) is a problem for redistribution, but it can be problem w.r.t. contributing patches to upstream.

The software is dual-licensed in the sense that the recipient can choose either one of the two licenses (GPLv2 or proprietary). So we can ignore the proprietary option and distribute the software under the terms of GPLv2. So far so good. Of course asking Legal is the right thing to do if you have any doubts.

The only problem I can see is if we (or any other Free Software developer) develop a patch and send it back to Mellanox. If we contribute a patch under GPLv2, Mellanox will likely refuse to merge it, because then they couldn't distribute the resulting work under the proprietary license.

Alex, how do you expect to handle patch contributions? Are you going to require copyright assignment agreements?
Comment 21 alexv 2016-03-08 08:38:39 EST
Thank you Neil and Michal for the comments.

After intensive consulting we agreed to change libvma license: to GPLv2 or BSD (dual license). 
I think that this change should resolve the discussed issue and any ambiguities related to licensing.

We are going to release a new libvma version with this change.
As soon as the new version is ready I will send an updated package for review.
Comment 22 Neil Horman 2016-03-08 10:27:07 EST
Michal, thats a question for legal to answer.  Dual Licensing can mean any number of things, including your interpretation, or others in which certain files are licensed exclusively under a specific license (DPDK does this, electing GPLv2 for some files, and BSD for others, and a proprietary license for yet others).

Fedora typically treats a dual license scenario on a per-file basis (each file selects which of the dual licenses applies to it).  Regardless, any non-open source compatible license requires legal approval to be packaged and distributed.

However, its a moot point, since Mellanox is being kind enough to fix the packaging to be dual licensed BSD and GPLv2, which solves the problem for us, as both of those are compatible.

alex, for your reference, here are the fedora license guidelines:
https://fedoraproject.org/wiki/Packaging:LicensingGuidelines?rd=Packaging/LicensingGuidelines

Should they help guide your conversion.  Thank you for taking the time to clear this additional hurdle.  Please post a new spec and srpm here when you have one available.
Comment 23 alexv 2016-03-08 10:54:23 EST
Thank you Neil for the pointer.

I will post the new spec and srpm as soon as they are ready.
Comment 24 alexv 2016-03-14 11:49:35 EDT
Hi Neil/Michal,

I've updated the package:

* Sun Mar 13 2016 Alex Vainman <alexv@mellanox.com> - 8.0.1-1
- New upstream release
- Move to dual license: GPLv2 or BSD
- ExcludeArch update
- Removal of extra space in:
  config(noreplace) {_sysconfdir}/security/limits.d/30-libvma-limits.conf
- Add V=1 to make

Spec URL: http://www.mellanox.com/downloads/Accelerator/libvma.spec
SRPM URL: http://www.mellanox.com/downloads/Accelerator/libvma-8.0.1-1.fc24.src.rpm
Comment 25 Neil Horman 2016-03-14 12:09:17 EDT
Ok, this looks good to me.  There are still two files in the tests subdirectory that claim to be proprietary licenses, but they don't seem to get included in the binary rpm, so I think thats ok.

ACK from me.

Avner, I presume you still need to get sponsored in the feodora system, correct?  I can do that for you.  Just to confirm avnerbh+mellanox@gmail.com is the email you registered for a fedora account with, correct?
Comment 26 Neil Horman 2016-03-15 10:58:42 EDT
ping, I need to know what fedora account system email/username you are using so that I can approve you as a pacakger
Comment 27 Ophir Munk 2016-03-15 16:08:00 EDT
Hi Neil,

Following are my accounts details

Fedora account
==============
Account Name: ophirmunk
Full Name: Ophir Munk
Email: ophirmu@mellanox.com

redhat.bugzilla.com
===================
Real Name: Ophir Munk
Email: ophirmu@mellanox.com

Please approve me as a packager
Comment 28 Neil Horman 2016-03-16 09:48:47 EDT
done, please complete the packaging process here:
https://fedoraproject.org/wiki/Package_Review_Process

To create the package in dist-git.
Comment 29 Gwyn Ciesla 2016-03-16 15:01:56 EDT
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/libvma
Comment 30 Honggang LI 2016-03-28 03:54:17 EDT
Checked with 'fedpkg clone libvma', package had been created in dist-git.
libvma-8.0.1-1.fc24/25 had been built.
http://koji.fedoraproject.org/koji/buildinfo?buildID=746017
http://koji.fedoraproject.org/koji/buildinfo?buildID=746001

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