Bug 892625
Summary: | Review Request: resiprocate - SIP reference implementation, SIP proxy, TURN server | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Daniel Pocock <daniel> |
Component: | Package Review | Assignee: | Peter Lemenkov <lemenkov> |
Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | unspecified | ||
Version: | rawhide | CC: | 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
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 Spec file has been renamed as per packaging guidelines: Spec URL: https://svn.resiprocate.org/viewsvn/resiprocate/main/resiprocate.spec.in?view=markup 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 I'll review it and I'll sponsor you as well. 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)? 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. 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. 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 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. 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? 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? 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. 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. (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 Unblocking FE-NEEDSPONSOR - I've just sponsored Daniel. 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 Git done (by process-git-requests). 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 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 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 resiprocate-1.8.7-1.fc19 has been pushed to the Fedora 19 testing repository. resiprocate-1.8.7-1.fc17 has been pushed to the Fedora 17 stable repository. resiprocate-1.8.7-1.fc18 has been pushed to the Fedora 18 stable repository. resiprocate-1.8.7-1.fc19 has been pushed to the Fedora 19 stable repository. |