Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 1438927 - Review Request: nanomsg - A fast, scalable, and easy to use socket library
Summary: Review Request: nanomsg - A fast, scalable, and easy to use socket library
Keywords:
Status: CLOSED WONTFIX
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 1123511 (view as bug list)
Depends On:
Blocks: FE-NEEDSPONSOR FE-DEADREVIEW
TreeView+ depends on / blocked
 
Reported: 2017-04-04 19:02 UTC by Tarjei Knapstad
Modified: 2023-09-12 01:15 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2018-06-18 08:21:52 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Tarjei Knapstad 2017-04-04 19:02:38 UTC
Spec URL: https://gist.github.com/tknapstad/6c8baca1678307acfbea59ad1b59da13
SRPM URL: https://copr.fedorainfracloud.org/coprs/tknapstad/nanomsg/
Description: nanomsg is a socket library that provides several common communication patterns. It aims to make the networking layer fast, scalable, and easy to use. Implemented in C, it works on a wide range of operating systems with no further dependencies.
Fedora Account System Username: tknapstad

This is a renewed review request based on a the work done in bug 1123511 which was never completed and is now closed. A stable 1.0.0 release of nanomsg has been released since then. I have modified the spec to build 1.0.0 and tried to correct the spec based on comments on the origianl request. I've successfully built the package in Copr and using the fedora-packager tools. The Epel7 build fails, but I don't quite understand why, so if anyone could look into that I would appreciate it. Spec changelog from the previous request is in the spec file.

Comment 1 Michael Schwendt 2017-04-05 11:50:22 UTC
*** Bug 1123511 has been marked as a duplicate of this bug. ***

Comment 2 Igor Gnatenko 2017-09-26 14:04:52 UTC
Hello,

unfortunately I can't sponsor you, but I will review your package and will try to help making it ready for inclusion into Fedora.

First thing is Spec/SRPM URLs, make sure that you point them to direct files, for example your spec url leads to html page with rendered spec file and srpm just shows copr repository. So you want to link https://gist.githubusercontent.com/tknapstad/6c8baca1678307acfbea59ad1b59da13/raw/7436f267b19197bb0ed627382aed977bced774cd/nanomsg.spec as a spec file and https://copr-be.cloud.fedoraproject.org/results/tknapstad/nanomsg/fedora-rawhide-x86_64/00535509-nanomsg/nanomsg-1.0.0-1.fc27.src.rpm as a srpm file. This is needed for fedora-review tool and also simplifies life for reviewer.

Now let's go through spec file, I will comment inline:

> Group:	 System Environment/Libraries
> BuildRoot: %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)
> %clean
No need to include this sections/tags, it's mostly needed for EL5 compatibility which I guess is not your case
See: https://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections

> Summary: A fast, scalable, and easy to use socket library
My personal preference is to remove such "A" or "The" since they don't make sense and just consuming space =) Probably there are guidelines about this, but I can't find them right now.

> Source0: https://github.com/nanomsg/nanomsg/archive/%{version}.tar.gz
Having tarball names like 1.0.0.tar.gz is not really useful, it is better to include name in there, so SourceURL could be like https://github.com/nanomsg/nanomsg/archive/%{version}/%{name}-%{version}.tar.gz
See: https://fedoraproject.org/wiki/Packaging:SourceURL#Git_Tags

> BuildRequires: rubygem-asciidoctor xmlto cmake
Personally I prefer each dependency to be on its own line because this simplifies git diffs a lot and makes rebasing / cherry-picking way easier.
Also you miss BuildRequires: gcc because nanomsg uses it to build =)
See: https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRequires

> %setup -q -n %{name}-%{version}
Note that "-n %{name}-%{version}" is default for %*setup macro, so you can safely remove this. Also I would recommend you to change whole line to "%autosetup" which also would apply patches automatically if you have them (P.S. if you will have problems with applying patches, you might want to add "-p1" after)

> make %{?_smp_mflags} V=1
You could replace (and I recommend to) this with %make_build (I don't think that V=1 is also needed because %cmake macro automatically adds -DCMAKE_VERBOSE_MAKEFILE:BOOL=ON to cmake invocation which makes make verbose)

> rm -fR %{buildroot}
This is not needed, buildroot is removed automatically in modern rpm versions

> make install DESTDIR="%{buildroot}"
You could replace (and I recommend to) this with "%make_install" which does absolutely same

> make test LD_LIBRARY_PATH="%{buildroot}%{_libdir}" DESTDIR="%{buildroot}"
I don't think that DESTDIR is required here..

> %defattr(-,root,root)
You don't need to declare this line because it is default for RPM
See: https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions

> %doc AUTHORS COPYING README
You should use %license for referencing licenses, so change this line to:
%license COPYING
%doc AUTHORS README
See: https://fedoraproject.org/wiki/Packaging:LicensingGuidelines

> %{_libdir}/*.so.*
> %{_libdir}/*.a*
> %{_mandir}/man7/*
> %{_mandir}/man3/*
> %{_includedir}/*
> %{_libdir}/*.so
> %{_libdir}/pkgconfig/*.pc
> %{_mandir}/man1/*
> %{_bindir}/*
I would recommend you changing this to reflect real names of files, it is hard to guess what files are there and might become problem when upstream adds new binary / file and you just don't notice this..

> %{_docdir}/%{name}
While this works on modern Fedoras, I would recommend you to change cmake definitions to not install it automatically and use %doc to install documentation. The reason is on (for example) RHEL, docdir is %{name}-%{version} while on Fedora it is %{name}.

---

Also you missed "Requires: %{name}%{?_isa} = %{version}-%{release}" in the "utils" subpackage, it links with libnanomsg.so and you need to keep subpackage in sync.

I also noticed that "static" bcond doesn't really work: File not found: /home/brain/rpmbuild/BUILDROOT/nanomsg-1.0.0-1.fc28.x86_64/usr/lib64/*.a*
And since we don't really want static libraries in Fedora, I would recommend you to drop bcond or at least fix it.

Another thing which needs to be fixed is pkg-config file having "libdir=${prefix}/lib" which on 64-bit platforms should have lib64 instead.

===

I hope you will find my review useful and I'm very sorry that no one reviewed your simple package for half year =(

Comment 3 Neal Gompa 2017-09-26 14:26:44 UTC
I can sponsor if Igor takes on reviewing.

Comment 4 Michael Schwendt 2017-09-26 20:25:04 UTC
> I don't think that V=1 is also needed

The older review request bug 1123511 included version 0.7.0 of the program, which wasn't using CMake. This new request is about 1.0.0.

Comment 5 Peter Robinson 2018-06-17 17:31:22 UTC
Related: I packaged up the now stable nng (nanomsg next generation) - rhbz 1592138

Comment 6 Pavel Zhukov 2018-06-18 07:12:14 UTC
I think this can be closed as reporter

Comment 7 Pavel Zhukov 2018-06-18 07:13:45 UTC
I think this can be closed as reporter is non responsible and we have new RR BZ#1592138

Comment 8 Tarjei Knapstad 2018-06-18 07:48:53 UTC
Sorry for not responding sooner, this took a backseat among a lot of other responsibilities.

I agree that this can be closed:

1. The author recommends transitioning to NNG from nanomsg
2. There are no packages in Fedora that requires nanomsg.

Comment 9 Red Hat Bugzilla 2023-09-12 01:15:07 UTC
The needinfo request[s] on this closed bug have been removed as they have been unresolved for 1000 days


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