[Bug 497441] Review Request: mumble - Voice chat application

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


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


Mamoru Tasaka <mtasaka at ioa.s.u-tokyo.ac.jp> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mtasaka at ioa.s.u-tokyo.ac.jp




--- Comment #59 from Mamoru Tasaka <mtasaka at ioa.s.u-tokyo.ac.jp>  2009-05-14 14:54:57 EDT ---
Well, I have not fully read previous comments, however
for -9:

* BR (BuildRequires)
  - Would you explain why non-devel packages like:
-------------------------------------------------------------------
dbus-qt
qt-sqlite
-------------------------------------------------------------------
    have to be written as BRs explicitly?

* Requires
  - Proper Requires(pre) or so are missing:
    https://fedoraproject.org/wiki/Packaging/UsersAndGroups
   
https://fedoraproject.org/wiki/Packaging/SysVInitScript#InitscriptScriptlets
  - Would you explain why murmur subpackage does not depend
    on mumble main package?
  - The directory %{_datadir}/kde4/services/ is owned by kde-filesystem
    so it is better that -protocol subpackage should have
    "Requires: kde-filesystem".

* Parallel make
  - Support parallel make if possible. If not possible
    write some comments about this:
    https://fedoraproject.org/wiki/Packaging/Guidelines#Parallel_make

* Installation directory
------------------------------------------------------------------
install -pD -m0755 release/murmurd %{buildroot}%{_sbindir}/murmurd
ln -s ../sbin/murmurd %{buildroot}%{_bindir}/%{name}-server
------------------------------------------------------------------
  - Why is the same command has to be in two different directories
    in the path with different names?

  - By the way
------------------------------------------------------------------
%attr(-,mumble-server,mumble-server) %{_bindir}/mumble-server
------------------------------------------------------------------
    Here %{_bindir}/mumble-server is a symlink and
    %attr(.....) on symlink is useless (see man symlink(2))
    ! By the way using %{name} macro is preferred because
      you already use it.

------------------------------------------------------------------
#man pages
mkdir -p %{buildroot}%{_mandir}/
install -pD -m0664 man/murmurd.1 %{buildroot}%{_mandir}/
install -pD -m0664 man/mumble* %{buildroot}%{_mandir}/
------------------------------------------------------------------
  - These man pages must be installed under %_mandir/man1,
    not under %_mandir (and also see below)

* Permissions
------------------------------------------------------------------
install -pD -m0664 icons/%{name}.16x16.png
%{buildroot}%{_datadir}/icons/hicolor/16x16/apps/%{name}.png
------------------------------------------------------------------
  - Usually these files should have 0644 (not 0664) permission

------------------------------------------------------------------
install -pD scripts/%{name}.protocol
%{buildroot}%{_datadir}/kde4/services/%{name}.protocol
install -pD scripts/murmur.conf
%{buildroot}%{_sysconfdir}/dbus-1/system.d/murmur.conf
------------------------------------------------------------------
  - Due to this these files have executable permission however it
    seems these should be with 0644 permision.

* Inconsistent commands
------------------------------------------------------------------
install -d %{buildroot}%{_libdir}/%{name}/
mkdir -p %{buildroot}%{_mandir}/
------------------------------------------------------------------
  - Why do you use both "install -d" and "mkdir -p"?

* Scriptlets
  - Scriptlets for GTK cache updating does not match current Fedora
    guideline. Please fix this:
    https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Icon_Cache
    (especially check %postun)

  - Please specify the home directory of mumble-server user
    (see the -d option:
     https://fedoraproject.org/wiki/Packaging/UsersAndGroups )
    Currently this is set as "/home/mumble-server" automatically
    (on Fedora)

------------------------------------------------------------------
%preun -n murmur
if [$1 = 0]; then
------------------------------------------------------------------
  - This is wrong.
------------------------------------------------------------------
[tasaka1 at localhost ~]$ num=0
[tasaka1 at localhost ~]$ if [$num = 0] ; then echo "yes" ; fi
-bash: [0: command not found
------------------------------------------------------------------
    "if [ $1 = 0 ] ; then" is correct
    ! Note: here "[" is a built-in "command".

------------------------------------------------------------------
%post -n murmur
/sbin/chkconfig --add murmur --level a|| :
------------------------------------------------------------------
  - "--level a" is not needed:
   
https://fedoraproject.org/wiki/Packaging/SysVInitScript#InitscriptScriptlets

* Duplicate %files entry
------------------------------------------------------------------
-%files plugins
%defattr(-,root,root,-)
%{_libdir}/%{name}
%{_libdir}/%{name}/*.so
-----------------------------------------------------------------
  - The %files entry "%{_libdir}/%{name}" means the directory %_libdir/%name
    itself and all files/directories/etc under %_libdir/%name.
    build.log shows:
-----------------------------------------------------------------
   857  Processing files: mumble-plugins-1.1.8-9.fc11.i586
   858  warning: File listed twice: /usr/lib/mumble/liblink.so
-----------------------------------------------------------------

* initscript behavior
-----------------------------------------------------------------
[root at localhost ~]# service murmur start
Starting murmur:                                           [  OK  ]
[root at localhost ~]# service murmur stop
Shutting down murmur:                                      [  OK  ]
[root at localhost ~]#                                        [  OK  ]
-----------------------------------------------------------------
  - killproc() (in %_initrcdir/functions) internally calls
    success() or failure so there are duplicate messages when
    shutting down murmur daemon.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.