Bug 499137 - Review Request: sipwitch - SIP telephony server for secure phone systems
Review Request: sipwitch - SIP telephony server for secure phone systems
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Extras Quality Assurance
:
Depends On: 498736
Blocks:
  Show dependency treegraph
 
Reported: 2009-05-05 07:14 EDT by David Sugar
Modified: 2009-05-18 22:08 EDT (History)
4 users (show)

See Also:
Fixed In Version: 0.5.4-3.fc10
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-05-16 09:50:33 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mtasaka: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
rpmlint on x86_64 (after fixing rpath and so on) (1.80 KB, text/plain)
2009-05-07 14:29 EDT, Mamoru TASAKA
no flags Details

  None (edit)
Description David Sugar 2009-05-05 07:14:26 EDT
Spec URL: http://www.gnutelephony.org/specs/sipwitch.spec
SRPM URL: http://www.gnutelephony.org/specs/sipwitch-0.5.4-0.src.rpm
Description: GNU SIP Witch is a pure SIP-based office telephone call server that supports generic phone system features like call forwarding, hunt groups and call distribution, call coverage and ring groups, holding, and call transfer, as well as offering SIP specific capabilities such as presence and messaging. It supports secure telephone extensions for making calls over the Internet, and intercept/decrypt-free peer-to-peer audio and video extensions. It is not a SIP proxy, a multi-protocol telephone server, or an IP-PBX, and does not try to emulate Asterisk, FreeSWITCH, or Yate.

WARNING: This is a rather complex package, it includes plugins, python and php swig.  Many different things and packaging issues are found within!
Comment 1 Mamoru TASAKA 2009-05-06 14:08:17 EDT
From very quick glance at your spec file:

- Fedora suggests that one line in %description must
  not have more than 79 characters
  (please check rpmlint warnings)
- Use macros consistently.
----------------------------------------------------
    31  Requires: %{name} = %{version}-%{release}
    36  Requires: sipwitch = %{version}-%{release}
----------------------------------------------------
- Would you explain why some of the subpackage does not
  have the dependency for main package?
- %{buildroot} is missing:
----------------------------------------------------
%{__rm} -f %{_libdir}/*.la
%{__rm} -f %{_libdir}/sipwitch/*.la
----------------------------------------------------
- Perhaps "INSTALL" is not needed for rpm document files.
- Files under %_mandir are automatically marked as %doc.
- The main package must not own the directory %_sbindir,
  %_bindir themselves. 
- For initscripts related convention, 
  * please follow
    https://fedoraproject.org/wiki/Packaging/SysVInitScript#Initscripts_on_the_filesystem
    https://fedoraproject.org/wiki/Packaging/SysVInitScript#Initscript_packaging
  From rpmlint:
  * service should not be enabled by default (i.e. change
-----------------------------------------------------
# chkconfig: 2345 95 15
-----------------------------------------------------
    to
-----------------------------------------------------
# chkconfig: - 95 15
-----------------------------------------------------
  * init script should have "status" entry
    (please also check "$ rpmlint -I no-status-entry")
  * init script should put a lock file in
    %_localstatedir/lock/subsys .
- {_datadir}/snmp{,/mibs} is not owned by the main package
  but it installs some files under these directories.
  These directories are owned by net-snmp-libs, currently this
  package (sipwitch) does not seem to require net-snmp-libs.
  Would you check if sipwitch should require net-snmp-libs?
  (or examine why some files are installed under %_datadir/snmp?)
- Using %_libdir/python* is wrong (by the way this does not
  seem to work on 64 bit architecture). Please follow
  https://fedoraproject.org/wiki/Packaging/Python#System_Architecture
- Because of some reasons (one of the reasons is to avoid
  selinux AVC denial), we always install byte-compiled .py{o,c}
  files altogether (note that these .py{o,c} files are automatically
  created by /usr/lib/rpm/brp-python-bytecompile).
  So
  - these files must appear in %files list (using sipwitch.py*
    is easier)
  - creating/removing byte-compiled python files in scriptlets
    (i.e. at %post or so) should be removed.
- Usually configuration files should be marked as
  %config(noreplace)
  https://fedoraproject.org/wiki/Packaging/Guidelines#Configuration_files
- When only /sbin/ldconfig is called on a scriptlet, use -p
  option to avoid unneeded shell call, like:
---------------------------------------------------------
%postun -p /sbin/ldconfig
---------------------------------------------------------
  (however also please check initscripts scriptlets convention)
Comment 2 David Sugar 2009-05-06 23:28:39 EDT
To do this I will also had to add a patch until changes make it into the next upstream release...but I have a question, on net-snmp.  We are supplying a valid snmp MIB, and it really does belong in the mib directory.  So I created sipwitch-snmp as a sub-package.

Updated Spec: http://www.gnutelephony.org/specs/sipwitch.spec
Current Patch: http://www.gnutelephony.org/specs/sipwitch.patch
Updated SRPM: http://www.gnutelephony.org/specs/sipwitch-0.5.4-1.src.rpm
Comment 3 Mamoru TASAKA 2009-05-07 14:29:45 EDT
Created attachment 342909 [details]
rpmlint on x86_64 (after fixing rpath and so on)

For 0.5.4-1:

* BR
  - "BR: pkgconfig" is not needed because ucommon-devel
    has "Requiers: pkgconfig"
    (Note that any packages containing pkgconfig .pc file
    should have "Requires: pkgconfig")
  - By the way would you check if some version specific
    dependency for BuildRequires (not Requires) is really
    needed?

* Internal dependency
  - Is it safe that sipwitch-snmp subpackage does not depend
    on sipwitch?

* Patch0 vs %patch
  - On rawhide %patch is rejected when you use "Patch0:".
    You should use "Patch: <-> %patch" or "Patch0: <-> %patch0".

* rpath
  Ref:
  https://fedoraproject.org/wiki/Packaging/Guidelines#Beware_of_Rpath
  - On x86_64 the rebuilt binaries have unneeded rpath:
-------------------------------------------------------------
sipwitch-plugin-forward.x86_64: E: binary-or-shlib-defines-rpath /usr/lib64/sipwitch/forward.so ['/usr/lib64']
sipwitch-plugin-rtpproxy.x86_64:        E: binary-or-shlib-defines-rpath /usr/lib64/sipwitch/rtpproxy.so ['/usr/lib64']
sipwitch-plugin-scripting.x86_64:       E: binary-or-shlib-defines-rpath /usr/lib64/sipwitch/scripting.so ['/usr/lib64']
sipwitch-plugin-subscriber.x86_64:      E: binary-or-shlib-defines-rpath /usr/lib64/sipwitch/subscriber.so ['/usr/lib64']
sipwitch-plugin-zeroconf.x86_64:        E: binary-or-shlib-defines-rpath /usr/lib64/sipwitch/zeroconf.so ['/usr/lib64']
-------------------------------------------------------------
    Fedora requests to remove these rpaths.
    This can be removed by the below (I usually do the following)
-------------------------------------------------------------
%prep
%setup -q
%patch -p0
sed -i.rpath -e \
  '/sys_lib_dlsearch_path_spec/s|/usr/lib |/usr/lib /usr/lib64 /lib /lib64 |' \
  configure
....
....
%build
....
%configure \
....
-------------------------------------------------------------
    (note that the way recommended in the wiki often breaks
    linkage against libraries rebuilt from the same source)

* stripping binaries
  - Some of the binaries are stripped:
-------------------------------------------------------------
  1660  DEBUG: + /usr/bin/make DESTDIR=/builddir/build/BUILDROOT/sipwitch-0.5.4-1.1.fc11.x86_64 'INSTALL=install -p' swig-python
.....
  1685  DEBUG: g++ -pthread -shared -lc -Wno-variadic-macros -Wno-long-long -fexceptions -DNEW_STDCPP -pthread -fno-check-new -finline -fvisibility=hidden -DUCOMMON_VISIBILITY=1 -I/builddir/build/BUILD/sipwitch-0.5.4/inc -I/builddir/build/BUILD/sipwitch-0.5.4 build/temp.linux-x86_64-2.6/wrapper.o -L/usr/lib64 -lpython2.6 -lucommon -o _sipwitch.so
  1686  DEBUG: /bin/sh /builddir/build/BUILD/sipwitch-0.5.4/autoconf/install-sh -d /builddir/build/BUILDROOT/sipwitch-0.5.4-1.1.fc11.x86_64`python -c 'from distutils.sysconfig import get_python_lib; print get_python_lib()'`
  1687  DEBUG: /bin/sh /builddir/build/BUILD/sipwitch-0.5.4/autoconf/install-sh -d /builddir/build/BUILDROOT/sipwitch-0.5.4-1.1.fc11.x86_64`python -c 'from distutils.sysconfig import get_python_lib; print get_python_lib(1)'`
  1688  DEBUG: strip _sipwitch.so
.....
  1692  DEBUG: + /usr/bin/make DESTDIR=/builddir/build/BUILDROOT/sipwitch-0.5.4-1.1.fc11.x86_64 'INSTALL=install -p' swig-php5
  1698  DEBUG: g++ -shared -module -shared -avoid-version -o sipwitch.so wrapper.o -lucommon -lc
  1699  DEBUG: /bin/sh /builddir/build/BUILD/sipwitch-0.5.4/autoconf/install-sh -d /builddir/build/BUILDROOT/sipwitch-0.5.4-1.1.fc11.x86_64`php-config --extension-dir`
  1700  DEBUG: strip sipwitch.so
-------------------------------------------------------------
    To create debuginfo rpm correctly, binaries must not be stripped
    before %install ends. ref:
    https://fedoraproject.org/wiki/Packaging/Debuginfo
    For this package, the following will prevent this strip:
-------------------------------------------------------------
%build
export STRIP=/bin/true
%configure --with-pkg-config --disable-static
%{__make} %{?_smp_mflags} 
-------------------------------------------------------------

* Automated autotools call
  - At some point autotools are automatically called after configure ->
    make is executed:
-------------------------------------------------------------
  1660  DEBUG: + /usr/bin/make DESTDIR=/builddir/build/BUILDROOT/sipwitch-0.5.4-1.1.fc11.x86_64 'INSTALL=install -p' swig-python
  1661  DEBUG: (cd swig ; make python-swig)
  1662  DEBUG: make[1]: Entering directory `/builddir/build/BUILD/sipwitch-0.5.4/swig'
  1663  DEBUG:  cd .. && /bin/sh /builddir/build/BUILD/sipwitch-0.5.4/autoconf/missing --run automake-1.10 --gnu  swig/Makefile
  1664  DEBUG:  cd .. && /bin/sh ./config.status swig/Makefile 
  1665  DEBUG: config.status: creating swig/Makefile
-------------------------------------------------------------
    This modifies autotools-generated files and this must be done
    before configure/make is executed.
    For this package, this is because your patch modifies only
    swig/Makefile.am but does not modify swig/Makefile.in.
    Solution:
    - Patch against swig/Makefile.in, not against swig/Makefile.am
      This is recommended to avoid unneeded autotools call.
    - If you want to call autotools anyway, explicitly call autotools
      before configure/make is executed. However this is not
      recommended and should be avoided when possible.

* %config(noreplace)
  - Would you explain why %{_sysconfdir}/logrotate.d/sipwitch is not marked
    as (noreplace)?

* rpmlint issue
  See attached:
  ! Note that "sipwitch.src:151: E: hardcoded-library-path in /lib" is
    because of rpath fix suggested above and this can be ignored.
  - If %{python_sitelib}/sipwitch.py is to be executed directly from
    users, then this script should have 0755 permission.
    Otherwise the shebang on this script should be removed.

* Macros
  - For the directory where php binary modules are installed, please
    use %php_extdir macro. This macro is defined in /etc/rpm/macros.php
    in php-devel.

* Initscripts convention
  Again please check:
  https://fedoraproject.org/wiki/Packaging/SysVInitScript
  - Initscript must be installed under %_initrddir (/etc/rc.d/init.d,
    and please use this macro), not %_sysconfdir/init.d.
  - Some proper Requires(post) or so and scriptlets are needed.
    Please refer to
    https://fedoraproject.org/wiki/Packaging/SysVInitScript#Initscripts_in_spec_file_scriptlets
Comment 4 Mamoru TASAKA 2009-05-08 13:28:43 EDT
(Removing NEEDSPONSOR)
Comment 5 David Sugar 2009-05-08 13:34:20 EDT
I am going to look this over later tonight or maybe even over the weekend. 
There are some aspects that I may wish to do a new upstream release for also...

In respect to init scripts, I usually like to keep the scripts themselves
simple because they are going to run on different platforms...but most of this
relates to the package part :).  

One concern I find in this is I wish to make sure that the upstream source
picks the same directories as the packaging system :).  For that, for things
like python_sitearch, etc, I think I should add to configure.ac some stuff so I
can do something like --with-python-sitearch=%{python_sitearch}...,
--with-python-sitelib=..., --with-initrd-dir=... in %configure rather than
simply depend on the upstream target directories happening to match :).

So I want to think about that aspect later today...

This in many ways is a good package for use in training, as it covers so many
different aspects and issues.
Comment 6 David Sugar 2009-05-08 14:41:30 EDT
It is safe for sipwitch-snmp to be separate because it may be separately installed on a network management station, for example to collect and decode traps.  No sipwitch would be installed there :)
Comment 7 David Sugar 2009-05-08 16:46:49 EDT
I decided to try and complete all changes on the current upstream release.

Updated spec: http://www.gnutelephony.org/specs/sipwitch.spec
Updated srpm: http://www.gnutelephony.org/specs/sipwitch-0.5.4-2.fc10.src.rpm
Updated patch: http://www.gnutelephony.org/specs/sipwitch.patch

And I will shift some changes upstream after this...
Comment 8 Mamoru TASAKA 2009-05-09 12:35:19 EDT
For -2:

* BR
  - build.log shows:
-------------------------------------------------------------
   714  DEBUG: checking gcrypt/gcrypt.h usability... no
   715  DEBUG: checking gcrypt/gcrypt.h presence... no
   716  DEBUG: checking for gcrypt/gcrypt.h... no
   717  DEBUG: checking gcrypt.h usability... no
   718  DEBUG: checking gcrypt.h presence... no
   719  DEBUG: checking for gcrypt.h... no
   720  DEBUG: checking openssl/md5.h usability... no
   721  DEBUG: checking openssl/md5.h presence... no
   722  DEBUG: checking for openssl/md5.h... no
   723  DEBUG: checking sasl/md5global.h usability... no
   724  DEBUG: checking sasl/md5global.h presence... no
   725  DEBUG: checking for sasl/md5global.h... no
-------------------------------------------------------------
    Looking at configure.in, common/digest.cpp, and utils/sipdigest.cpp,
    it seems that adding "BuildRequires: libgcrypt-devel is preferable.
    After that, build.log shows:
-------------------------------------------------------------
   722  DEBUG: checking gcrypt/gcrypt.h usability... no
   723  DEBUG: checking gcrypt/gcrypt.h presence... no
   724  DEBUG: checking for gcrypt/gcrypt.h... no
   725  DEBUG: checking gcrypt.h usability... yes
   726  DEBUG: checking gcrypt.h presence... yes
   727  DEBUG: checking for gcrypt.h... yes
   728  DEBUG: checking for gcry_control in -lgcrypt... yes
-------------------------------------------------------------

* build failure
  - -2 srpm installs sipwitch initscript under 
    %buildroot%_sysconfdir/init.d, not %buildroot%_initrddir and
    build fails there. Currently the following is needed:
-------------------------------------------------------------
%{__mkdir_p} %{buildroot}%{_initrddir}
%{__mv} -f %{buildroot}%{_sysconfdir}/init.d/* %{buildroot}%{_initrddir}
rmdir %{buildroot}%{_sysconfdir}/init.d/
-------------------------------------------------------------

* Initscripts
  - Requires(postun): initscripts is still missing.
  - "service sipwitch restart" won't work like:
-------------------------------------------------------------
# service sipwitch restart
Restarting sipwitch: /etc/init.d/sipwitch: line 116: start-stop-daemon: command not found
-------------------------------------------------------------

* rpmlint
  - Still rpmlint complains like:
-------------------------------------------------------------
sipwitch-python-swig.i586: E: script-without-shebang /usr/lib/python2.6/site-packages/sipwitch.py
-------------------------------------------------------------
    Please check my previous comment:
>   - If %{python_sitelib}/sipwitch.py is to be executed directly from
>     users, then this script should have 0755 permission.
>     Otherwise the shebang on this script should be removed.
Comment 9 David Sugar 2009-05-09 21:47:50 EDT
The sipwitch.py issue is partially related to swig, since it is generated by swig.  I changed the patch to install using INSTALL_DATA, and maybe I will need to set explicit attrib too?  I will test this a bit here...

I see now yes, "upstart" based debian/ubuntu only have /etc/init.d, where rh/fedora symlink it to /etc/rc.d/init.d.  In the next upstream release I am adding --with-initrddir option for configure, but for the current release, I will use your hack...

The stop-start-daemon issue gets to the heart of some init scripting differences between debian & rh.  I am not sure if this is best solved with a patch, since init scripts dont change much, so that would not be too hard to maintain, or to have an entirely separate source1 just to take along a fedora/rh specific init script.

I have already worked on the other changes, and once I test them and resolve the debian vs rh init script issues remaining, I will do a new update.  Probably I will go for extending the patch for now.

Incidentally, if gcrypt is not used, sipwitch falls back to openssl libcrypto.  This in fact should have been preferred since libeXosip2 already links in openssl, but it seems to fail to detect libcrypto support (even for md5), and that is strange.  Also I tested mostly with the gcrypt crypto functions, so it is probably safer to keep using those for now.  Anyway, at some point we all should try to  converge on nss based stuff :).
Comment 10 David Sugar 2009-05-09 22:12:26 EDT
rpmlint on sipwitch binary rpm itself:

sipwitch.i386: E: non-readable /etc/sipwitch.conf 0660
sipwitch.i386: E: non-readable /etc/sipwitch.d/lab.xml 0660
sipwitch.i386: E: non-readable /etc/sysconfig/sipwitch 0660
sipwitch.i386: E: non-readable /etc/sipwitch.d/tests.xml 0660
sipwitch.i386: E: non-standard-dir-perm /etc/sipwitch.d 0770

In fact, I do NOT want those files to be "world" readable...and I do want them to be group and user r/w.  So I feel these are actually correct, not error.
Comment 11 Mamoru TASAKA 2009-05-10 00:18:01 EDT
Hello:

(In reply to comment #9)
> The sipwitch.py issue is partially related to swig, since it is generated by
> swig.  I changed the patch to install using INSTALL_DATA, and maybe I will need
> to set explicit attrib too?  I will test this a bit here...
- For this just using %attr(...) or using chmod at %install
  is enough.

> Incidentally, if gcrypt is not used, sipwitch falls back to openssl libcrypto. 
> This in fact should have been preferred since libeXosip2 already links in
> openssl, but it seems to fail to detect libcrypto support (even for md5), and
> that is strange.  Also I tested mostly with the gcrypt crypto functions, so it
> is probably safer to keep using those for now.  Anyway, at some point we all
> should try to  converge on nss based stuff :).  

- Well, on rawhide libeXosip2 is not linked against openssl
  library.
  Note that this is under GPLv3+, and strictly speaking openssl
  license conflicts with GPL (any version):
  https://fedoraproject.org/wiki/Licensing
Comment 12 Mamoru TASAKA 2009-05-10 00:20:19 EDT
(In reply to comment #10)
> rpmlint on sipwitch binary rpm itself:
> 
> sipwitch.i386: E: non-readable /etc/sipwitch.conf 0660
> sipwitch.i386: E: non-readable /etc/sipwitch.d/lab.xml 0660
> sipwitch.i386: E: non-readable /etc/sysconfig/sipwitch 0660
> sipwitch.i386: E: non-readable /etc/sipwitch.d/tests.xml 0660
> sipwitch.i386: E: non-standard-dir-perm /etc/sipwitch.d 0770
> 
> In fact, I do NOT want those files to be "world" readable...and I do want them
> to be group and user r/w.  So I feel these are actually correct, not error.  

- rpmlint always complains about these types of permission/attributes,
  however we usually ignores these "errors" if there is enough
  reason
  (By the way, there is no difference for this case, 
   however I prefer "0640" or "0750" permission)
Comment 13 David Sugar 2009-05-10 10:55:46 EDT
I actually wrote a patch (for upstream) to make openssl optional in exosip2 at configure awhile back.  Yes, openssl licensing is...rather problematic.  And nss has fips certification.  I am doing another upstream patch for exosip2 upstream related to terminating with reason, and maybe I will try to also substitute nss in it as well then.  I also often hear "openssl" falls under the "system library exception", a rather uncertain assertion though perhaps a bit easier to say on BSD.

I re-wrote the init script in a generic way for the next upstream release, and have used that as a patch for this one, hence the patch file is updated and should solve these other issues.  I now understand how condrestart relates to updates, and it makes a lot of sense to me.  In debian, I did it an uglier way though achieving the same effect.

I also replaced $(INSTALL) sipwitch.py with $(INSTALL_DATA) in the patch and that cleared the exec permission problem, since sipwitch.py generated by swig has no #! and should not be marked execute.

I use 0660/0770 because I often create an "admin" group for the daemon, and users of that group have to be able to modify config, write the control fifo, etc.  Perhaps that process should occur automatically in pre/post scripts and be "standardized"?

Anyway here is what I have now:

Updated Spec: http://www.gnutelephony.org/specs/sipwitch.spec
Updated SRPM: http://www.gnutelephony.org/specs/sipwitch-0.5.4-3.fc10.src.rpm
Updated Patch: http://www.gnutelephony.org/specs/sipwitch.patch
Comment 14 Mamoru TASAKA 2009-05-10 13:23:31 EDT
Well,
------------------------------------------------------
    This package (sipwitch) is APPROVED by mtasaka
------------------------------------------------------

Now please follow http://fedoraproject.org/wiki/Package_Review_Process
from "CVS admin requests" (also please write cvs admin request
on ucommon review request)
Comment 15 David Sugar 2009-05-10 18:10:57 EDT
New Package CVS Request
=======================
Package Name: sipwitch
Short Description: SIP telephony server for secure phone systems
Owners: dyfet
Branches: F-10 F-11
InitialCC: mtasaka
Comment 16 Kevin Fenzi 2009-05-13 01:09:10 EDT
cvs done.
Comment 17 Fedora Update System 2009-05-15 13:27:57 EDT
sipwitch-0.5.4-3.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/sipwitch-0.5.4-3.fc10
Comment 18 Fedora Update System 2009-05-15 13:30:00 EDT
sipwitch-0.5.4-3.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/sipwitch-0.5.4-3.fc11
Comment 19 Mamoru TASAKA 2009-05-16 09:50:33 EDT
Now closing.
Comment 20 Fedora Update System 2009-05-18 22:03:15 EDT
sipwitch-0.5.4-3.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 21 Fedora Update System 2009-05-18 22:08:26 EDT
sipwitch-0.5.4-3.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

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