[Fedora-i18n-bugs] [Bug 626666] Review Request: groonga - An Embeddable Fulltext Search Engine

[Red Hat Bugzilla]

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  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_f...

- 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_...

* %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_Owner...
    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.