I'll take the review for this.

Er, wait, you've not been sponsored for the packaging SIG. I'm not sure if there's anything special for Red Hat folks, but the details on becoming sponsored are laid out at https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group

I'm going to set this bug back to NEW until then.

Hello, Jozef:

Just looking at your spec file, it says this package depends
on rubygem(bundler), however I don't see rubygem-bundle in
Fedora repository or in package review. Would you package
rubygem-bundle first if it is needed for this package?

Hi, Mamoru,

i'll do package rubygem-bundler. But for now isn't important, because i don't have a sponsor and also i'm not in Fedora Packager Group, so i've focused on this issues.
(In reply to comment #3)
> Hello, Jozef:
> 
> Just looking at your spec file, it says this package depends
> on rubygem(bundler), however I don't see rubygem-bundle in
> Fedora repository or in package review. Would you package
> rubygem-bundle first if it is needed for this package?

(In reply to comment #4)
> Hi, Mamoru,
> 
> i'll do package rubygem-bundler. But for now isn't important, because i don't
> have a sponsor and also i'm not in Fedora Packager Group, so i've focused on
> this issues.

Well, 
What I mean here is that as "Requires: rubygem(bundler)" is in
the spec file of rubygem-database_cleaner, unless you firstly
package rubygem-bundler and rubygem-bundler review request is approved
beforehand, this package can never be approved, because unless
rubygem-bundler is in Fedora repository this rpm cannot be
installed (because of dependency issue).

For now setting NotReady.

So would you package rubygem-bundler, submit review request
and let us know it?

(In reply to comment #6)
> So would you package rubygem-bundler, submit review request
> and let us know it?

https://bugzilla.redhat.com/show_bug.cgi?id=646836

Hello,

I am taking over this package from Jozef. Here are the files updated to the latest upstream version. Bundler is no longer issue (actually it never was IMO). I will appreciate your review.

Spec URL: http://people.redhat.com/vondruch/rubygem-database_cleaner.spec
SRPM URL: http://people.redhat.com/vondruch/rubygem-database_cleaner-0.6.6-1.fc15.src.rpm

Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=2931185

Since this has been stale for so long, I'll take over the review here.

[clalance@localhost ~]$ rpmlint rubygem-database_cleaner.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.
[clalance@localhost ~]$ rpmlint rubygem-database_cleaner-0.6.6-1.fc15.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

Based on the guidelines at: https://fedoraproject.org/wiki/Packaging:Ruby

MUST require ruby abi: OK
MUST be called rubygem-UPSTREAM: OK
MUST have full URL to released gem archive: OK
MUST Require rubygems: OK
MUST BuildRequire rubygems: OK
MUST Provide rubygem(%{gemname}): OK
SHOULD have empty %prep: FAIL
SHOULD have empty %build: OK
MUST be installed into %{gemdir}: OK
SHOULD be installed with gem install --local: FAIL
MUST own gems, cache, and specifications: OK
MUST not install arch-specific content into %{gemdir}: N/A
MUST be marked as BuildArch noarch for pure ruby: OK

This looks pretty good, except for the fact that the installation is done during the %prep section instead of during the %install section.  However, I have seen reasons that this was necessary in the past, so if there is a good reason we can waive it.  Vit, what is the reason that the "gem install" command is done in %prep as opposed to %install?

Thanks,
Chris Lalancette

Hello Chris, thank you for taking over this review. Since this package needs to be patched, it is necessary to install the gem in %prep section. Furthemore, the gem should be always installed in %prep section although the guidelines say something different atm. The reasons are:

1) The patch may be required in any time during the life of the gem and moving gem installation from %install into the %prep section is therefore required. If you install in %prep right from the start, you don't need to do this.
2) It is common for all packages to be installed in %prep section. It is bit clumsy with gems, but still its the best place IMO.

I hope I find time to prepare updated guidelines one day.

(In reply to comment #10)
> Hello Chris, thank you for taking over this review. Since this package needs to
> be patched, it is necessary to install the gem in %prep section. Furthemore,
> the gem should be always installed in %prep section although the guidelines say
> something different atm. The reasons are:
> 
> 1) The patch may be required in any time during the life of the gem and moving
> gem installation from %install into the %prep section is therefore required. If
> you install in %prep right from the start, you don't need to do this.

Note that you can reference patches via %{PATCH#} at any stage in the process to apply patches.  Even so, I've rarely seen the case where that is strictly necessary; you can most usually do it during %prep.

> 2) It is common for all packages to be installed in %prep section. It is bit
> clumsy with gems, but still its the best place IMO.

See, the thing is, it is *not* common for packages to be installed in the %prep section.  If you look at any non-gem RPM, the %prep section is all about unpacking the sources and applying patches.  What you typically end up with at that point is a development environment that you could do a "make" against.  (as one example, fedpkg clone openssh ; cd openssh ; fedpkg prep)

Even amongst rubygems, it seems like the preferred way to do things is to leave %prep and %build empty, and just install the gem in the %install section.  That seems to make sense; you are installing the gem into the final destination at that point.  %prep just seems like the wrong place to be doing it, in my opinion, without a good reason.

Chris, I had quite lengthy discussion on this topic with Mamoru at [1] and I was convinced by him. There you can find few points why to install gem in %prep section. 

> Even amongst rubygems, it seems like the preferred way to do things is to leave
> %prep and %build empty, and just install the gem in the %install section.

There is only one preferred way amongst rubygems and that is the .spec file produced by rubygem-gem2rpm. Most of the packager even do not fix the license until review, so do not take other gems too seriously please.

[1] https://bugzilla.redhat.com/show_bug.cgi?id=676308

After thinking about it overnight, I think you are right.  Where I confused myself was that the last gem I packaged had a build component.  That is, it had a bunch of *.c files that had to be compiled in order to produce the final gem.  In those particular cases, it seems to be better to something like a gem unpack in %prep, and then build in %build, and then install in %install.

However, for pure ruby gems, I agree with you.  The problem is that in order to split it up like a "standard" RPM, you would have to gem unpack in the first step, and then repack it during the %install phase so that you could then run gem install.  That's ugly, and would introduce additional build dependencies (for the gem.add_development_dependency stuff).

That all being said, I think this package is now good to go.  I've set the fedora-review+ flag, so I think you should be good to open up an SCM admin request and build it into rawhide.

Thanks for the patience,
Chris Lalancette

Chris, thank you for your review!



New Package SCM Request
=======================
Package Name: rubygem-database_cleaner
Short Description: Strategies for cleaning databases
Owners: vondruch

Git done (by process-git-requests).