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@xxxxxxxxxx changed: What |Removed |Added ---------------------------------------------------------------------------- AssignedTo|lkundrak@xxxxxxxxxx |overholt@xxxxxxxxxx ------- Additional Comments From lkundrak@xxxxxxxxxx 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. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@xxxxxxxxxx http://www.redhat.com/mailman/listinfo/fedora-package-review