Bug 468597 - Review Request: rubygem-ferret - Full-featured text search engine library
Review Request: rubygem-ferret - Full-featured text search engine library
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-10-26 12:25 EDT by Jeroen van Meeuwen
Modified: 2009-05-09 00:10 EDT (History)
3 users (show)

See Also:
Fixed In Version: 0.11.6-9.fc11
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-04-25 13:15:23 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mtasaka: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Jeroen van Meeuwen 2008-10-26 12:25:51 EDT
Spec URL: http://www.kanarip.com/custom/SPECS/rubygem-ferret.spec
SRPM URL: http://www.kanarip.com/custom/f9/SRPMS/rubygem-ferret-0.11.6-1.fc9.src.rpm
Description: Full-featured text search engine library

$ rpmlint /home/jmeeuwen/rpmbuild/RPMS/x86_64/rubygem-ferret-0.11.6-1.fc9.x86_64.rpm
rubygem-ferret.x86_64: W: no-soname /usr/lib/ruby/gems/1.8/gems/ferret-0.11.6/lib/ferret_ext.so
1 packages and 0 specfiles checked; 0 errors, 1 warnings.
$ rpmlint /home/jmeeuwen/rpmbuild/RPMS/x86_64/rubygem-ferret-devel-0.11.6-1.fc9.x86_64.rpm
rubygem-ferret-devel.x86_64: W: no-documentation
1 packages and 0 specfiles checked; 0 errors, 1 warnings.
$ rpmlint /home/jmeeuwen/rpmbuild/RPMS/x86_64/rubygem-ferret-debuginfo-0.11.6-1.fc9.x86_64.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

Seems obvious to me -devel does not need documentation separately from the base package, and I'm not sure what the no-soname is about and how to patch it given http://fedoraproject.org/wiki/PackageMaintainers/Common_Rpmlint_Issues#no-soname and that this is a rubygem...

koji scratch builds:

- dist-f8-updates-candidate: http://koji.fedoraproject.org/koji/taskinfo?taskID=904323
- dist-f9-updates-candidate: http://koji.fedoraproject.org/koji/taskinfo?taskID=904328
- dist-f10: http://koji.fedoraproject.org/koji/taskinfo?taskID=904333
- dist-f11: http://koji.fedoraproject.org/koji/taskinfo?taskID=904338

Note that the %files section uses absolute paths and no macros because for some weird reason the macros would not work
Comment 1 Jeroen van Meeuwen 2008-10-26 12:32:11 EDT
New SPEC: http://www.kanarip.com/custom/SPECS/rubygem-ferret.spec
New SRPM: http://www.kanarip.com/custom/f9/SRPMS/rubygem-ferret-0.11.6-2.fc9.src.rpm

rpmlint now silent
better use of macros
Comment 2 S.A. Hartsuiker 2008-10-26 13:07:40 EDT
RPM Lint: quiet
Package name: ok
Spec file: ok
License: MIT
Actual License: MIT
%doc License: MIT-LICENSE file (see below)
Spec file language: ok
Spec file readable: ok
Upstream source vs. used tarball: ok (md5: 928b6f90c61593059d8668dc70ebf337)
Compile and Build:
 - F-8: ok
 - F-9: ok
 - F-10: ok
 - rawhide: ok 
 - EL-5: n/a

Applicable Package Guidelines: ok

Locales: n/a
Shared libs: ok

Relocatable: no
Directory and file ownership: ok
No duplicate files in %files: ok
File Permissions: ok
Macro usage: ok
Code vs. Content: ok
(Large) Documentation: n/a
%doc affecting runtime: ok
Header files in -devel package: n/a
Static Libraries in -static package: n/a
pkgconfig Requires: n/a
Library files: ok
Devel requires base package: n/a
.la libtool archives: n/a
Duplicate ownership of files/directories: ok
Remove BuildRoot: ok
UTF-8 filenames: ok

You forgot to package the MIT-LICENSE file. Please include it as a %doc.
Otherwise the package is ok now.
Comment 4 S.A. Hartsuiker 2008-10-26 13:17:03 EDT
ok, license file included now.
package APPROVED.
Comment 5 Jeroen van Meeuwen 2008-10-26 13:18:36 EDT
New Package CVS Request
=======================
Package Name: rubygem-ferret
Short Description: Full-featured text search engine library
Owners: kanarip
Branches: EL-4 EL-5 F-8 F-9 devel
InitialCC:
Comment 6 Mamoru TASAKA 2008-10-26 13:19:22 EDT
First of all, this is not gem....
Comment 7 Mamoru TASAKA 2008-10-26 13:20:08 EDT
I cannot approve this.
Comment 8 Jeroen van Meeuwen 2008-10-26 13:54:46 EDT
Care to comment? Upstream provides a gem what makes you think this is not a gem? The installation instructions even indicate the program and libraries can be installed by using 'gem install ferret'... You got me confused.
Comment 9 Mamoru TASAKA 2008-10-26 14:04:28 EDT
(In reply to comment #8)
> Care to comment? Upstream provides a gem 
Then you must use the gem as the source

> what makes you think this is not a
> gem? 
Because _this package_ does not contain any gem.

> The installation instructions even indicate the program and libraries can
> be installed by using 'gem install ferret'... 
It is for people not using rpm system by installing this software by him/herself.
Comment 10 Mamoru TASAKA 2008-10-26 14:27:39 EDT
Some notes:

- As ext/posh.c is under BSD, the license tag must be "MIT and BSD" (we
  have to actually check the licenses of all codes, not just license
  text)
Comment 11 Jeroen van Meeuwen 2008-10-26 14:35:16 EDT
OK, this is why the package ended up being what it is right now:

Attempt #1: https://koji.fedoraproject.org/koji/taskinfo?taskID=904055
Attempt #2: https://koji.fedoraproject.org/koji/taskinfo?taskID=904077
Attempt #3: https://koji.fedoraproject.org/koji/taskinfo?taskID=904137
Attempt #4: https://koji.fedoraproject.org/koji/taskinfo?taskID=904313
Attempt #5 (current package, successful): https://koji.fedoraproject.org/koji/taskinfo?taskID=904318

The first 4 attempts use .gem and gem install from upstream's source code .tgz, but fail during build although running them through mock locally would succeed. Since I wasn't able to determine how to package the software 1) according to package guidelines, 2) with debuginfo, 3) without -devel ending up in the base package, 4) without /usr/lib/rpm/check-buildroot borking over binaries that matched the buildroot, I went with packaging it the way that I did (Attempt #5).

(In reply to comment #9)
> (In reply to comment #8)
> > Care to comment? Upstream provides a gem 
> Then you must use the gem as the source
This one is new for me... can you elaborate on the fact that if it's a gem, you must use the upstream provided gem? I can't seem to find the guidelines saying that I should.

Thanks in advance!
Comment 12 Jeroen van Meeuwen 2008-10-26 14:43:27 EDT
(In reply to comment #10)
> Some notes:
> 
> - As ext/posh.c is under BSD, the license tag must be "MIT and BSD" (we
>   have to actually check the licenses of all codes, not just license
>   text)

I found GPLv2+ too in ext/q_parser.c. Pending other comments, I'll push these changes when we're done.
Comment 13 Mamoru TASAKA 2008-10-26 15:33:10 EDT
(In reply to comment #11)
> Attempt #5 (current package, successful):
> https://koji.fedoraproject.org/koji/taskinfo?taskID=904318
I will look at this after I wake up again.
 
> (In reply to comment #9)
> > (In reply to comment #8)
> > > Care to comment? Upstream provides a gem 
> > Then you must use the gem as the source
> This one is new for me... can you elaborate on the fact that if it's a gem, you
> must use the upstream provided gem? I can't seem to find the guidelines saying
> that I should.

Please recheck this.
https://fedoraproject.org/wiki/Packaging/Ruby#Ruby_Gems

- The Source of the package must be the full URL to the released Gem archive; 
  the version of the package must be the Gem's version 

(In reply to comment #12)
> I found GPLv2+ too in ext/q_parser.c. Pending other comments, I'll push these
> changes when we're done.
Please also check Bison exception mentioned in the file.
Comment 14 S.A. Hartsuiker 2008-10-26 15:33:55 EDT
(In reply to comment #10)
> Some notes:
> 
> - As ext/posh.c is under BSD, the license tag must be "MIT and BSD" (we
>   have to actually check the licenses of all codes, not just license
>   text)

Hmm, I had thought that the MIT license, so I didn't differentiate it.
Comment 15 Mamoru TASAKA 2008-10-26 15:40:08 EDT
(In reply to comment #13)
> (In reply to comment #11)
> > Attempt #5 (current package, successful):
> > https://koji.fedoraproject.org/koji/taskinfo?taskID=904318
> I will look at this after I wake up again.

Ah, this one is current srpm...
Comment 16 Jeroen van Meeuwen 2008-10-26 16:42:10 EDT
https://koji.fedoraproject.org/koji/getfile?taskID=904080&name=build.log uses the tgz to build the gem and then install the gem, but has left-over .o files installed

https://koji.fedoraproject.org/koji/getfile?taskID=904141&name=build.log is the next attempt and cleans out those .o files, but the ext/ferret_ext.so.debug file makes the rpm build bork nonetheless. This is one of the main reasons why I couldn't find a way to use or build and then use the gem itself.
Comment 17 Jeroen van Meeuwen 2008-10-26 17:18:03 EDT
(In reply to comment #13)
> (In reply to comment #12)
> > I found GPLv2+ too in ext/q_parser.c. Pending other comments, I'll push these
> > changes when we're done.
> Please also check Bison exception mentioned in the file.

The Bison exception is a "may remove (at your option)" which I'm not sure makes sense; how should I note this in the RPM License tag if it should be included in the RPM License tag?
Comment 18 Mamoru TASAKA 2008-10-26 23:14:09 EDT
(In reply to comment #16)
> https://koji.fedoraproject.org/koji/getfile?taskID=904080&name=build.log uses
> the tgz to build the gem and then install the gem, but has left-over .o files
> installed
> 
> https://koji.fedoraproject.org/koji/getfile?taskID=904141&name=build.log is the
> next attempt and cleans out those .o files, but the ext/ferret_ext.so.debug
> file makes the rpm build bork nonetheless. This is one of the main reasons why
> I couldn't find a way to use or build and then use the gem itself.

Sorry, however other maintainer handles this. Please fix this.
Example A (my case)
http://cvs.fedoraproject.org/viewvc/rpms/rubygem-zoom/devel/rubygem-zoom.spec?view=co

Example B (spec file by sseago)
http://cvs.fedoraproject.org/viewvc/rpms/rubygem-mongrel/devel/rubygem-mongrel.spec?view=co(In reply to comment #17)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > I found GPLv2+ too in ext/q_parser.c. Pending other comments, I'll push these
> > > changes when we're done.
> > Please also check Bison exception mentioned in the file.
> 
> The Bison exception is a "may remove (at your option)" which I'm not sure makes
> sense; how should I note this in the RPM License tag if it should be included
> in the RPM License tag?

Simply use "License: MIT and BSD".
Comment 20 Dennis Gilmore 2008-10-27 00:35:03 EDT
unsetting cvs request
Comment 21 Jeroen van Meeuwen 2008-10-28 06:18:49 EDT
I revised the packaging strategy using the spec samples from Mamoru;

New SPEC: http://www.kanarip.com/custom/SPECS/rubygem-ferret.spec
New SRPM: http://www.kanarip.com/custom/f9/SRPMS/rubygem-ferret-0.11.6-5.fc9.src.rpm

I'm not sure whether I should package the tests provided with the gem in -doc or in the base package... Please advise.

rpmlint is silent

koji scratch builds:

- dist-f8-updates-candidate: http://koji.fedoraproject.org/koji/taskinfo?taskID=908852
- dist-f9-updates-candidate: http://koji.fedoraproject.org/koji/taskinfo?taskID=908857
- dist-f10: http://koji.fedoraproject.org/koji/taskinfo?taskID=908870
- dist-f11: http://koji.fedoraproject.org/koji/taskinfo?taskID=908873
Comment 22 Jeroen van Meeuwen 2008-10-28 06:19:30 EDT
Sorry, SRPM URL should have been http://www.kanarip.com/custom/f9/SRPMS/rubygem-ferret-0.11.6-6.fc9.src.rpm
Comment 23 Mamoru TASAKA 2008-10-28 12:29:51 EDT
(In reply to comment #21)
> http://www.kanarip.com/custom/f9/SRPMS/rubygem-ferret-0.11.6-6.fc9.src.rpm
- So for license "GPLv2+" for ext/q_parser.c can be ignored by Bison exception term
  (you can regard that ext/q_parser.c are under "BSD or GPLv2+")
  Please use "BSD and MIT" for license tag.

> I'm not sure whether I should package the tests provided with the gem in -doc
> or in the base package... Please advise.
I would move these files to -doc (if I create -doc subpackage), however
this is under your choise.

By the way:
- I recommend to use %{version} tag for Source0:
  https://fedoraproject.org/wiki/Packaging/SourceURL#Using_.25.7Bversion.7D

  Also, the number "28549" in URL of Source0 will change each time the version
  is upgraded. If you don't like this, gem has alternative URL:
  http://gems.rubyforge.org/gems/%{gemname}-%{version}.gem

- Please make it sure that the directory %{geminstdir} itself is also
  owned by this package.
Comment 24 Mamoru TASAKA 2008-10-28 14:34:41 EDT
(In reply to comment #23)
> (In reply to comment #21)
> > http://www.kanarip.com/custom/f9/SRPMS/rubygem-ferret-0.11.6-6.fc9.src.rpm
> - So for license "GPLv2+" for ext/q_parser.c can be ignored by Bison exception
> term
>   (you can regard that ext/q_parser.c are under "BSD or GPLv2+")

This is "MIT or GPLv2+".
Comment 25 Jeroen van Meeuwen 2008-10-29 15:09:12 EDT
(In reply to comment #23)
> (In reply to comment #21)
> > http://www.kanarip.com/custom/f9/SRPMS/rubygem-ferret-0.11.6-6.fc9.src.rpm
> - So for license "GPLv2+" for ext/q_parser.c can be ignored by Bison exception
> term
>   (you can regard that ext/q_parser.c are under "BSD or GPLv2+")
>   Please use "BSD and MIT" for license tag.
> 

Fixed

> > I'm not sure whether I should package the tests provided with the gem in -doc
> > or in the base package... Please advise.
> I would move these files to -doc (if I create -doc subpackage), however
> this is under your choise.
> 

Moved the files to -doc subpackage for efficiency in production environments.

> By the way:
> - I recommend to use %{version} tag for Source0:
>   https://fedoraproject.org/wiki/Packaging/SourceURL#Using_.25.7Bversion.7D
> 
>   Also, the number "28549" in URL of Source0 will change each time the version
>   is upgraded. If you don't like this, gem has alternative URL:
>   http://gems.rubyforge.org/gems/%{gemname}-%{version}.gem
> 

Awesome, I didn't know this. Fixed.

> - Please make it sure that the directory %{geminstdir} itself is also
>   owned by this package.

Fixed.

New SPEC: http://www.kanarip.com/custom/SPECS/rubygem-ferret.spec
New SRPM: http://www.kanarip.com/custom/f9/SRPMS/rubygem-ferret-0.11.6-7.fc9.src.rpm
Comment 26 Mamoru TASAKA 2008-10-30 14:16:48 EDT
Looks good to me.

One note:
$ gem contents ferret
returns some files under %geminstdir/ext/, however they are missing
because you intentionally deleted them.
While I think removing files under %geminstdir/ext is preferable
(because I don't think these files are needed to make ferret gem
 work, they are only needed to make ruby C extension), 
however currently I have no idea how we should treat the issue that
gem reports that the files should have been installed.

Currently for rubygem-zoom (I maintain, which also creates ruby C
extension) I package all C sources into -doc subpackage....
Comment 27 Jeroen van Meeuwen 2008-10-30 17:31:14 EDT
If in ext/ is the ruby C extension files, would it be reasonable to package them in a -devel subpackage?
Comment 28 Mamoru TASAKA 2008-10-31 04:12:19 EDT
Umm.... usually a subpackage named "-devel" usually contains some header
files or so and means that other applications will make use of the files
in the -devel rpm.
However for this package files under ext/ directory are not meant to
be used from other packages (no header files installed), just to create
ferret C extension.

Maybe:
- to create -csource subpackage? (however if you want to
  name this as "-devel" I won't oppose it)
- to simply remove these files as before and ignore inconsistency
  with gem report (note: other maintainer of rubygem rpms containing
  C extension simply do this).
For now I won't complain whatever you choice

Perhaps when I can take some time I have to write some draft about
how to package rubygem including C extension... (I hope that
I can work on this this weekend), however I don't think this
should block this review request for now.
Comment 29 Jeroen van Meeuwen 2008-11-01 10:49:19 EDT
I'll leave this as-it-is then.
Comment 30 Mamoru TASAKA 2008-11-02 01:56:54 EST
Okay, then now I leave the final judgement for this review request to
the current assignee.
Comment 31 Jeroen van Meeuwen 2008-11-02 05:14:37 EST
Thanks a lot, both the assignee and I have learned something ;-)
Comment 32 Jeroen van Meeuwen 2009-01-31 21:22:48 EST
Stefan, ping?
Comment 33 Jeroen van Meeuwen 2009-03-05 06:19:29 EST
Stefan, ping?
Comment 34 Jeroen van Meeuwen 2009-03-16 08:51:35 EDT
Removed assignee
Comment 35 Mamoru TASAKA 2009-03-26 10:44:10 EDT
Taking this review request.

kanarip, would you update your srpm?
Note that now for rubygems containing C codes to compile,
packaging guidelines changed since this review request is
submitted, see:

https://fedoraproject.org/wiki/Packaging/Ruby#Ruby_Gem_with_extension_libraries_written_in_C

Examples are useful:
http://cvs.fedoraproject.org/viewvc/rpms/rubygem-hpricot/devel/rubygem-hpricot.spec?view=co
http://cvs.fedoraproject.org/viewvc/rpms/rubygem-nokogiri/devel/rubygem-nokogiri.spec?view=co

By the way I appreciate it if you would review my review request
(bug 492171)
Comment 36 Mamoru TASAKA 2009-04-04 12:16:13 EDT
ping?
Comment 38 Mamoru TASAKA 2009-04-05 14:29:48 EDT
Notes:

- BR: findutils is redundant. See:
  https://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2

- Would you check if "setup.rb" "Rakefile" are really needed?
  If it is needed, I think "setup.rb" should be moved to %doc

- %defattr(755, root, root, -) is redundant because the permission
  of ferret_ext.so is explicitly changed to 0755
  ! Note: It is needed anyway to make it sure that ferret_ext.so
    has 0755 permission before %install ends because otherwise
    find-debuginfo.sh won't work correctly.

- %exclude %{geminstdir}/Rakefile is redundant (no glob is used
  here)

! Not a blocker, however as ruby maintainer would you examine
  why "rake test_units" fails on ppc64?
  By the way it is preferable to enable tests as much as possible
  like
---------------------------------------------------------------
%check
pushd .%{geminstdir}
%ifarch %{ix86} x86_64 ppc
rake test_units
%else
rake test_units || :
%endif
---------------------------------------------------------------

But other things are okay.

---------------------------------------------------------------
     This package (rubygem-ferret) is APPROVED by mtasaka
---------------------------------------------------------------
Comment 39 Jeroen van Meeuwen 2009-04-05 17:42:37 EDT
- moved setup.rb to -doc subpackage
- removed redundant %defattr(755,root,root,-)
- removed %exclude %{geminstdir}/Rakefile
- Enabled the unit tests since they now fail softly.

rake test_units fail due to segmentation faults; I really need to obtain some skills on these kinds of issues in order to solve them ;-)

New Package CVS Request
=======================
Package Name: rubygem-ferret
Short Description: Full-featured text search engine library
Owners: kanarip
Branches: EL-4 EL-5 F-9 F-10
InitialCC:
Comment 40 Kevin Fenzi 2009-04-06 23:18:24 EDT
cvs done.
Comment 41 Mamoru TASAKA 2009-04-18 11:53:47 EDT
Please build this package on buildsystem.
Comment 42 Fedora Update System 2009-04-23 12:13:47 EDT
rubygem-ferret-0.11.6-9.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/rubygem-ferret-0.11.6-9.fc9
Comment 43 Fedora Update System 2009-04-23 12:13:54 EDT
rubygem-ferret-0.11.6-9.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/rubygem-ferret-0.11.6-9.fc10
Comment 44 Fedora Update System 2009-04-23 12:13:59 EDT
rubygem-ferret-0.11.6-9.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/rubygem-ferret-0.11.6-9.fc11
Comment 45 Mamoru TASAKA 2009-04-25 13:15:23 EDT
Closing.
Comment 46 Fedora Update System 2009-04-27 17:19:11 EDT
rubygem-ferret-0.11.6-9.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 47 Fedora Update System 2009-04-27 17:37:42 EDT
rubygem-ferret-0.11.6-9.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 48 Fedora Update System 2009-05-09 00:10:34 EDT
rubygem-ferret-0.11.6-9.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

Note You need to log in before you can comment on or make changes to this bug.