[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033

Michal Schmidt  changed:

   What|Removed |Added

 Status|ASSIGNED|CLOSED
   Fixed In Version||ucx-1.2.2-1.fc27
 Resolution|--- |CURRENTRELEASE
Last Closed||2018-02-05 08:48:56



--- Comment #37 from Michal Schmidt  ---
ucx is in Rawhide, F27 updates, and batched for F26 updates. Closing this
review.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033



--- Comment #36 from Andrey Maslennikov  ---
Michal,
yes, my changes were merged. I'm waiting for the release to be created
(otherwise spec file is misleading).
Thank you!

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033



--- Comment #35 from Michal Schmidt  ---
Andrey,
were you able to get your changes merged to upstream?
Let me know if you need help on the Fedora side.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033



--- Comment #34 from Gwyn Ciesla  ---
(fedrepo-req-admin):  The Pagure repository was created at
https://src.fedoraproject.org/rpms/ucx

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033



--- Comment #33 from Andrey Maslennikov  ---
Thank you!

I will work on merging my changes to upstream and then push to Fedora's repo.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033

Michal Schmidt  changed:

   What|Removed |Added

  Flags|fedora-review?  |fedora-review+



--- Comment #32 from Michal Schmidt  ---
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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033



--- Comment #31 from Andrey Maslennikov  ---
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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033



--- Comment #30 from Michal Schmidt  ---
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
 

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033



--- Comment #29 from Andrey Maslennikov  ---
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

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033



--- Comment #28 from Michal Schmidt  ---
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

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033



--- Comment #27 from Michal Schmidt  ---
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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033



--- Comment #26 from Andrey Maslennikov  ---
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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033



--- Comment #25 from Michal Schmidt  ---
(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"?

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033



--- Comment #24 from Andrey Maslennikov  ---
Spec URL:
https://gist.github.com/amaslenn/3c847e0bdc063bcbb4b6507b5efbf6b9/raw/4a3cc83297b579f2ab89fbe6c804a1b858449394/ucx.spec
SRPM URL:
https://gist.github.com/amaslenn/3c847e0bdc063bcbb4b6507b5efbf6b9/raw/4a3cc83297b579f2ab89fbe6c804a1b858449394/ucx-1.2.2-1.fc25.src.rpm
Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=22481063

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

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033



--- Comment #23 from Andrey Maslennikov  ---
> 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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033



--- Comment #22 from Michal Schmidt  ---
> 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?

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033



--- Comment #21 from Andrey Maslennikov  ---
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  1.2.2-1
+* Thu Oct 12 2017 Andrey Maslennikov  1.2.2-1

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033



--- Comment #20 from Michal Schmidt  ---
(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.)

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033



--- Comment #19 from Andrey Maslennikov  ---
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  1.2.2-1
+* Wed Oct 11 2017 Andrey Maslennikov  1.2.2-1
 - Spec file: new Source link, set default BuildRoot
+
 * Mon Aug 21 2017 Andrey Maslennikov  1.2.1-1
 - Spec file now complies with Fedora guidelines
+

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033



--- Comment #18 from Andrey Maslennikov  ---
>> %{!?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}-XX)
>
> "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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033

Michal Schmidt  changed:

   What|Removed |Added

  Flags||fedora-review?



-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033

Michal Schmidt  changed:

   What|Removed |Added

 Status|NEW |ASSIGNED
   Assignee|nob...@fedoraproject.org|mschm...@redhat.com



-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033

Michal Schmidt  changed:

   What|Removed |Added

 CC||mschm...@redhat.com



--- Comment #17 from Michal Schmidt  ---
(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}-XX)

"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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033



--- Comment #16 from Andrey Maslennikov  ---
Hello! Any feedback on my last changes?

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033



--- Comment #15 from Andrey Maslennikov  ---
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}-XX)

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  1.2.2-1
+- Spec file: new Source link, set default BuildRoot
+* Mon Aug 21 2017 Andrey Maslennikov  1.2.1-1
+- Spec file now complies with Fedora guidelines
 * Mon Jul 3 2017 Andrey Maslennikov  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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033



--- Comment #14 from Andrey Maslennikov  ---
Thanks! I will work on release preparation to submit fully correct spec.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033



--- Comment #13 from Michael Schwendt  ---
> 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

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033



--- Comment #12 from Andrey Maslennikov  ---
Hello!
Any feedback on my last changes and question?

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033



--- Comment #11 from Andrey Maslennikov  ---
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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033



--- Comment #10 from Michael Schwendt  ---
> 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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033



--- Comment #9 from Andrey Maslennikov  ---
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).

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033



--- Comment #8 from Michael Schwendt  ---
> %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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033



--- Comment #7 from Andrey Maslennikov  ---
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  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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about 

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033



--- Comment #6 from Michael Schwendt  ---
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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033



--- Comment #5 from Andrey Maslennikov  ---
Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=21104781

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033



--- Comment #4 from Andrey Maslennikov  ---
Spec URL:
https://gist.github.com/amaslenn/3c847e0bdc063bcbb4b6507b5efbf6b9/raw/708923dcd5342351cda488fa51cf296091abfac1/ucx.spec
SRPM URL:
https://gist.github.com/amaslenn/3c847e0bdc063bcbb4b6507b5efbf6b9/raw/708923dcd5342351cda488fa51cf296091abfac1/ucx-1.2.0-1.el7.src.rpm

Please review.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033



--- Comment #3 from Andrey Maslennikov  ---
Thanks a lot for the review! I'll be back with fixes.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033



--- Comment #2 from Michael Schwendt  ---
> 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

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033



--- Comment #1 from Michael Schwendt  ---
> %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  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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1474033] Review Request: ucx - Communication library implementing high-performance messaging

[bugzilla]

https://bugzilla.redhat.com/show_bug.cgi?id=1474033

Andrey Maslennikov  changed:

   What|Removed |Added

URL||https://github.com/openucx/
   ||ucx
 CC||andre...@mellanox.com
 Blocks||177841 (FE-NEEDSPONSOR)




Referenced Bugs:

https://bugzilla.redhat.com/show_bug.cgi?id=177841
[Bug 177841] Tracker: Review requests from new Fedora packagers who need a
sponsor
-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org