Bug 433312 (opengrok-review) - Review Request: opengrok - A wicked fast source browser
Summary: Review Request: opengrok - A wicked fast source browser
Keywords:
Status: CLOSED NEXTRELEASE
Alias: opengrok-review
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Lubomir Rintel
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: lucene-2.3.0 jflex-1.4.1 433295 swing-layout-review
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-02-18 15:49 UTC by Lubomir Kundrak
Modified: 2009-01-07 18:09 UTC (History)
3 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2008-04-19 06:30:36 UTC
Type: ---
Embargoed:
overholt: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)
specfile patch to remove gcj bits (1.99 KB, patch)
2008-04-11 18:38 UTC, Andrew Overholt
no flags Details | Diff

Description Lubomir Kundrak 2008-02-18 15:49:14 UTC
Spec URL: http://people.redhat.com/lkundrak/SPECS/opengrok.spec
SRPM URL:
http://people.redhat.com/lkundrak/mock-results/opengrok-0.5-2.c4dea0135445.fc8.noarch/opengrok-0.5-2.c4dea0135445.fc8.src.rpm
mock:
http://people.redhat.com/lkundrak/mock-results/opengrok-0.5-2.c4dea0135445.fc8.noarch/

Description: A wicked fast source browser

OpenGrok is a fast and usable source code search and cross reference
engine, written in Java. It helps you search, cross-reference and navigate
your source tree. It can understand various program file formats and
version control histories like SCCS, RCS, CVS, Subversion and Mercurial.

Comment 1 Lubomir Kundrak 2008-02-18 16:00:03 UTC
Note: This is by no means complete. This reports exists to keep track of
packaging progress and to simplify cooperation on packaging.

Comments from experienced Java packagers are very welcome.

If you aware that any of these are already packaged please make a note here:
	ant-tools
	lucene-core 2.2.0
	lucene-spellchecker 2.2.0
	servlet-api
	swing-layout 0.9

JRCS is no longer part of Apache commons. But OpenGrok can not use the new ones
anyways as they reportedly don't include necessary fixes. The opengrok source
package contains ./ext/jrcs.zip that must be used to build these (we can
fearlessly install them, as they won't clash with anything existing for sure):
	org.apache.commons.jrcs.diff
	org.apache.commons.jrcs.rcs

To build it you need it itself [1] and newer jflex [2] (which is covered by bug
#433272) (source to it is in the same directory, needs rawhide java_cup to compile).

[1]
http://people.redhat.com/lkundrak/mock-results/opengrok-0.5-2.c4dea0135445.fc8.noarch/opengrok-0.5-2.c4dea0135445.fc8.noarch.rpm
[2] http://people.redhat.com/lkundrak/jflex/jflex-1.4.1-2jpp.2.fc8.noarch.rpm

Comment 2 Lubomir Kundrak 2008-02-18 16:01:31 UTC
And -- this won't run, I forgot %{_bindir}/opengrok from older version. I am
aware of this and will fix it once I roll new packages:

/usr/bin/build-classpath: error: Could not find lucene-core-2.1.0.jar Java
extension for this JVM
/usr/bin/build-classpath: error: Could not find lucene-spellchecker-2.1.0.jar
Java extension for this JVM
/usr/bin/build-classpath: error: Could not find svn-javahl.jar Java extension
for this JVM
/usr/bin/build-classpath: error: Some specified jars were not found

Comment 3 Lubomir Kundrak 2008-02-18 21:14:07 UTC
opengrok-0.5-3.c4dea0135445
http://people.redhat.com/lkundrak/opengrok/

	ant-tools
	lucene-core 2.2.0
	lucene-spellchecker 2.2.0
-	servlet-api
	swing-layout 0.9
-	org.apache.commons.jrcs.diff
-	org.apache.commons.jrcs.rcs

Comment 4 Lubomir Kundrak 2008-02-19 16:55:49 UTC
opengrok-0.5-4.c4dea0135445
http://people.redhat.com/lkundrak/opengrok/

	ant-tools
-	lucene-core 2.2.0
-	lucene-spellchecker 2.2.0
	swing-layout 0.9

Needs newer lucene (bug #228141). Grab it from [1] if it builds or (gjc-less
noarch from here [2]).

[1] http://koji.fedoraproject.org/koji/taskinfo?taskID=442802
[2] http://people.redhat.com/lkundrak/lucene/

Comment 5 Lubomir Kundrak 2008-02-20 09:16:23 UTC
mock:
http://people.redhat.com/lkundrak/mock-results/opengrok-0.5-5.c4dea0135445.fc8.noarch/
SRPM:
http://people.redhat.com/lkundrak/mock-results/opengrok-0.5-5.c4dea0135445.fc8.noarch/opengrok-0.5-5.c4dea0135445.fc8.src.rpm
SPEC: http://people.redhat.com/lkundrak/SPECS/opengrok.spec

-	ant-tools
-	swing-layout 0.9

TODO:
* Package war (how to do so? Is the way subversion does that sane? Not.)
* .desktop

Comment 6 Lubomir Kundrak 2008-02-20 22:58:05 UTC
mock:
http://people.redhat.com/lkundrak/mock-results/opengrok-0.5-6.e3806d642190.fc8.noarch/
SRPM:
http://people.redhat.com/lkundrak/mock-results/opengrok-0.5-6.e3806d642190.fc8.noarch/opengrok-0.5-6.e3806d642190.fc8.src.rpm
SPEC: http://people.redhat.com/lkundrak/SPECS/opengrok.spec

Ready for review.

Known outstanding issues:
1.) webapp doesn't work. Probably broken by upstream in snapshot. Will
investigate more.
2.) SVN jar thingie; see blockers
3.) Dependencies; see blockers

RPMlint:
opengrok.noarch: W: dangling-symlink /usr/share/java/svn-javahl.jar
/usr/lib64/svn-javahl/svn-javahl.jar
opengrok.noarch: W: symlink-should-be-relative /usr/share/java/svn-javahl.jar
/usr/lib64/svn-javahl/svn-javahl.jar

Both are related to bug #433295, it will go away once the bug is fixed.

Rationale behind using the snapshot is features -- 0.5 can't be reconfigured
without modifying WAR.

Comment 7 Lubomir Kundrak 2008-04-03 09:25:52 UTC
Until the package gets to Fedora, work gets done in another place. You can get
the package sources from Mercurial repo:

hg clone ssh://fedorapeople.org//home/fedora/lkundrak/repo/opengrok

The built binaries appear here:

http://people.redhat.com/lkundrak/opengrok/
http://people.redhat.com/lkundrak/opengrok-el5/

Comment 8 Lubomir Kundrak 2008-04-09 15:43:56 UTC
Sooo, new snapshot aims at compliance with current Java guidelines, ready for
review:

http://people.redhat.com/lkundrak/SPECS/opengrok.spec
http://people.redhat.com/lkundrak/mock-results/opengrok-0.6-8.hg275.fc9.x86_64/opengrok-0.6-8.hg275.fc9.src.rpm
http://people.redhat.com/lkundrak/mock-results/opengrok-0.6-8.hg275.fc9.x86_64/

Known issues:
* Does not compile with GCJ 1.5, I will probably need assistance from someone
who is better familiar with Java language and tools
* Web application package is a crap, and the guidelines are not finished yet. It
can eventually be omitted and reviewed separately once guidelines are finished.

RPMLint:
Quiet, except for -tomcat5 subpackage, see above.

Comment 9 Andrew Overholt 2008-04-11 18:38:07 UTC
Created attachment 302154 [details]
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?

Comment 10 Lubomir Kundrak 2008-04-12 14:30:56 UTC
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

Comment 11 Andrew Overholt 2008-04-15 20:08:41 UTC
(In reply to comment #10)
> (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.

It's not a blocker for us, so if you think it'll be easier to maintain, go for it :)

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

Yeah, it wasn't using libgcj's class library to build before.

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

Okay, if it was generated against 0.5 then keep it.  I guess my question is why
it wasn't applied upstream :)  I like to keep bugs and/or rationale in comments
to denote why we're carrying patches ... but don't worry about it if you don't
want to do it.

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

Okay, well as long as it's building from source (as you have it doing), it's
fine then.

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

Alright, I guess that's acceptable.

> 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

Okay, just two more things:

What's going on with this?

javadoc: error - Error while reading file
/home/overholt/rpmbuild/BUILD/opengrok-0.6-src/jrcs/src/java/org/apache/commons/jrcs/overview.html

And I don't think it should be trying to access the internet using hg during the
build:

-hg-get-changeset:
     [exec] Execute failed: java.io.IOException: Cannot run program "hg":
java.io.IOException: error=2, No such file or directory

Comment 12 Lubomir Kundrak 2008-04-15 22:38:07 UTC
> > 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.
> 
> Okay, if it was generated against 0.5 then keep it.  I guess my question is why
> it wasn't applied upstream :)  I like to keep bugs and/or rationale in comments
> to denote why we're carrying patches ... but don't worry about it if you don't
> want to do it.

Upstream is very responsive to problems so it won't be a big problem. Most of
the patches (probably with exception of Patch0 and at least of the two
addressing the issues below) are things that are specific to our build -- it can
be hardly imaginable that upstream will omit foreign jar files from the
distribution, etc. When submitting patches upstream I usually direct them to our
viewcvs, so they can eventually pick patches other that ones that I propose if 
they want to -- I'll probably do that in this case too.

> Okay, just two more things:
> 
> What's going on with this?
> 
> javadoc: error - Error while reading file
>
/home/overholt/rpmbuild/BUILD/opengrok-0.6-src/jrcs/src/java/org/apache/commons/jrcs/overview.html

Not really fatal and doesn't seem to do any harm to the build, but for it being
an annoyance I patched that away.

> And I don't think it should be trying to access the internet using hg during the
> build:
> 
> -hg-get-changeset:
>      [exec] Execute failed: java.io.IOException: Cannot run program "hg":
> java.io.IOException: error=2, No such file or directory

Right. I just commented out that target. I guess the right solution (understand:
one acceptable by upstream) would be to look for existence of .hg directory
before trying to speak upstream. (Now I realize -- maybe hg would look into the
local respoitory only, it's a distributed vcs, but I am not sure).

New bits:

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

Comment 13 Andrew Overholt 2008-04-16 16:35:52 UTC
Thanks, Lubomir!

This package is APPROVED (by myself and Deepak Bhole).

Comment 14 Lubomir Kundrak 2008-04-17 07:18:16 UTC
New Package CVS Request
=======================
Package Name: opengrok
Short Description: Source browser and indexer
Owners: lkundrak
Branches: EL-5
Cvsextras Commits: yes

Comment 15 Kevin Fenzi 2008-04-18 16:27:11 UTC
cvs done.

Comment 16 Lubomir Kundrak 2008-04-19 06:30:36 UTC
Thanks for the review, Andrew and Deepak;
thanks for CVS, Kevin;
imported and built.


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