Bug 892625

Summary: Review Request: resiprocate - SIP reference implementation, SIP proxy, TURN server
Product: [Fedora] Fedora Reporter: Daniel Pocock <daniel>
Component: Package ReviewAssignee: Peter Lemenkov <lemenkov>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: unspecified    
Version: rawhideCC: jorgeml, lemenkov, notting, package-review
Target Milestone: ---Flags: lemenkov: fedora-review+
pbabinca: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2013-05-01 03:27:50 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Daniel Pocock 2013-01-07 12:41:51 UTC
Spec URL: https://svn.resiprocate.org/viewsvn/resiprocate/main/resip.spec.in?view=markup
SRPM URL: http://www.resiprocate.org/ReSIProcate_1.8_Release
Description: SIP reference implementation (C++ shared lib), SIP proxy, ICE/STUN/TURN server
Fedora Account System Username: pocock

Please note: we are currently testing the 1.8.6 release, which has been tweaked to fix compile bugs on Fedora 17.  The source tree (1.8 release branch and trunk) also includes systemd artifacts in these locations now:

repro/pkg/fedora
reTurn/pkg/fedora

A package can be built from a tarball using the instructions here:

http://list.resiprocate.org/archive/resiprocate-devel/msg08155.html

Comment 1 Daniel Pocock 2013-01-07 12:45:02 UTC
Also, please note this is my first submission of a package for Fedora and a sponsor is needed.

I currently maintain packages in Debian and OpenCSW, including the Debian version of reSIProcate
http://www.pocock.com.au/open-source-software-projects

Comment 2 Daniel Pocock 2013-01-07 18:44:08 UTC
Spec file has been renamed as per packaging guidelines:

Spec URL: https://svn.resiprocate.org/viewsvn/resiprocate/main/resiprocate.spec.in?view=markup

Comment 3 Daniel Pocock 2013-01-09 19:29:57 UTC
reSIProcate 1.8.6 tarball has now been released.

The previous issue with rpmbuild -tb failing on resiprocate tarballs has been fixed - it appears that rpmbuild choked because a second spec file was included in the tarball (repro/repro.spec) - that has been removed.  Now it is possible to just do:

wget https://www.resiprocate.org/files/pub/reSIProcate/releases/resiprocate-1.8.6.tar.gz

rpmbuild -tb resiprocate-1.8.6.tar.gz

Comment 4 Peter Lemenkov 2013-01-11 07:17:46 UTC
I'll review it and I'll sponsor you as well.

Comment 5 Daniel Pocock 2013-01-11 08:29:57 UTC
Thanks, please feel free to reach out to me by email or xmpp if you need to.  

This will probably be the first TURN server in Fedora as well - what do you thing about jointly proposing an F19 feature for federated VoIP support (SIP + XMPP + TURN + ENUM)?

Comment 6 Daniel Pocock 2013-03-13 09:21:46 UTC
Peter, could you please add the comments you submitted over chat?

Do you think this package is potentially ready (or close to being ready) for F19?

WebRTC (actually, SIP over WebSockets) support is also coming in the next release, 1.9.0 most likely, but it could be a couple of months away.  That is a real game-changer, but it may be too late for F19.

Comment 7 Daniel Pocock 2013-03-13 22:28:09 UTC
Peter's initial feedback:

made a few changes to the resiprocate.spec.in

https://github.com/lemenkov/resiprocate/compare/d8ed117f61...529d33bb89

Most of them are applicable to other RPM-based distros as well, although some might be problematic. This one for example:

https://github.com/lemenkov/resiprocate/commit/0b9b94109:55:44

Here is a resulting spec-file

http://peter.fedorapeople.org/resiprocate.spec09:59:54

Builds perfectly fine in Rawhide

http://koji.fedoraproject.org/koji/taskinfo?taskID=4916758

Requesting updated spec-file from you.

Comment 8 Daniel Pocock 2013-03-14 07:52:15 UTC
I notice this change:

https://github.com/lemenkov/resiprocate/commit/81150e46e12c4426f00b1886431f62504dfad512

This was discussed in Debian.  The reSIProcate API (and often ABI) changes between releases.  Sadly, this is just the nature of developing code for a protocol like SIP.  So an application that compiles against 1.X won't always compile against 1.(X+1) without changes.

To make this easier, in Debian, we wanted to let people install multiple versions of the libs concurrently.

You will notice the version is also part of the SONAME (it is done with libtool):

http://lists.debian.org/debian-mentors/2009/07/msg00130.html

Comment 9 Peter Lemenkov 2013-03-14 13:02:57 UTC
Sorry for the delay. Ok, let's continue reviewing this.

Fresh Koji scratch-build for F-18:

* http://koji.fedoraproject.org/koji/taskinfo?taskID=5120911

REVIEW:

Legend: + = PASSED, - = FAILED, 0 = Not Applicable

- rpmlint is not silent and some of his messages must be addressed:

work ~/Desktop: rpmlint resiprocate-*
resiprocate.src: W: name-repeated-in-summary C Resiprocate

^^^ Not a critical but this clearly means that summary should be improved. Not a blocker.

resiprocate.src: W: invalid-url Source0: resiprocate-1.8.6.tar.gz

^^^ Should point to https://www.resiprocate.org/files/pub/reSIProcate/releases/resiprocate-1.8.6.tar.gz . Not a blocker but should be fixed.

resiprocate-b2bua.x86_64: W: summary-not-capitalized C basic SIP B2BUA

^^^ Not a blocker, but should be fixed as well.

resiprocate-b2bua.x86_64: W: spelling-error %description -l en_US repro -> retro, re pro, re-pro

^^^ false positive. Should be ignored.

resiprocate-b2bua.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/basicB2BUA ['/usr/lib64']

^^^ That should be removed. See this for the details - http://fedoraproject.org/wiki/Packaging:Guidelines#Beware_of_Rpath

resiprocate-b2bua.x86_64: W: no-documentation
resiprocate-b2bua.x86_64: W: no-manual-page-for-binary basicB2BUA

^^^ Not an issue.

resiprocate-clicktocall.x86_64: W: summary-not-capitalized C click-to-call server process

^^^ Not a blocker.

resiprocate-clicktocall.x86_64: W: spelling-error %description -l en_US repro -> retro, re pro, re-pro

^^^ false positive.

resiprocate-clicktocall.x86_64: E: binary-or-shlib-defines-rpath /usr/sbin/clicktocall ['/usr/lib64']

^^^ rpath issue - similar to one above.

resiprocate-clicktocall.x86_64: W: no-documentation
resiprocate-clicktocall.x86_64: W: no-manual-page-for-binary clicktocall
resiprocate-devel.x86_64: W: no-documentation

^^^ Not an issue, just another one friendly reminder.

resiprocate-libs.x86_64: W: summary-not-capitalized C shared libraries http://www.resiprocate.org

^^^ Should be fixed (not a blocker).

resiprocate-libs.x86_64: W: spelling-error %description -l en_US repro -> retro, re pro, re-pro
resiprocate-libs.x86_64: W: spelling-error %description -l en_US librutil -> librettist
resiprocate-libs.x86_64: W: spelling-error %description -l en_US libresip -> Libreville
resiprocate-libs.x86_64: W: spelling-error %description -l en_US libdum -> Librium
resiprocate-libs.x86_64: W: spelling-error %description -l en_US dialogs -> dialog, dialog s

^^^ false positives.

resiprocate-libs.x86_64: E: binary-or-shlib-defines-rpath /usr/lib64/libsipdial-1.8.so ['/usr/lib64']

^^^ rpath issue again.

resiprocate-libs.x86_64: W: shared-lib-calls-exit /usr/lib64/libb2bua-1.8.so exit.5

^^^ That's a bad architectural decision (shared libs shouldn't call exit but rather trow an exception/rerurn error code) but not an blocker issue. Think of this as of another friendly advice.

resiprocate-libs.x86_64: E: binary-or-shlib-defines-rpath /usr/lib64/libb2bua-1.8.so ['/usr/lib64']

^^^ rpath again.

resiprocate-libs.x86_64: W: no-documentation

^^^ Can be ignored. This package doesn't have any docs and that was intended.

resiprocate-presence-server.x86_64: W: summary-not-capitalized C basic SIP presence server

^^^ Should be fixed.

resiprocate-presence-server.x86_64: W: spelling-error %description -l en_US repro -> retro, re pro, re-pro


^^^ false positive.

resiprocate-presence-server.x86_64: E: binary-or-shlib-defines-rpath /usr/sbin/presSvr ['/usr/lib64']


^^^ rpath again.

resiprocate-presence-server.x86_64: W: no-documentation
resiprocate-presence-server.x86_64: W: no-manual-page-for-binary presSvr

^^^ another one friendly reminder.

resiprocate-repro.x86_64: E: binary-or-shlib-defines-rpath /usr/sbin/reprocmd ['/usr/lib64']
resiprocate-repro.x86_64: E: binary-or-shlib-defines-rpath /usr/sbin/repro ['/usr/lib64']

^^^ rpath. 

resiprocate-repro.x86_64: W: only-non-binary-in-usr-lib


^^^ false positive triggered by a systemd-related script.

resiprocate-repro.x86_64: W: non-standard-uid /var/lib/repro repro
resiprocate-repro.x86_64: W: non-standard-gid /var/lib/repro repro

^^^ Not an issue - this was intended.

resiprocate-sipdialer.x86_64: W: summary-not-capitalized C click-to-call utility

^^^ Looks like a copypasted leftover. Should be fixed.

resiprocate-sipdialer.x86_64: W: spelling-error %description -l en_US repro -> retro, re pro, re-pro

^^^ false positive.

resiprocate-sipdialer.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/sipdialer ['/usr/lib64']


^^^ rpath again - should be fixed.

resiprocate-turn-server.x86_64: W: spelling-error %description -l en_US reTurn -> re Turn, return, returns
resiprocate-turn-server.x86_64: W: spelling-error %description -l en_US standardised -> standardized, standardize, standard
resiprocate-turn-server.x86_64: W: spelling-error %description -l en_US repro -> retro, re pro, re-pro


^^^ false positive.

resiprocate-turn-server.x86_64: E: binary-or-shlib-defines-rpath /usr/sbin/reTurnServer ['/usr/lib64']

^^^ rpath again - should be fixed.

resiprocate-turn-server.x86_64: W: only-non-binary-in-usr-lib


^^^ false positive triggered by a systemd-related script.

10 packages and 0 specfiles checked; 9 errors, 32 warnings.
work ~/Desktop:


Ok, I see a number of issues and some of them should be fixed before final approval.

+ The package is named according to the  Package Naming Guidelines.
+ The spec file name matches the base package %{name}, in the format %{name}.spec.
+ The package meets the Packaging Guidelines except the issues already mentioned above.
+ The package is licensed with a Fedora approved license and meets the Licensing Guidelines.
+ The License field in the package spec file matches the actual license (Vovida).

- The file, containing the text of the license(s) for the package, MUST be included in %doc.

+ The spec file is written in American English.
+ The spec file for the package is legible.
+ The sources used to build the package, match the upstream source, as provided in the spec URL.

sulaco ~/rpmbuild/SOURCES: sha256sum resiprocate-1.8.6.tar.gz*
4dac3b7a17fa7842f5dbe994ba7cfef720f62b19781f8912f106ad6e3adfd5f8  resiprocate-1.8.6.tar.gz
4dac3b7a17fa7842f5dbe994ba7cfef720f62b19781f8912f106ad6e3adfd5f8  resiprocate-1.8.6.tar.gz.1
sulaco ~/rpmbuild/SOURCES:


+ The package successfully compiles and builds into binary rpms on at least one primary architecture. See koji link above.
+ All build dependencies are listed in BuildRequires.
0 No need to handle locales.
+ The package stores shared library files in some of the dynamic linker's default paths, and it calls ldconfig in %post and %postun.
+ The package does NOT bundle copies of system libraries.
0 The package is not designed to be relocatable.
+ The package owns all directories that it creates.
+ The package does not list a file more than once in the spec file's %files listings.
+ Permissions on files are set properly.
0 The package DOESN'T have a %clean section, so it won't build cleanly on systems with old rpm (EL-4 and EL-5). Beware.
+ The package consistently uses macros.
+ The package contains code, or permissible content.
0 No extremely large documentation files.
+ Anything, the package includes as %doc, does not affect the runtime of the application.
+ Header files are stored in a -devel package.
0 No static libraries.
0 No pkgconfig(.pc) files.
+ The library file(s) that end in .so (without suffix) is(are) stored in a -devel package.
+ The -devel package requires the lib package using a fully versioned dependency: Requires: %{name}-lib%{?_isa} = %{version}-%{release}
+ The package does NOT contain any .la libtool archives.
0 Not a GUI application.
+ The package does not own files or directories already owned by other packages.
0 At the beginning of %install, the package  does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) so it won't build cleanly on systems with old rpm (EL-4 and EL-5). Beware.
+ All filenames in rpm packages are valid UTF-8.


Ok, so here are some remaining issues:

- Remove rpath
- Fix capitalization of descriptions
- Fix sipdialer summary
- Add licensing info into package (mark it as %doc)
- Use a direct internel link to the source tarball

Please address/explain these issues and I'll finish review.

Comment 10 Daniel Pocock 2013-03-22 21:13:09 UTC
All issues except rpath appear quite trivial to fix (and I've just done them in trunk)

For the rpath issue:

* The configure.ac is relatively standard, no real hacks like some other projects
* I bootstrap the upstream releases on a Debian 6 (squeeze) system with the Debian autotools, that generates the configure script
* according to the Debian wiki about rpath, there are slight differences in the way libtool works in this scenario because Fedora has lib64 and Debian had lib32
* I suspect that running autoreconf will generate another configure script that is more Fedora friendly - is it safe to do that from within a spec file?  Have you seen this done elsewhere?


http://wiki.debian.org/RpathIssue

Can you also comment on the %doc issue?  Should I just include it in one (or all?) of the %files sections?  E.g.

%files libs
%doc COPYING
%{_libdir}/libb2bua-*.so
%{_libdir}/libdum-*.so
%{_libdir}/librepro-*.so
%{_libdir}/libresip-*.so
%{_libdir}/libreTurnClient-*.so
%{_libdir}/librutil-*.so
%{_libdir}/libsipdial-*.so

Or is there some way to tell it that file belongs in every package?

Comment 11 Daniel Pocock 2013-03-22 21:28:17 UTC

I found these comments on the autoreconf subject:

http://www.redhat.com/archives/libvir-list/2011-December/msg00230.html

http://www.redhat.com/archives/rhl-devel-list/2008-October/msg00866.html

and this seems to confirm my guess that it is a Debian/Fedora autotools mismatch issue:
http://lists.fedoraproject.org/pipermail/packaging/2010-June/007187.html

My own feeling is that:

a) wearing my upstream developer's hat, I'm not keen on different people re-running autoreconf and then asking for support, because they end up with different permutations of the Makefiles

b) but on the other hand, I'm the one who implement autotools for reSIProcate, and I tried to make the configure.ac and **/Makefile.am as conventional as possible (no special hacks) so they should be portable to newer/older autotools

Have you seen this issue before?  Do you think we should follow the suggestion from that first link, force autoreconf to run before configure?  Is there anywhere else where this should be discussed on the mailing list perhaps?

Comment 12 Daniel Pocock 2013-04-04 11:33:49 UTC
Discussion raised on the libtool and Fedora packaging lists:

http://lists.gnu.org/archive/html/libtool/2013-04/msg00000.html

http://lists.fedoraproject.org/pipermail/packaging/2013-April/009005.html


So far, the most probably immediate solution appears to be from this email
http://lists.fedoraproject.org/pipermail/packaging/2010-June/007187.html

------------------------------------------------------------------------
> sed -i.libdir_syssearch -e \
>    '/sys_lib_dlsearch_path_spec/s|/usr/lib |/usr/lib /usr/lib64 /lib /lib64
> |' \ configure
> ------------------------------------------------------------------------
>        i.e. just add the needed paths to sys_lib_dlsearch_path_spec in
>        configure (note that libtool in the build directory is generated by
>        configure) before calling %configure.

Comment 13 Daniel Pocock 2013-04-05 13:06:15 UTC
I tried the configure option --disable-rpath, it is not supported, it appears to be a custom option.

Therefore, I've implemented the hack with sed:

sed -i 's|^hardcode_libdir_flag_spec=.*|hardcode_libdir_flag_spec=""|g' libtool
sed -i 's|^runpath_var=LD_RUN_PATH|runpath_var=DIE_RPATH_DIE|g' libtool


and a new spec file is available:

These changes should be part of the official 1.8.7 upstream release too

http://svn.resiprocate.org/viewsvn/resiprocate/main/resiprocate.spec.in?view=markup

Please let me know if further changes are required.

Comment 14 Peter Lemenkov 2013-04-06 06:14:58 UTC
(In reply to comment #13)
> I tried the configure option --disable-rpath, it is not supported, it
> appears to be a custom option.
> 
> Therefore, I've implemented the hack with sed:
> 
> sed -i 's|^hardcode_libdir_flag_spec=.*|hardcode_libdir_flag_spec=""|g'
> libtool
> sed -i 's|^runpath_var=LD_RUN_PATH|runpath_var=DIE_RPATH_DIE|g' libtool
> 
> 
> and a new spec file is available:
> 
> These changes should be part of the official 1.8.7 upstream release too
> 
> http://svn.resiprocate.org/viewsvn/resiprocate/main/resiprocate.spec.
> in?view=markup
> 
> Please let me know if further changes are required.

Hello Daniel, and sorry for leaving you alone again. Yes, I don't see any other issues, so this package is 



APPROVED.



ps proceed as described here:
* https://fedoraproject.org/wiki/Package_SCM_admin_requests#New_Packages

Comment 15 Peter Lemenkov 2013-04-06 06:18:43 UTC
Unblocking FE-NEEDSPONSOR - I've just sponsored Daniel.

Comment 16 Daniel Pocock 2013-04-06 17:04:27 UTC
New Package SCM Request
=======================
Package Name: resiprocate
Short Description: SIP reference implementation, SIP proxy, TURN server
Owners: pocock
Branches: f17 f18 f19 el6
InitialCC: peter

Comment 17 Pavol Babinčák 2013-04-08 08:56:33 UTC
Git done (by process-git-requests).

Comment 18 Fedora Update System 2013-04-19 06:44:19 UTC
resiprocate-1.8.7-1.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/resiprocate-1.8.7-1.fc18

Comment 19 Fedora Update System 2013-04-19 06:44:37 UTC
resiprocate-1.8.7-1.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/resiprocate-1.8.7-1.fc17

Comment 20 Fedora Update System 2013-04-19 06:44:47 UTC
resiprocate-1.8.7-1.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/resiprocate-1.8.7-1.fc19

Comment 21 Fedora Update System 2013-04-19 16:51:22 UTC
resiprocate-1.8.7-1.fc19 has been pushed to the Fedora 19 testing repository.

Comment 22 Fedora Update System 2013-05-01 03:27:52 UTC
resiprocate-1.8.7-1.fc17 has been pushed to the Fedora 17 stable repository.

Comment 23 Fedora Update System 2013-05-01 03:36:29 UTC
resiprocate-1.8.7-1.fc18 has been pushed to the Fedora 18 stable repository.

Comment 24 Fedora Update System 2013-05-01 04:26:13 UTC
resiprocate-1.8.7-1.fc19 has been pushed to the Fedora 19 stable repository.