Bug 514833
| Summary: | Review Request: sphinx - Free open-source SQL full-text search engine | ||
|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | Allisson Azevedo <allisson> |
| Component: | Package Review | Assignee: | Andrew Colin Kissa <andrew> |
| Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
| Severity: | medium | Docs Contact: | |
| Priority: | medium | ||
| Version: | rawhide | CC: | andrew, fedora-package-review, notting |
| Target Milestone: | --- | Flags: | andrew:
fedora-review+
j: fedora-cvs+ |
| Target Release: | --- | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Whiteboard: | |||
| Fixed In Version: | Doc Type: | Bug Fix | |
| Doc Text: | Story Points: | --- | |
| Clone Of: | Environment: | ||
| Last Closed: | 2009-08-14 17:30:33 UTC | Type: | --- |
| Regression: | --- | Mount Type: | --- |
| Documentation: | --- | CRM: | |
| Verified Versions: | Category: | --- | |
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
| Cloudforms Team: | --- | Target Upstream Version: | |
| Embargoed: | |||
|
Description
Allisson Azevedo
2009-07-30 23:21:39 UTC
A few issues to begin with * The package contains searchd which is a server daemon (http://sphinxsearch.com/docs/current.html#ref-searchd) so the package should contain an init script for managing the service. * The *.conf.dist files installed in /etc/sphinx could be installed as doc's as they are not really required for the program to function. * The sample sql file should not be placed in /etc, better installed as part of the doc's * Consider packaging libsphinxclient as a sub package as we will need this to package the PHP PEAR package for sphinx http://pecl.php.net/package/sphinx * Need to create a data directory where the index is stored, since this is data that changes it cannot be stored in /etc where the default configuration is configured to store it. Update package: Spec URL: http://allisson.fedorapeople.org/packages/sphinx/sphinx.spec SRPM URL: http://allisson.fedorapeople.org/packages/sphinx/sphinx-0.9.8.1-2.fc11.src.rpm Changelog: * Fri Aug 7 2009 Allisson Azevedo <allisson> 0.9.8.1-2 - Added sysv init. - Added logrotate.d entry. koji build scratch: http://koji.fedoraproject.org/koji/taskinfo?taskID=1589151
Some notes:
* Language API's
- As per the package description you need to ship the Search API's with the
package, as suggested before i think you should ship libsphinxclient as
subpackages (lib and devel) since it is a library
The other languages (PHP,Python,Ruby,Java) can be shipped within the main
package
* * Macros consistency
- Well, if you want to use %{__mkdir} or %{__cp} style, please
use %{__make}, %{__rm}, %{__sed}, etc for consistency.
* Timestamps
- Would you consider using
---------------------------------------------------------------
make install DESTDIR=%{buildroot} INSTALL="install -p"
install -p -D -m 0755 %{SOURCE1} $RPM_BUILD_ROOT%{_initrddir}/%{name}
---------------------------------------------------------------
for example to keep timestamps on installed files?
http://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps
Update package: Spec URL: http://allisson.fedorapeople.org/packages/sphinx/sphinx.spec SRPM URL: http://allisson.fedorapeople.org/packages/sphinx/sphinx-0.9.8.1-3.fc11.src.rpm Changelog: * Wed Aug 12 2009 Allisson Azevedo <allisson> 0.9.8.1-3 - Fixed macros consistency. - Modified make install to keep timestamps. - Added libsphinxclient package. Allisson, The final issue is not a blocker, just a good to have. The API's would be better installed in usable state (Java compiled, python in the correct directory, etc) Am sure you can do that before CVS if you want. Otherwise all looks fine now. ------------------------------------------------------------------- This package (sphinx) is APPROVED by topdog ------------------------------------------------------------------- New Package CVS Request ======================= Package Name: sphinx Short Description: Free open-source SQL full-text search engine Owners: allisson Branches: F-10 F-11 EL-5 CVS done. |