Bug 1474033 - Review Request: ucx - Communication library implementing high-performance messaging
Review Request: ucx - Communication library implementing high-performance mes...
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Michal Schmidt
Fedora Extras Quality Assurance
https://github.com/openucx/ucx
:
Depends On:
Blocks: FE-NEEDSPONSOR
  Show dependency treegraph
 
Reported: 2017-07-23 05:43 EDT by Andrey Maslennikov
Modified: 2018-02-05 08:48 EST (History)
3 users (show)

See Also:
Fixed In Version: ucx-1.2.2-1.fc27
Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2018-02-05 08:48:56 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mschmidt: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Andrey Maslennikov 2017-07-23 05:43:04 EDT
Spec URL: https://gist.github.com/amaslenn/3c847e0bdc063bcbb4b6507b5efbf6b9/raw/ccae11efb3a573d77633e60b485f1f221cdd7e10/ucx.spec
SRPM URL: https://gist.github.com/amaslenn/3c847e0bdc063bcbb4b6507b5efbf6b9/raw/ccae11efb3a573d77633e60b485f1f221cdd7e10/ucx-1.3.3274-1.src.rpm

Description: UCX is a communication library implementing high-performance messaging.
Requires either RDMA-capable device, Cray Gemini or Aries, for inter-node
communication.
Future versions will support also TCP for inter-node, to lift that hardware
dependency.
In addition, the library can be used for intra-node communication by leveraging
the following shared memory mechanisms: posix. sysv, cma, knem, xpmem.

Fedora Account System Username: andreyma
That is my first package, so I need a sponsor.

Koji build: https://koji.fedoraproject.org/koji/tasks?state=all&owner=andreyma&view=tree&method=all&order=-id
Comment 1 Michael Schwendt 2017-07-23 09:54:04 EDT
> %global rel 1
> %global version 1.3.3274

Completely pointless definition and redefinition of macros for various reasons:

1) You define %rel only to use it once in the spec file.
2) You also use %release and not only %rel.
3) The "Release" tag implicitly defines %release, so both macros would be the same.
4) The "Version" tag implicitly defines %version. You redefine %version.

Further, the dist tag is missing: https://fedoraproject.org/wiki/Packaging:Versioning#Simple_versioning


> %global __check_files %{nil}

Comment/rationale missing!


> %bcond_with valgrind

No-op due to nothing related within the spec file.


> Summary: Unified Communication X

That's only what the UCX acronym stands for. The %description could explain that and expand on the summary, while the %summary could tell a bit more:

Summary: Communication framework for data centric and high-performance applications


> Group: Development/Libraries

No. The group for system runtime library packages is "System Environment/Libraries" for decades. On the contrary, "Development/Libraries" is for -devel packages, for example.


> Source: %{name}-%{version}.tar.gz

https://fedoraproject.org/wiki/Packaging:SourceURL#Referencing_Source


> ExclusiveArch: aarch64 ppc64le x86_64

https://fedoraproject.org/wiki/Packaging:Guidelines#Architecture_Support


> Requires(post): /sbin/ldconfig
> Requires(postun): /sbin/ldconfig

Implicit and automatic with /sbin/ldconfig scriptlets for a *very* long time.


> %description
> %description devel

Odd that the -devel package contains the more detailed description. The base package also contains more than libraries, lacking an explanation.


> %build
> ./contrib/configure-release \

That's a configure script for which you really want to use the %configure macro. See "rpm -E %configure" on what it does.


> mkdir -p %{buildroot}%{_sysconfdir}/ld.so.conf.d/
> echo %{_libdir} > %{buildroot}%{_sysconfdir}/ld.so.conf.d/ucx.conf

No, %_libdir is in the default search path list for runtime libs.


> %clean
> rm -rf %{buildroot}

https://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections


> %files
> %{_libdir}/lib*.so.*
> %{_bindir}/uc*
> %{_datadir}/ucx/perftest/*

https://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership


> %{_sysconfdir}/ld.so.conf.d/ucx.conf

Superfluous.


> %files devel
> %{_includedir}/uc*
> %{_libdir}/lib*.a

https://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries


> %changelog
> * Mon Jul 3 2017 Andrey Maslennikov <andreyma@mellanox.com> 1.3
> - Fedora package created

Not matching %version.


Please take a look at the fedora-review tool. Point it at this ticket via "fedora-review -b 1474033" and let it fetch the latest package files to help you with lots of automated reviewing tests.
Comment 2 Michael Schwendt 2017-07-23 09:57:00 EDT
> https://kojipkgs.fedoraproject.org//work/tasks/8693/20688693/build.log

Please look for ways to make build output verbose, so more of the compiler/linker calls and options can be seen in the build.log. You may need to disable .silent rules or execute Make with V=1, or enable other settings in the build framework.

https://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags
Comment 3 Andrey Maslennikov 2017-07-24 02:39:49 EDT
Thanks a lot for the review! I'll be back with fixes.
Comment 5 Andrey Maslennikov 2017-08-08 04:07:44 EDT
Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=21104781
Comment 6 Michael Schwendt 2017-08-09 03:08:02 EDT
For reasons unknown, you've missed various issues mentioned in comment 1. Since you haven't commented on any of the findings and since your spec %changelog is still wrong and hasn't been updated either, it's hard to tell what you've worked on. Please revisit comment 1.

Also please do take a look at running the fedora-review tool against this ticket.
Comment 7 Andrey Maslennikov 2017-08-09 06:43:18 EDT
Spec URL: https://gist.github.com/amaslenn/3c847e0bdc063bcbb4b6507b5efbf6b9/raw/9c7187b1aaa516030c08e3216675b4ff3145906d/ucx.spec
SRPM URL: https://gist.github.com/amaslenn/3c847e0bdc063bcbb4b6507b5efbf6b9/raw/9c7187b1aaa516030c08e3216675b4ff3145906d/ucx-1.2.0-1.fc25.src.rpm

Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=21125023

Please see details on what was done regarding your first review (sorry for not posting it with prev comment):
> > %global rel 1
> > %global version 1.3.3274
> 
> Completely pointless definition and redefinition of macros for various reasons:
>
> 1) You define %rel only to use it once in the spec file.
> 2) You also use %release and not only %rel.
> 3) The "Release" tag implicitly defines %release, so both macros would be the same.
> 4) The "Version" tag implicitly defines %version. You redefine %version.
>
> Further, the dist tag is missing: https://fedoraproject.org/wiki/Packaging:Versioning#Simple_versioning
Removed extra assignments, new code looks following:
Name: ucx
Version: 1.2.0
Release: 1%{?dist}


> > %global __check_files %{nil}
> 
> Comment/rationale missing!
Removed.


> > %bcond_with valgrind
> 
> No-op due to nothing related within the spec file.
Removed.


> > Summary: Unified Communication X
> 
> That's only what the UCX acronym stands for. The %description could explain that and expand on the > summary, while the %summary could tell a bit more:> 
> 
> Summary: Communication framework for data centric and high-performance applications
Summary/Description for both packages were updated.


> > Group: Development/Libraries
> 
> No. The group for system runtime library packages is "System Environment/Libraries" for decades. On the contrary, "Development/Libraries" is for -devel packages, for example.
Fixed.


> > Source: %{name}-%{version}.tar.gz
> 
> https://fedoraproject.org/wiki/Packaging:SourceURL#Referencing_Source
Fixed, new value is https://github.com/openucx/%{name}/archive/v1.2.0.tar.gz.


> > ExclusiveArch: aarch64 ppc64le x86_64
> 
> https://fedoraproject.org/wiki/Packaging:Guidelines#Architecture_Support
Added comment: "UCX currently supports only the following architectures".


> > Requires(post): /sbin/ldconfig
> > Requires(postun): /sbin/ldconfig
> 
> Implicit and automatic with /sbin/ldconfig scriptlets for a *very* long time.
Removed.


> > %description
> > %description devel
> 
> Odd that the -devel package contains the more detailed description. The base package also contains more than libraries, lacking an explanation.
Updated. Now -devel package contains only additional info.


> > %build
> > ./contrib/configure-release \
> 
> That's a configure script for which you really want to use the %configure macro. See "rpm -E %configure" on what it does.
Updated to use %configure.


> > mkdir -p %{buildroot}%{_sysconfdir}/ld.so.conf.d/
> > echo %{_libdir} > %{buildroot}%{_sysconfdir}/ld.so.conf.d/ucx.conf
> 
> No, %_libdir is in the default search path list for runtime libs.
Removed.


> > %clean
> > rm -rf %{buildroot}
> 
> https://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections
Removed.


> > %files
> > %{_libdir}/lib*.so.*
> > %{_bindir}/uc*
> > %{_datadir}/ucx/perftest/*
> 
> https://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership
Extra files are now removed in %install, fixed issue with file pattern. Is there anything else to fix here? Please also see below.


> > %{_sysconfdir}/ld.so.conf.d/ucx.conf
> 
> Superfluous.
Removed.


> > %files devel
> > %{_includedir}/uc*
> > %{_libdir}/lib*.a
> 
> https://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries
-devel package now has 'Provides: %{name}-static = %{version}-%{release}'.


> > %changelog
> > * Mon Jul 3 2017 Andrey Maslennikov <andreyma@mellanox.com> 1.3
> > - Fedora package created
> 
> Not matching %version.
Fixed.


> > https://kojipkgs.fedoraproject.org//work/tasks/8693/20688693/build.log
> 
> Please look for ways to make build output verbose, so more of the compiler/linker calls and options can be seen in the build.log. You may need to disable .silent rules or execute Make with V=1, or enable other settings in the build framework.
Added V=1 to make command.

Regarding fedora-review tool.
It reports "[!]: Package should not use obsolete m4 macros" complaining on AC_PROG_LIBTOOL. Is it critical and has to fixed?
Another error it reports is from rpmlint: binary-or-shlib-defines-rpath. Is there any other way to correctly specify the path for .so/executable files?
It also reports mismatch in sizes/checksums of the tarball, which is expected: current link is for prev release, we will create a new one (v1.2.1) once pass this review.
Other checks look good.
Comment 8 Michael Schwendt 2017-08-09 11:01:10 EDT
> %files
> %{_libdir}/lib*.so*

https://fedoraproject.org/wiki/Packaging:Guidelines#Devel_Packages


> %{_datadir}/ucx/perftest/*
> 
> https://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership
> Extra files are now removed in %install, fixed issue with file pattern.
> Is there anything else to fix here? Please also see below.

The issue here is that the directories /usr/share/ucx and /usr/share/ucs/perftest are not included in your packages. That's why I've linked the directory ownership guidelines.


> -devel package now has 'Provides: %{name}-static = %{version}-%{release}'.

It is as if you deliberately misread

  https://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries

because even if a "compelling reason" where given as why to include the static libs, they don't belong into the -devel package, if there are also shared libs.


> It reports "[!]: Package should not use obsolete m4 macros" complaining
> on AC_PROG_LIBTOOL. Is it critical and has to fixed?

That's not part of the review guidelines or packaging guidelines. The tool is trying to be helpful. In case it became necessary to regenerate the configure script during the build process, such as for a fix, obsolete macros would be problematic. It's something to fix upstream. Make sure you can autoreconf the source tarball on a recent installation of Fedora.


> Another error it reports is from rpmlint: binary-or-shlib-defines-rpath.
> Is there any other way to correctly specify the path for .so/executable files?

If check-rpaths during an official build complained about it, proceed as described at: https://fedoraproject.org/wiki/Packaging:Guidelines#Beware_of_Rpath


> It also reports mismatch in sizes/checksums of the tarball, which is
> expected: current link is for prev release, we will create a new one
> (v1.2.1) once pass this review.

That is completely *unexpected*. The SourceURL *must* link exactly the source archive that is included in the src.rpm.

https://fedoraproject.org/wiki/Packaging:ReviewGuidelines

MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. Reviewers should use sha256sum for this task as it is used by the sources file once imported into git. If no upstream URL can be specified for this package, please see the Source URL Guidelines for how to deal with this.
Comment 9 Andrey Maslennikov 2017-08-10 09:27:56 EDT
Spec URL: https://gist.github.com/amaslenn/3c847e0bdc063bcbb4b6507b5efbf6b9/raw/5be459ccf60ee116cc56296f72ecd38560961dee/ucx.spec
SRPM URL: https://gist.github.com/amaslenn/3c847e0bdc063bcbb4b6507b5efbf6b9/raw/5be459ccf60ee116cc56296f72ecd38560961dee/ucx-1.2.0-1.fc25.src.rpm

Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=21147884

Details:
> > %files
> > %{_libdir}/lib*.so*
> 
> https://fedoraproject.org/wiki/Packaging:Guidelines#Devel_Packages
Unversioned .so moved to -devel.


> > %{_datadir}/ucx/perftest/*
> > 
> > https://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership
> > Extra files are now removed in %install, fixed issue with file pattern.
> > Is there anything else to fix here? Please also see below.
> 
> The issue here is that the directories /usr/share/ucx and /usr/share/ucs/perftest are not included in your packages. That's why I've linked the directory ownership guidelines.
Fixed.


> > -devel package now has 'Provides: %{name}-static = %{version}-%{release}'.
> 
> It is as if you deliberately misread
> 
>   https://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries
> 
> because even if a "compelling reason" where given as why to include the static libs, they don't belong into the -devel package, if there are also shared libs.
Static libs moved to separate -static package. -devel depends on it.


> > It reports "[!]: Package should not use obsolete m4 macros" complaining
> > on AC_PROG_LIBTOOL. Is it critical and has to fixed?
> 
> That's not part of the review guidelines or packaging guidelines. The tool is trying to be helpful. In case it became necessary to regenerate the configure script during the build process, such as for a fix, obsolete macros would be problematic. It's something to fix upstream. Make sure you can autoreconf the source tarball on a recent installation of Fedora.
OK, thanks. autoreconf works on f25.


> > Another error it reports is from rpmlint: binary-or-shlib-defines-rpath.
> > Is there any other way to correctly specify the path for .so/executable files?
> 
> If check-rpaths during an official build complained about it, proceed as described at: https://fedoraproject.org/wiki/Packaging:Guidelines#Beware_of_Rpath
Couldn't reproduce it with fedora-review ran locally on spec/srpm. Added --disable-rpath to %configure anyway.


> > It also reports mismatch in sizes/checksums of the tarball, which is
> > expected: current link is for prev release, we will create a new one
> > (v1.2.1) once pass this review.
> 
> That is completely *unexpected*. The SourceURL *must* link exactly the source archive that is included in the src.rpm.
> 
> https://fedoraproject.org/wiki/Packaging:ReviewGuidelines
> 
> MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. Reviewers should use sha256sum for this task as it is used by the sources file once imported into git. If no upstream URL can be specified for this package, please see the Source URL Guidelines for how to deal with this.
Just want to have issues in spec fixed before creating actual release. Can we ignore this one for a while? Once others are resolved I'll fix it ASAP (will required creating a release).
Comment 10 Michael Schwendt 2017-08-12 08:04:23 EDT
> Static libs moved to separate -static package. -devel depends on it.

That is not what the guidelines suggest. Just think about it a bit. Such a dependency defeats the purpose of moving the static libs into a _separate_ package.


> Added --disable-rpath to %configure anyway.

That's a no-op, if the configure script doesn't support that option [yet]:

| configure: WARNING: unrecognized options: --disable-rpath


> %files
> ...
> %{_datadir}/ucx
> %exclude %{_datadir}/ucx/examples
> %{_datadir}/ucx/perftest

The first line in the quote above includes the complete %_datadir/ucx/ tree already minus the excluded directory. Hence the last line is superfluous.


> Just want to have issues in spec fixed before creating actual release.
> Can we ignore this one for a while?

You only need to keep in mind that replacing the source tarball with another release could be a rather disruptive step during review.


Btw, the build.log contains several warnings about libraries and tools (doxygen!) it cannot find.
Comment 11 Andrey Maslennikov 2017-08-14 09:14:36 EDT
Spec URL: https://gist.github.com/amaslenn/3c847e0bdc063bcbb4b6507b5efbf6b9/raw/09b525799964828c26eb6604a0404511ae231167/ucx.spec
SRPM URL: https://gist.github.com/amaslenn/3c847e0bdc063bcbb4b6507b5efbf6b9/raw/09b525799964828c26eb6604a0404511ae231167/ucx-1.2.0-1.fc25.src.rpm

Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=21224428


> > Static libs moved to separate -static package. -devel depends on it.
> 
> That is not what the guidelines suggest. Just think about it a bit. Such a dependency defeats the purpose of moving the static libs into a _separate_ package.
Just want to make it easier for users to get all packages required for development.
Will removing Required from -devel be enough to comply this policy?


> > Added --disable-rpath to %configure anyway.
> 
> That's a no-op, if the configure script doesn't support that option [yet]:
> 
> | configure: WARNING: unrecognized options: --disable-rpath
Missed that. Thanks, removed. Don't see this issue anyway.


> > %files
> > ...
> > %{_datadir}/ucx
> > %exclude %{_datadir}/ucx/examples
> > %{_datadir}/ucx/perftest
> 
> The first line in the quote above includes the complete %_datadir/ucx/ tree already minus the excluded directory. Hence the last line is superfluous.
Fixed, thanks.


> > Just want to have issues in spec fixed before creating actual release.
> > Can we ignore this one for a while?
> 
> You only need to keep in mind that replacing the source tarball with another release could be a rather disruptive step during review.
Don't like it either. Hope to fix it soon.


> Btw, the build.log contains several warnings about libraries and tools (doxygen!) it cannot find.
All of them (including doxygen) are "nice to have", not "required". The build is possible, it only reduces some functionality (which is OK). I added one (libibverbs-devel) to BuildRequires, this one is important.
Comment 12 Andrey Maslennikov 2017-08-17 06:27:54 EDT
Hello!
Any feedback on my last changes and question?
Comment 13 Michael Schwendt 2017-08-21 05:58:58 EDT
> Just want to make it easier for users to get all packages required
> for development.

It is a violation of the packaging guidelines, which want the static lib to be separated from the shared lib. The -static package may depend on the -devel package, however, to pull in the headers and license terms.

If you think the static lib is worth mentioning specifically, maybe give a hint in the %description of the -devel package. It currently only mentions headers, btw.


> Will removing Required from -devel be enough to comply this policy?

Yes, since it is the first case here:
https://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries_2
Comment 14 Andrey Maslennikov 2017-08-22 07:28:45 EDT
Thanks! I will work on release preparation to submit fully correct spec.
Comment 15 Andrey Maslennikov 2017-09-20 03:25:08 EDT
Hello.

Spec URL: https://gist.github.com/amaslenn/3c847e0bdc063bcbb4b6507b5efbf6b9/raw/47cc17411155ed5d7af6827af8feb2e238a2fc9a/ucx.spec
SRPM URL: https://gist.github.com/amaslenn/3c847e0bdc063bcbb4b6507b5efbf6b9/raw/47cc17411155ed5d7af6827af8feb2e238a2fc9a/ucx-1.2.2-1.fc25.src.rpm

Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=21979932

Changes in spec file.
Updated version:
-Version: 1.2.0
+Version: 1.2.2

New source link:
-Source: https://github.com/openucx/%{name}/archive/v1.2.0.tar.gz
+Source: https://github.com/amaslenn/%{name}/releases/download/v1.2.2/ucx-1.2.2.tar.gz

Default BuildRoot is required for old SLES:
+
+BuildRoot: %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)

Removed requirement of the -static from -devel
-Requires: %{name}-static = %{version}-%{release}


Added requirement of the -devel from -static
+Requires: %{name}-devel = %{version}-%{release}

Updated changelog:
+* Tue Sep 19 2017 Andrey Maslennikov <andreyma@mellanox.com> 1.2.2-1
+- Spec file: new Source link, set default BuildRoot
+* Mon Aug 21 2017 Andrey Maslennikov <andreyma@mellanox.com> 1.2.1-1
+- Spec file now complies with Fedora guidelines
 * Mon Jul 3 2017 Andrey Maslennikov <andreyma@mellanox.com> 1.2.0-1
 - Fedora package created

fedora-review tool reports only one error: "[!]: Package should not use obsolete m4 macros".

Source checksums
----------------
https://github.com/amaslenn/ucx/releases/download/v1.2.2/ucx-1.2.2.tar.gz :
  CHECKSUM(SHA256) this package     : 47a3fced54e602e7c7e34aecef3f7df970ffa2f4685196b509f906106d5e61f9
  CHECKSUM(SHA256) upstream package : 47a3fced54e602e7c7e34aecef3f7df970ffa2f4685196b509f906106d5e61f9
.tar.gz can be different if build on another system due to diff in configure and other tools.
Comment 16 Andrey Maslennikov 2017-09-28 02:50:48 EDT
Hello! Any feedback on my last changes?
Comment 17 Michal Schmidt 2017-10-10 11:33:05 EDT
(In reply to Andrey Maslennikov from comment #15)
> Spec URL:
> https://gist.github.com/amaslenn/3c847e0bdc063bcbb4b6507b5efbf6b9/raw/
> 47cc17411155ed5d7af6827af8feb2e238a2fc9a/ucx.spec

A few comments on the spec file:

> %{!?configure_options: %global configure_options %{nil}}

How do you intend to use this? It will always be nil when the package is built in the Fedora build system.

> Group: System Environment/Libraries
> [...]
> BuildRoot: %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)

"Group:" and "BuildRoot:" SHOULD NOT be used according to
http://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections

"SHOULD NOT" is not as strong as "MUST NOT", but sharing the same spec file between Fedora and SLES does not seem like a worthwhile goal in the long run. IMO it's not a valid reason for having these tags.

> BuildRequires: automake autoconf libtool

It seems you're only running ./configure in the %build step, not regenerating it. Does the build really require autotools?

> the following shared memory mechanisms: posix. sysv, cma, knem, xpmem.
Typo. This period should be a comma -----------^

> %package static

The packaging guidelines say: "In general, packagers are strongly encouraged not to ship static libs unless a compelling reason exists."
So a comment stating a reason for including the static libs would be nice.

> make %{?_smp_mflags} V=1

Just a minor suggestion: We have a macro called "make_build", defined as:
%{__make} -O %{?_smp_mflags}
It would be nice to use that macro:
%make_build V=1

> make DESTDIR=%{buildroot} install

There is a macro for this as well: %make_install

> %{!?_licensedir:%global license %%doc}

Is this to make the spec file work in older distros?

Other:
"[...] you must list a BuildRequires against gcc, gcc-c++ or clang"
http://fedoraproject.org/wiki/Packaging:C_and_C%2B%2B#BuildRequires_and_Requires

Please insert empty lines between %changelog entries.
Comment 18 Andrey Maslennikov 2017-10-11 08:48:27 EDT
>> %{!?configure_options: %global configure_options %{nil}}
> How do you intend to use this? It will always be nil when the package is built in the Fedora build system.
>> Group: System Environment/Libraries
>> [...]
>> BuildRoot: %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)
>
> "Group:" and "BuildRoot:" SHOULD NOT be used according to
> http://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections
>
> "SHOULD NOT" is not as strong as "MUST NOT", but sharing the same spec file between Fedora and SLES does >not seem like a worthwhile goal in the long run. IMO it's not a valid reason for having these tags.

Supporting several specs is more complicated especially taking into account that the diff will be in 1-2 lines only.
So I'd like to keep it this way, the line is required. It was proved by some fails in our build env :)


>> BuildRequires: automake autoconf libtool
> It seems you're only running ./configure in the %build step, not regenerating it. Does the build really require autotools?
Will check.

>>  the following shared memory mechanisms: posix. sysv, cma, knem, xpmem.
> Typo. This period should be a comma -----------^
Will fix, thanks.


>> %package static
> The packaging guidelines say: "In general, packagers are strongly encouraged not to ship static libs unless a compelling reason exists."
> So a comment stating a reason for including the static libs would be nice.
Will add a comment. In fact, static libs are simply required to develop with UCX.


>> make %{?_smp_mflags} V=1
> Just a minor suggestion: We have a macro called "make_build", defined as:
> %{__make} -O %{?_smp_mflags}
> It would be nice to use that macro:
> %make_build V=1
Will check if it works for as and switch if yes.


>> make DESTDIR=%{buildroot} install
> There is a macro for this as well: %make_install
Will update, thank you.


>> %{!?_licensedir:%global license %%doc}
> Is this to make the spec file work in older distros?
It is. Need a comment?


> Other:
> "[...] you must list a BuildRequires against gcc, gcc-c++ or clang"
> http://fedoraproject.org/wiki/Packaging:C_and_C%2B%2B#BuildRequires_and_Requires
Will add.


> Please insert empty lines between %changelog entries.
Will add.
Comment 19 Andrey Maslennikov 2017-10-11 10:03:53 EDT
Spec URL: https://gist.github.com/amaslenn/3c847e0bdc063bcbb4b6507b5efbf6b9/raw/6acd2f4a4a5a34f6915e225b80c34d5ec7daf915/ucx.spec
SRPM URL: https://gist.github.com/amaslenn/3c847e0bdc063bcbb4b6507b5efbf6b9/raw/6acd2f4a4a5a34f6915e225b80c34d5ec7daf915/ucx-1.2.2-1.fc25.src.rpm
Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=22382583

Changes:
1. automake/autoconf/libtool --> gcc
-BuildRequires: automake autoconf libtool
+BuildRequires: gcc

2. fix . --> ,
-the following shared memory mechanisms: posix. sysv, cma, knem, xpmem.
+the following shared memory mechanisms: posix, sysv, cma, knem, xpmem.

3. Add a comment on static
+# Static libs are required for user apps to use UCX

4. make --> %make_build
-make %{?_smp_mflags} V=1
+%make_build V=1

5. make install --> %make_install
 %install
-make DESTDIR=%{buildroot} install
+%make_install

6. Add empty lines, update date (release is the same)
-* Tue Sep 19 2017 Andrey Maslennikov <andreyma@mellanox.com> 1.2.2-1
+* Wed Oct 11 2017 Andrey Maslennikov <andreyma@mellanox.com> 1.2.2-1
 - Spec file: new Source link, set default BuildRoot
+
 * Mon Aug 21 2017 Andrey Maslennikov <andreyma@mellanox.com> 1.2.1-1
 - Spec file now complies with Fedora guidelines
+
Comment 20 Michal Schmidt 2017-10-11 11:40:41 EDT
(In reply to Andrey Maslennikov from comment #18)
> Supporting several specs is more complicated especially taking into account
> that the diff will be in 1-2 lines only.
> So I'd like to keep it this way, the line is required. It was proved by some
> fails in our build env :)

Of course if the build test was performed on a system with RPM < 4.6, then without explicit BuildRoot and Group tags it would fail. It's just that usually in Fedora spec files we do not care about such systems.

I'm not going to block the review for this, so keep the tags if you want them.

> Will add a comment. In fact, static libs are simply required to develop with
> UCX.

Do you mean it is impossible to develop with dynamic libraries? Where is the problem?


I am looking at the contents of the ucx and ucx-devel binary packages. The contents of the /usr/share/ucx directory look like documentation to me. Would you consider moving those files under /usr/share/doc/ucx? (That would be an upstream change, not just a packaging change.)
Comment 21 Andrey Maslennikov 2017-10-12 04:54:58 EDT
Spec URL: https://gist.github.com/amaslenn/3c847e0bdc063bcbb4b6507b5efbf6b9/raw/715fd6631f940fc84fea47890b2ebc8368d80e61/ucx.spec
SRPM URL: https://gist.github.com/amaslenn/3c847e0bdc063bcbb4b6507b5efbf6b9/raw/715fd6631f940fc84fea47890b2ebc8368d80e61/ucx-1.2.2-1.fc25.src.rpm
Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=22396615

Changes:
1. Updated comment regarding static libs. We want to support both static and dynamic linking, some of our customers use it.
-# Static libs are required for user apps to use UCX
+# UCX ships both static and dynamic libs to support different use-cases

2. /usr/share/ucx content moved to /usr/share/doc/ucx.
-%{_datadir}/ucx
-%exclude %{_datadir}/ucx/examples
 %doc README AUTHORS NEWS
+%{_datadir}/doc/ucx
+%exclude %{_datadir}/doc/ucx/examples
...
-%{_datadir}/ucx/examples
+%doc %{_datadir}/doc/ucx/examples

3. Date in changelog
-* Wed Oct 11 2017 Andrey Maslennikov <andreyma@mellanox.com> 1.2.2-1
+* Thu Oct 12 2017 Andrey Maslennikov <andreyma@mellanox.com> 1.2.2-1
Comment 22 Michal Schmidt 2017-10-12 12:20:26 EDT
> Source: https://github.com/amaslenn/%{name}/releases/download/v1.2.2/ucx-1.2.2.tar.gz

I see you respun the 1.2.2 tarball to move the documentation files.
Modifying previously released tarballs without updating the version number is confusing to your users and downstream distributors.
The github links on http://www.openucx.org suggest that the canonical upstream repository is https://github.com/openucx/ucx.
Perhaps the purpose of your personal fork (amaslenn/ucx) is to make the package pass this review and then you plan to merge the changes into the openucx/ucx repository and do a real (and no longer changing) 1.2.2 release?
Do you then intend to change the Source tag in the spec file to point to https://github.com/openucx/ucx?

> %{_datadir}/doc/ucx

Did you intentionally avoid using %{_pkgdocdir}? Is it because older distros do not define the macro? You could do what some other Fedora packages did for compatibility with EPEL 6:

%{!?_pkgdocdir: %global _pkgdocdir %{_docdir}/%{name}-%{version}}

> %doc %{_datadir}/doc/ucx/examples

I believe it is unnecessary to use the explicit "%doc" marking. RPM automatically marks files under _pkgdocdir as documentation.

I am not sure how safe it is to mix the usage of both %doc with relative paths (for README, etc.) and _docdir / _pkgdocdir. The guidelines forbid it if I'm reading them correctly:
https://fedoraproject.org/wiki/Packaging:Guidelines#Documentation
I see no obvious issue in the built packages, but maybe it could cause trouble with other versions of RPM. Better avoid the mixed usage by installing the files README, AUTHORS, NEWS into _pkgdocdir in the %install step instead of relying on %doc with relative paths.

> # UCX ships both static and dynamic libs to support different use-cases

I still don't get what the usecase is. Is it for performance reasons?


I see the sources include some unit tests. Have you considered running them in the %check step of the rpm build?
Comment 23 Andrey Maslennikov 2017-10-15 08:32:22 EDT
> Perhaps the purpose of your personal fork (amaslenn/ucx) is to make the package pass this review and then you plan to merge the changes into the openucx/ucx repository and do a real (and no longer changing) 1.2.2 release?
> Do you then intend to change the Source tag in the spec file to point to https://github.com/openucx/ucx?
That is exactly my purpose.


>> %{_datadir}/doc/ucx
> Did you intentionally avoid using %{_pkgdocdir}? Is it because older distros do not define the macro? You could do what some other Fedora packages did for compatibility with EPEL 6:
> %{!?_pkgdocdir: %global _pkgdocdir %{_docdir}/%{name}-%{version}}
Yes, I expect issue with old distros we support. Should I add this WA and use %{_pkgdocdir} instead?


>> %doc %{_datadir}/doc/ucx/examples
> I believe it is unnecessary to use the explicit "%doc" marking. RPM automatically marks files under _pkgdocdir as documentation.
Will move %{_datadir}/doc/ucx/examples out of %doc tag.


> I am not sure how safe it is to mix the usage of both %doc with relative paths (for README, etc.) and _docdir / _pkgdocdir. The guidelines forbid it if I'm reading them correctly:
> https://fedoraproject.org/wiki/Packaging:Guidelines#Documentation
> I see no obvious issue in the built packages, but maybe it could cause trouble with other versions of RPM. Better avoid the mixed usage by installing the files README, AUTHORS, NEWS into _pkgdocdir in the %install step instead of relying on %doc with relative paths.
Considering the previous paragraph, no issue here, right?


>> # UCX ships both static and dynamic libs to support different use-cases
> I still don't get what the usecase is. Is it for performance reasons?
Yes.


> I see the sources include some unit tests. Have you considered running them in the %check step of the rpm build?
Tests are HW dependent, so we won't use them in rpm build. Running tests is a separate stage of our CI, and release package won't be approved without all tests passing.
Comment 25 Michal Schmidt 2017-10-16 09:28:54 EDT
(In reply to Andrey Maslennikov from comment #23)
> >> %{_datadir}/doc/ucx
> > Did you intentionally avoid using %{_pkgdocdir}? Is it because older distros do not define the macro? You could do what some other Fedora packages did for compatibility with EPEL 6:
> > %{!?_pkgdocdir: %global _pkgdocdir %{_docdir}/%{name}-%{version}}
> Yes, I expect issue with old distros we support. Should I add this WA and
> use %{_pkgdocdir} instead?

Yes, please.

> >> %doc %{_datadir}/doc/ucx/examples
> > I believe it is unnecessary to use the explicit "%doc" marking. RPM automatically marks files under _pkgdocdir as documentation.
> Will move %{_datadir}/doc/ucx/examples out of %doc tag.

You still have:
 %doc %{_datadir}/doc/ucx/examples
in the "%files devel" section in the current version.
Here the %doc tag is simply redundant.

> > I am not sure how safe it is to mix the usage of both %doc with relative paths (for README, etc.) and _docdir / _pkgdocdir. The guidelines forbid it if I'm reading them correctly:
> > https://fedoraproject.org/wiki/Packaging:Guidelines#Documentation
> > I see no obvious issue in the built packages, but maybe it could cause trouble with other versions of RPM. Better avoid the mixed usage by installing the files README, AUTHORS, NEWS into _pkgdocdir in the %install step instead of relying on %doc with relative paths.
> Considering the previous paragraph, no issue here, right?

It is an issue. You have these two lines:

  %{_datadir}/doc/ucx
  %doc README AUTHORS NEWS

(The first line should be written as "%{_pkgdocdir}", but that does not change the argument.)

The first line uses the method of placing the documentation files in %_pkgdocdir.
The second line uses the method of letting RPM copy the files from %_builddir to %_pkgdocdir.
According to my reading of the guidelines, these two methods must not be both used in the same source package.

> >> # UCX ships both static and dynamic libs to support different use-cases
> > I still don't get what the usecase is. Is it for performance reasons?
> Yes.

OK, can you please mention "for performance" in the comment rather than the vague "to support different use-cases"?
Comment 26 Andrey Maslennikov 2017-10-17 10:39:57 EDT
Spec URL: https://gist.github.com/amaslenn/3c847e0bdc063bcbb4b6507b5efbf6b9/raw/fbf819b17c11fe75ce4f43b96a3c6feb56725469/ucx.spec
SRPM URL: https://gist.github.com/amaslenn/3c847e0bdc063bcbb4b6507b5efbf6b9/raw/fbf819b17c11fe75ce4f43b96a3c6feb56725469/ucx-1.2.2-1.fc25.src.rpm
Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=22504611

Changes:
1. Doc related: new macro usage, docs are installed by make
+%{!?_pkgdocdir: %global _pkgdocdir %{_docdir}/%{name}}

...

-%{_datadir}/doc/ucx
-%exclude %{_datadir}/doc/ucx/examples
-%doc README AUTHORS NEWS
+%{_pkgdocdir}
+%exclude %{_pkgdocdir}/examples

...

-%doc %{_datadir}/doc/ucx/examples
+%{_pkgdocdir}/examples

2. Comment
-# UCX ships both static and dynamic libs to support different use-cases
+# UCX ships both static and dynamic libs to support different use-cases like
+# performance benefits.
Comment 27 Michal Schmidt 2017-10-18 06:31:29 EDT
A license check highlighted a file with an LGPL license header:
  ucx-1.2.2/src/ucm/ptmalloc283/sysdeps/generic/malloc-machine.h
Works under BSD and LGPL licenses can be combined without conflict, so the LGPL is not a problem. Moreover, nothing from the ptmalloc283 directory is actually used. The build is configured to link with a bundled ptmalloc286 instead:
  ucx-1.2.2/src/ucm/ptmalloc286/

Why does ucx bundle not just one, but two versions of ptmalloc?

ptmalloc is also the implementation of malloc used in glibc. Would it be possible to just use glibc's malloc?

Please delete the unused version in the %prep step:
  rm -rf src/ucm/ptmalloc283/

If ptmalloc286 is there to stay, please add "Provides: bundled(ptmalloc) = 2.8.6" to the spec with an explanatory comment.
Comment 28 Michal Schmidt 2017-10-18 11:30:59 EDT
Notes about the license of src/ucs/datastruct/sglib.h:

  This is SGLIB version 1.0.3

  (C) by Marian Vittek, Bratislava, http://www.xref-tech.com/sglib, 2003-5

  License Conditions: You can use a verbatim copy (including this
  copyright notice) of sglib freely in any project, commercial or not.
  You can also use derivative forms freely under terms of Open Source
  Software license or under terms of GNU Public License.  If you need
  to use a derivative form in a commercial project, or you need sglib
  under any other license conditions, contact the author.

The first two conditions are sufficient to make the license OK for Fedora.
Third condition is a bit confusing. I think the word "commercial" in there stands for "not open source".
There is a clarification at http://sglib.sourceforge.net/#license :

  Basically, I only care that you do not remove the Copyright notice from
  the source code when using Sglib.

  More precisely: you can use Sglib or its derivative forms (under the
  condition that the Copyright notice is preserved) in any project, whether
  commercial or not, free of charges. In particular, you can use Sglib under
  the terms of any license defined as an open source license by the Open
  Source Initiative (see http://www.opensource.org/). This includes most
  common open source licenses such as the BSD license and GNU GPL.

  If you need to use sglib under any particular license conditions, contact
  the author.

OK, so it is a very permissive free software (meta-)license.

Also note that though the header says "version 1.0.3", the source file matches version 1.0.4 as available from https://sourceforge.net/projects/sglib/files/sglib/ (the sglib developer simply forgot to bump the version in the comment).

You may as well add:
  Provides: bundled(sglib) = 1.0.4
Comment 29 Andrey Maslennikov 2017-10-19 10:55:36 EDT
Spec URL: https://gist.github.com/amaslenn/3c847e0bdc063bcbb4b6507b5efbf6b9/raw/1b786df9e922d230ae0541e4e6e7515af3374104/ucx.spec
SRPM URL: https://gist.github.com/amaslenn/3c847e0bdc063bcbb4b6507b5efbf6b9/raw/1b786df9e922d230ae0541e4e6e7515af3374104/ucx-1.2.2-1.fc25.src.rpm
Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=22550529

Changes in spec:
1. Added Provides
+Provides: bundled(sglib) = 1.0.4
+Provides: bundled(ptmalloc) = 2.8.6
+

Changes in upstream:
1. Removed ptmalloc283
Comment 30 Michal Schmidt 2017-10-20 07:33:29 EDT
Package Review
==============
Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated

Issues:
=======
Nothing serious:
 - Fix missing license headers.
 - Add a comment explaining the bundling of ptmalloc.
 - Tweak Summary.
For upstream, eventually:
 - Add manpages
 - Look into the overlinking issue pointed out by rpmlint.
See my inline comments prefixed with ####.

===== 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.
[x]: Development (unversioned) .so files in -devel subpackage, if present.

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)", "MIT/X11 (BSD like)", "GPL", "Unknown or
     generated", "*No copyright* BSD (unspecified)". 502 files have unknown
     license. Detailed output of licensecheck in
     /var/tmp/1474033-ucx/licensecheck.txt
#### GPL is just libtool, which grants additional permissions. OK.
#### licensecheck fails to identify the license of most of the source files,
#### because their headers say after asserting the copyright:
####        See file LICENSE for terms.
#### That's OK, though suboptimal for licensecheck detection.
#### Some files contain just placeholders:
####  * $COPYRIGHT$
####  * $HEADER$
#### To me it's obvious that the LICENSE applies to them as well, but please
#### fix those to be on the safe side.
[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.
#### The Guidelines are not actually that strict anymore.
#### A justification for bundling ptmalloc would be welcome.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[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.
[-]: Package is not known to require an ExcludeArch tag.
#### ExcludeArch is justified. It can only build on archs explicitly supported
#### in src/ucs/arch/.
[-]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 71680 bytes in 10 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]: 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]: 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: ucx-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
#### For spec compatibility with older distros.
[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 ucx-
     devel , ucx-static , ucx-debuginfo
#### The -devel package uses an arch-specific dependency. The -static
#### package does not need it.
[?]: 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.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[-]: %check is present and all tests pass.
#### Tests require specific hardware. Not going to run in Koji.
[-]: Packages should try to preserve timestamps of original installed
     files.
[x]: Reviewer should test that the package builds in mock.
[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]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: The placement of pkgconfig(.pc) files are correct.
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[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.


Rpmlint
-------
Checking: ucx-1.2.2-1.fc26.x86_64.rpm
          ucx-devel-1.2.2-1.fc26.x86_64.rpm
          ucx-static-1.2.2-1.fc26.x86_64.rpm
          ucx-debuginfo-1.2.2-1.fc26.x86_64.rpm
          ucx-1.2.2-1.fc26.src.rpm
ucx.x86_64: W: name-repeated-in-summary C UCX
ucx.x86_64: W: spelling-error %description -l en_US intra -> intro, infra, intranet
ucx.x86_64: W: spelling-error %description -l en_US posix -> posit
ucx.x86_64: W: spelling-error %description -l en_US sysv -> sysop
ucx.x86_64: W: spelling-error %description -l en_US cma -> cam, cm, ca
ucx.x86_64: W: spelling-error %description -l en_US knem -> knee, knew, kn em
ucx.x86_64: W: spelling-error %description -l en_US xpmem -> Memphis
ucx.x86_64: W: no-manual-page-for-binary ucx_info
ucx.x86_64: W: no-manual-page-for-binary ucx_perftest
ucx.x86_64: W: no-manual-page-for-binary ucx_read_profile
ucx-devel.x86_64: W: only-non-binary-in-usr-lib
ucx-static.x86_64: W: no-documentation
ucx.src: W: name-repeated-in-summary C UCX
ucx.src: W: spelling-error %description -l en_US intra -> intro, infra, intranet
ucx.src: W: spelling-error %description -l en_US posix -> posit
ucx.src: W: spelling-error %description -l en_US sysv -> sysop
ucx.src: W: spelling-error %description -l en_US cma -> cam, cm, ca
ucx.src: W: spelling-error %description -l en_US knem -> knee, knew, kn em
ucx.src: W: spelling-error %description -l en_US xpmem -> Memphis
5 packages and 0 specfiles checked; 0 errors, 19 warnings.

#### Manpages would be nice.


Rpmlint (debuginfo)
-------------------
Checking: ucx-debuginfo-1.2.2-1.fc26.x86_64.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.





Rpmlint (installed packages)
----------------------------
sh: /usr/bin/python: No such file or directory
ucx-devel.x86_64: W: invalid-url URL: http://www.openucx.org <urlopen error [Errno -2] Name or service not known>
ucx-devel.x86_64: W: only-non-binary-in-usr-lib
ucx-static.x86_64: W: invalid-url URL: http://www.openucx.org <urlopen error [Errno -2] Name or service not known>
ucx-static.x86_64: W: no-documentation
ucx-debuginfo.x86_64: W: invalid-url URL: http://www.openucx.org <urlopen error [Errno -2] Name or service not known>
ucx.x86_64: W: name-repeated-in-summary C UCX

#### This is a good point. The name does not need to be repeated in Summary
#### of the main package.

ucx.x86_64: W: spelling-error %description -l en_US intra -> intro, infra, intranet
ucx.x86_64: W: spelling-error %description -l en_US posix -> posit
ucx.x86_64: W: spelling-error %description -l en_US sysv -> sysop
ucx.x86_64: W: spelling-error %description -l en_US cma -> cam, cm, ca
ucx.x86_64: W: spelling-error %description -l en_US knem -> knee, knew, kn em
ucx.x86_64: W: spelling-error %description -l en_US xpmem -> Memphis
ucx.x86_64: W: invalid-url URL: http://www.openucx.org <urlopen error [Errno -2] Name or service not known>
ucx.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libucm.so.0.0.0 /lib64/librt.so.1
ucx.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libucp.so.0.0.0 /lib64/libibverbs.so.1
ucx.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libucp.so.0.0.0 /lib64/libnuma.so.1
ucx.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libucp.so.0.0.0 /lib64/libm.so.6
ucx.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libucp.so.0.0.0 /lib64/libucm.so.0
ucx.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libucp.so.0.0.0 /lib64/librt.so.1
ucx.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libucp.so.0.0.0 /lib64/libdl.so.2
ucx.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libuct.so.0.0.0 /lib64/libnuma.so.1
ucx.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libuct.so.0.0.0 /lib64/libdl.so.2

#### Is this overlinking? You may want to look into this upstream.

ucx.x86_64: W: no-manual-page-for-binary ucx_info
ucx.x86_64: W: no-manual-page-for-binary ucx_perftest
ucx.x86_64: W: no-manual-page-for-binary ucx_read_profile
4 packages and 0 specfiles checked; 0 errors, 25 warnings.



Requires
--------
ucx-devel (rpmlib, GLIBC filtered):
    /usr/bin/pkg-config
    libucm.so.0()(64bit)
    libucp.so.0()(64bit)
    libucs.so.0()(64bit)
    libuct.so.0()(64bit)
    ucx(x86-64)

ucx-static (rpmlib, GLIBC filtered):
    ucx-devel

ucx-debuginfo (rpmlib, GLIBC filtered):

ucx (rpmlib, GLIBC filtered):
    /sbin/ldconfig
    libc.so.6()(64bit)
    libdl.so.2()(64bit)
    libgcc_s.so.1()(64bit)
    libgcc_s.so.1(GCC_3.3.1)(64bit)
    libgomp.so.1()(64bit)
    libgomp.so.1(GOMP_1.0)(64bit)
    libgomp.so.1(GOMP_4.0)(64bit)
    libgomp.so.1(OMP_1.0)(64bit)
    libibverbs.so.1()(64bit)
    libibverbs.so.1(IBVERBS_1.0)(64bit)
    libibverbs.so.1(IBVERBS_1.1)(64bit)
    libm.so.6()(64bit)
    libnuma.so.1()(64bit)
    libpthread.so.0()(64bit)
    librt.so.1()(64bit)
    libucm.so.0()(64bit)
    libucp.so.0()(64bit)
    libucs.so.0()(64bit)
    libuct.so.0()(64bit)
    rtld(GNU_HASH)



Provides
--------
ucx-devel:
    pkgconfig(ucx)
    ucx-devel
    ucx-devel(x86-64)

ucx-static:
    ucx-static
    ucx-static(x86-64)

ucx-debuginfo:
    ucx-debuginfo
    ucx-debuginfo(x86-64)

ucx:
    bundled(ptmalloc)
    bundled(sglib)
    libucm.so.0()(64bit)
    libucp.so.0()(64bit)
    libucs.so.0()(64bit)
    libuct.so.0()(64bit)
    ucx
    ucx(x86-64)



Source checksums
----------------
https://github.com/amaslenn/ucx/releases/download/v1.2.2/ucx-1.2.2.tar.gz :
  CHECKSUM(SHA256) this package     : 9f3ea6b3fd2a2941498aaa45a19df49517741949069bd2dc077a79419360ce23
  CHECKSUM(SHA256) upstream package : 9f3ea6b3fd2a2941498aaa45a19df49517741949069bd2dc077a79419360ce23


Generated by fedora-review 0.6.1 (f03e4e7) last change: 2016-05-02
Command line :/usr/bin/fedora-review -b 1474033
Buildroot used: fedora-26-x86_64
Active plugins: Generic, Shell-api, C/C++
Disabled plugins: Java, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP
Disabled flags: EXARCH, DISTTAG, EPEL5, BATCH, EPEL6
Comment 31 Andrey Maslennikov 2017-10-24 05:34:29 EDT
Spec URL: https://gist.github.com/amaslenn/3c847e0bdc063bcbb4b6507b5efbf6b9/raw/21ae461fbf824e175bf0e37c989cea4d9c6a99ce/ucx.spec
SRPM URL: https://gist.github.com/amaslenn/3c847e0bdc063bcbb4b6507b5efbf6b9/raw/21ae461fbf824e175bf0e37c989cea4d9c6a99ce/ucx-1.2.2-1.fc25.src.rpm
Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=22663647

Changes:
1.
-Summary: UCX is a communication library implementing high-performance messaging
+Summary: A communication library implementing high-performance messaging

2.
+# UCX doesn’t use glibc’s malloc because it is modifying ptmalloc library
+# to notify UCX about memory map/unmap events

Other comments:
> #### Manpages would be nice.
Currently, we don't have resources for it. Is it a blocker?

> #### Is this overlinking? You may want to look into this upstream.
Dependencies are invoked by `la_LIBADD`, it also include dependency's
dependencies.

License headers were fixed.
Comment 32 Michal Schmidt 2017-10-25 06:43:15 EDT
The missing manpages are not a blocker issue.

The package is approved.

I will sponsor you into the 'packager' group. Feel free to contact me by email or find me on IRC (my nick is 'michich') when you need help with Fedora work.
Comment 33 Andrey Maslennikov 2017-10-26 06:58:13 EDT
Thank you!

I will work on merging my changes to upstream and then push to Fedora's repo.
Comment 34 Gwyn Ciesla 2017-11-07 07:47:46 EST
(fedrepo-req-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/ucx
Comment 35 Michal Schmidt 2017-11-21 11:16:49 EST
Andrey,
were you able to get your changes merged to upstream?
Let me know if you need help on the Fedora side.
Comment 36 Andrey Maslennikov 2017-11-22 01:44:19 EST
Michal,
yes, my changes were merged. I'm waiting for the release to be created (otherwise spec file is misleading).
Thank you!
Comment 37 Michal Schmidt 2018-02-05 08:48:56 EST
ucx is in Rawhide, F27 updates, and batched for F26 updates. Closing this review.

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