[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





------- Additional Comments From overholt at redhat.com  2008-04-11 14:38 EST -------
Created an attachment (id=302154)
 --> (https://bugzilla.redhat.com/attachment.cgi?id=302154&action=view)
specfile patch to remove gcj bits

Here's the full review done by Deepak Bhole and myself.  It's largely
fine.  Thanks for the submission.  For now, see the lines beginning with
an "X" and the notes section at the bottom.

MUST:
* package is named appropriately
* it is legal for Fedora to distribute this?
* license field matches the actual license.
* license is open source-compatible.
* specfile name matches %{name}
X verify source and patches
  - md5sums match.  see notes section for comments on patches
* summary and description fine
* correct buildroot
* %{?dist} used properly
* license text included in package and marked with %doc
* packages meets FHS (http://www.pathname.com/fhs/)
* rpmlint on <this package>.srpm gives no output
* changelog fine
* Packager tag should not be used
* Vendor tag should not be used
* Distribution tag should not be used
* use License and not Copyright 
* Summary tag should not end in a period
* if possible, replace PreReq with Requires(pre) and/or Requires(post)
X specfile is legible
  - I'd rather see common_reqs split out and enumerated in both Requires and
    BuildRequires
* package successfully compiles and builds on at least x86
* BuildRequires are proper
* summary should be a short and concise description of the package
* description expands upon summary
* make sure lines are <= 80 characters
  - some code lines longer; okay
* specfile written in American English
* no -doc sub-package necessary
* no static libraries
* no rpath
* config files should usually be marked with %config(noreplace)
* GUI apps should contain .desktop files
* should the package contain a -devel sub-package?
  - nope
* use macros appropriately and consistently
* don't use %makeinstall
* install section must begin with rm -rf $RPM_BUILD_ROOT or %{buildroot}
* locale data handling correct (find_lang)
X consider using cp -p to preserve timestamps
  - please use cp -pR in %install
* split Requires(pre,post) into two separate lines
* package contains code
* %files okay
* %clean should be present
* %doc files should not affect runtime
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 :)

    [localhost:SPECS]$ rpm -q --provides -p
../RPMS/noarch/opengrok-0.6-9.hg275.fc9.noarch.rpm
    config(opengrok) = 0.6-9.hg275.fc9
    opengrok = 0.6-9.hg275.fc9

    [localhost:SPECS]$ rpm -q --requires -p
../RPMS/noarch/opengrok-0.6-9.hg275.fc9.noarch.rpm
    /bin/sh  
    ant  
    bcel  
    config(opengrok) = 0.6-9.hg275.fc9
    ctags  
    jakarta-oro  
    java  
    javacc  
    jpackage-utils  
    lucene > 2
    lucene-contrib > 2
    rpmlib(CompressedFileNames) <= 3.0.4-1
    rpmlib(PayloadFilesHavePrefix) <= 4.0-1
    servlet  
    swing-layout 

    [localhost:SPECS]$ rpm -q --requires -p
../RPMS/noarch/opengrok-javadoc-0.6-9.hg275.fc9.noarch.rpm 
    rpmlib(CompressedFileNames) <= 3.0.4-1
    rpmlib(PayloadFilesHavePrefix) <= 4.0-1
    [localhost:SPECS]$ rpm -q --provides -p
../RPMS/noarch/opengrok-javadoc-0.6-9.hg275.fc9.noarch.rpm 
    opengrok-javadoc = 0.6-9.hg275.fc9


* run rpmlint on the binary RPMs

  $ rpmlint ../RPMS/noarch/opengrok-0.6-9.hg275.fc9.noarch.rpm 
  opengrok.noarch: W: uncompressed-zip /usr/share/java/opengrok-0.6.jar

  - this is fine

SHOULD:
* package should include license text in the package and mark it with %doc
* package should build on i386
? package should build in mock
  - I didn't try

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
- re-name patches to match version (0.5 -> 0.6)
- for now, remove the tomcat package as we don't have guidelines -- or even
  best practices -- for this
- why don't you build a jrcs package and Require/BR it?
- please comment the patches that don't have comments in them
- 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?

-- 
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.