[Bug 433312] Review Request: opengrok - A wicked fast source browser

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Review Request: opengrok - A wicked fast source browser
Alias: opengrok-review

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


lkundrak at redhat.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|lkundrak at redhat.com         |overholt at redhat.com




------- Additional Comments From lkundrak at redhat.com  2008-04-12 10:30 EST -------
Thanks for the review!

(In reply to comment #9)

> X specfile is legible
>   - I'd rather see common_reqs split out and enumerated in both Requires and
>     BuildRequires

I thought this will save me from some errors and work when adding common
requires, but it's just a matter of personal taste. If you both think it doesn't
look well, I'll split it.

> X consider using cp -p to preserve timestamps
>   - please use cp -pR in %install

Done. Also install -p. I was thinking about preserving all timestamps before,
but some are virtually impossible -- such as manual page, which gets generated
from the java code on the fly. We're not in risk of multiarch conflicts here
(now without gcj bit's we're even noarch), so I guess that's not necessary.

> X verify the final provides and requires of the binary RPMs
> 
>    - the -javadoc package should depend upon jpackage-utils (which owns
>      %{_javadocdir} ... yes, this should probably be in the guidelines :)

Fixed.

> Notes:
> 
> - don't build with gcj at all because of this missing bit:
> 
> [javac]     import java.util.Scanner;
> [javac] 	   ^^^^^^^^^^^^^^^^^
> [javac] The import java.util.Scanner cannot be resolved
> 
> - remove gcj bits as the diff I'm attaching does

Applied. I wonder if it's right that this didn't break the build?

> - re-name patches to match version (0.5 -> 0.6)

I read somewhere, though I am not able to find the link now, that the version
number in patch name is one the patch was created against, and doesn't change
when it applies to newer upstream package.

> - for now, remove the tomcat package as we don't have guidelines -- or even
>   best practices -- for this

Done.

> - why don't you build a jrcs package and Require/BR it?

AFAIK this is a fork of jrcs from times when it was part of Apache Commons and
is modified by OpenGrok developers. Currently development of jrcs development
continues in the place it did before, and I am not aware of any effort to put
OpenGrok modifications back there.

> - please comment the patches that don't have comments in them

Done (except for the hg diff, which is obvious, see below).

> - why the big patch between 0.6 and this hg snapshot?  if that's actually
>   required, why not just use an hg snapshot tarball as SOURCE0 instead of 0.6
>   and patching?

About the same reason kernel package does a thing like this. I updated the
package with new revisions quite rapidly and it would not make much sense to
waste space with new tarball until patch file has sane length. It can moreover
be compressed.

New package:

http://people.redhat.com/lkundrak/SRPMS/opengrok-0.6-8.hg275.fc8.1.src.rpm
http://people.redhat.com/lkundrak/SPECS/opengrok.spec

-- 
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, or are watching someone who is.