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=626666 --- Comment #2 from Mamoru Tasaka <mtasaka@xxxxxxxxxxxxxxxxxxx> 2010-10-01 14:54:28 EDT --- Some notes (checked 1.0.2-2) * BR - Looking into bindings/ directory, it seems that some languages bindings are available for this package (python, php, and also perhaps ruby?). Would you try to enable these bindings? * License ------------------------------------------------------------------- doc/ja/README says LGPLv2 BSD doc/ja/html/_static/ MIT or GPLv2 doc/ja/html/_static/jquery.js resource/admin_html/css/ui-lightness/jquery-ui-1.8.1.custom.css resource/admin_html/js/jquery-1.4.2.min.js ------------------------------------------------------------------ - So -libs license should be "LGPLv2 and (MIT or GPLv2)" and -doc license should be "LGPLv2 and BSD". * Timestamps - Please consider to use ------------------------------------------------------------------- make install DESTDIR=$RPM_BUILD_ROOT INSTALL~="install -p" ------------------------------------------------------------------- to keep timestamps on installed files as much as possible. This method usually works for Makefiles generated by recent autotool scripts. * Macros - Please use macros for standard directories. /var should be %_var or %_localstatedir, /usr/sbin should be %_sbin, and etc https://fedoraproject.org/wiki/Packaging/RPMMacros * SysV script related issues - Please use %{_initddir} (/etc/rc.d/init.d) instead of %_sysconfdir/init.d https://fedoraproject.org/wiki/Packaging/SysVInitScript#Initscripts_on_the_filesystem - For registering groonga user or group, please follow https://fedoraproject.org/wiki/Packaging/UsersAndGroups ! Note The current way of yours is anyway confusing because doing 'echo "Unexpected error adding group \"groonga\". Aborting installation."' does not actually abort installation (because $ echo foo will perhaps exit with status 0). ------------------------------------------------------------------- /usr/sbin/useradd -r -s /sbin/nologin -c 'groonga' \ -d %{_localstatedir}/lib/groonga --create-home \ ------------------------------------------------------------------- - $ env LANG=C man useradd says "--create-home" will copy files in skeleton directory into the created home directory. Is it needed? - And anyway %_localstatedir/lib/groonga must also be listed in %files entry, so I don't think --create-home option is needed here. - The lines ------------------------------------------------------------------- %post /bin/mkdir -p /var/run/groonga /bin/chown -R groonga:groonga /var/run/groonga ------------------------------------------------------------------- is wrong. In this case, the directory %_localstatedir/run/%name must be registered in %files entry with proper %attr added. ------------------------------------------------------------------- %post munin-plugins /usr/sbin/munin-node-configure --shell --remove-also | grep -e 'groonga_' | sh [ -f /var/lock/subsys/munin-node ] && \ /sbin/service munin-node restart > /dev/null 2>&1 ------------------------------------------------------------------- - Some Requires(post) is required here (Requires(post): munin-node) - Doesn't munin-node service support condrestart? - We don't usually call userdel / groupdel on scriptlets: https://fedoraproject.org/wiki/Packaging/UsersAndGroups - Would you explain why this is needed? -------------------------------------------------------------------- %postun munin-plugins if [ $1 -eq 0 ]; then rm %{_sysconfdir}/munin/plugins/groongar_* > /dev/null 2>&1 -------------------------------------------------------------------- - By the way calling /sbin/service or /sbin/chkconfig or getent or so requires writing Proper "Requires(foo): bar"s. Please refer to: https://fedoraproject.org/wiki/Packaging/UsersAndGroups https://fedoraproject.org/wiki/Packaging/SysVInitScript#Initscripts_in_spec_file_scriptlets * %files - We now prefer to use %defattr(-,root,root,-) - Files under %_mandir (note: please use %_mandir instead of %_datadir/man) are automatically marked as %doc, so there is no need to write explicit %doc attribute for %_mandir/foo. - Please take care of directory ownership issue. https://fedoraproject.org/wiki/Packaging/Guidelines#File_and_Directory_Ownership https://fedoraproject.org/wiki/Packaging/UnownedDirectories ! The point is that when installing a (binary) rpm creates some new directories, the created directories should also be owned by the (just installed) rpm. For example, when installing groonga-libs rpm, installing %_libdir/groonga/modules/suggest/suggest.so (on i686) newly creates the following directories: -------------------------------------------------------------- %_libdir/%name %_libdir/%name/modules %_libdir/%name/modules/suggest -------------------------------------------------------------- so -libs subpackage should also own these directories. Also please refer to https://fedoraproject.org/wiki/Packaging/UnownedDirectories#Common_Mistakes ! Instead if the needed directories for a rpm are already created by the packages which the rpm depends on, the rpm (to be newly installed) should not own the directory. For example the directory %_sysconfdir/munin/plugin-conf.d is already owned by munin-node, so -munin-plugins subpackage should not own this directory itself. -- 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. _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review