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@xxxxxxxxxxxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mtasaka@xxxxxxxxxxxxxxxxxxx --- Comment #59 from Mamoru Tasaka <mtasaka@xxxxxxxxxxxxxxxxxxx> 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@localhost ~]$ num=0 [tasaka1@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@localhost ~]# service murmur start Starting murmur: [ OK ] [root@localhost ~]# service murmur stop Shutting down murmur: [ OK ] [root@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. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@xxxxxxxxxx http://www.redhat.com/mailman/listinfo/fedora-package-review