Bug 176981 - Review Request: itext - a PDF creation library in java
Review Request: itext - a PDF creation library in java
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Chris Chabot
David Lawrence
:
Depends On:
Blocks: FE-ACCEPT 176982
  Show dependency treegraph
 
Reported: 2006-01-04 17:45 EST by Anthony Green
Modified: 2007-11-30 17:11 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-01-17 09:25:13 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Missing classes errors with javadoc (20.50 KB, text/plain)
2006-01-16 20:22 EST, Chris Chabot
no flags Details
Complete buildlog of itext in fedora-development-i386 mock build (268.50 KB, text/plain)
2006-01-16 20:23 EST, Chris Chabot
no flags Details

  None (edit)
Description Anthony Green 2006-01-04 17:45:46 EST
Spec Name or Url: http://people.redhat.com/green/FE/devel/itext.spec
SRPM Name or Url: http://people.redhat.com/green/FE/devel/itext-1.3-1jpp_2.src.rpm

Description: 
iText is a library that allows you to generate
PDF files on the fly. The iText classes are very
useful for people who need to generate read-only,
platform independent documents containing text,
lists, tables and images. The library is especially
useful in combination with Java(TM) technology-based
Servlets: The look and feel of HTML is browser
dependent; with iText and PDF you can control
exactly how your servlet's output will look.


This package is required by RSSOwl, which I'm also about submit.

The upstream package was maintained by the JPackage group.  I've tweaked it to work with our Free stack, and to generate native code.
Comment 1 Jeremy Katz 2006-01-10 15:06:50 EST
A few questions/comments before doing a full review:

1) What's the purpose of the section define at the top of the spec file?  I'm
assuming it's somehow related to jpackage...
2) We tend to avoid doing, eg, jpp or the like in release tags
3) Using %{summary}. for summaries is odd -- that doesn't describe what the
purpose of the subpackage is.  Also, you'll end up with double periods like that
4) You shouldn't need the javadoc %postun as that should get handled by the fact
that the file is ghosted.
Comment 2 Anthony Green 2006-01-13 07:33:55 EST
(In reply to comment #1)
> A few questions/comments before doing a full review:

Thanks for having a look and sorry for the delay.  I have been travelling.

> 1) What's the purpose of the section define at the top of the spec file?  I'm
> assuming it's somehow related to jpackage...

Yes, this package has been borrowed from the JPackage Project with minimal
changes.  The JPackage provides both Free and non-Free Software packages.  This
is how they indicate which section the package belongs to.  I can remove this
from the FE package, but my goal was to make as few changes as possible in order
to ease merging down the road.

> 2) We tend to avoid doing, eg, jpp or the like in release tags

All of the packages in Fedora Core that have are derived from JPackage Project
packages have jpp in the release tag.  I'm not sure I agree that it's a good
idea, but I was just following FC existing practice.

> 3) Using %{summary}. for summaries is odd -- that doesn't describe what the
> purpose of the subpackage is.  Also, you'll end up with double periods like that

I agree.  I'll fix that and submit the fix to the JPackage project as well.

> 4) You shouldn't need the javadoc %postun as that should get handled by the fact
> that the file is ghosted.

Ok, same with this.

Thanks - I'll post a new package shortly.
Comment 3 Anthony Green 2006-01-13 08:24:41 EST
Here are the updated files for review...

Spec Name or Url: http://people.redhat.com/green/FE/devel/itext.spec
SRPM Name or Url: http://people.redhat.com/green/FE/devel/itext-1.3-1jpp_3.src.rpm

Thanks,

AG
Comment 4 Chris Chabot 2006-01-16 17:43:09 EST
Picking this one up on green's request, changing to FE-REVIEW. Green please
assign the bug to me?

rpmlint on the itext packages shows:

W: itext non-standard-group Development/Libraries/Java
W: itext-javadoc non-standard-group Development/Documentation
W: itext-manual non-standard-group Development/Documentation

Please pick one of FE's standard groups:  
http://fedoraproject.org/wiki/RPMGroups

Development/Libraries and Documentation seem to be the proper 2 to choose

W: itext-manual invalid-license Mozilla Public License & LGPL
W: itext invalid-license Mozilla Public License & LGPL
W: itext-javadoc invalid-license Mozilla Public License & LGPL
Ignorable error,licence is valid and fedora compatible

W: itext incoherent-version-in-changelog 0:1.3-1jpp_3fc 1.3-1jpp_3
Please correct this, versions have to be the same, please remove the 'fc' part
from the changelog

W: itext-javadoc dangerous-command-in-%post rm
%post javadoc does confuse me, the scripplet:
  rm -f %{_javadocdir}/%{name}                                                 
                          
  ln -s %{name}-%{version} %{_javadocdir}/%{name}

What are you trying to do here, isn't there a way to get the file locations
correct in the %install without doing dangerous rm's and confusing links (with
then not-owned) to directories? On removing this package this will result into a
dead link

W: itext wrong-file-end-of-line-encoding /usr/share/doc/itext-1.3/lgpl.txt
W: itext wrong-file-end-of-line-encoding /usr/share/doc/itext-1.3/MPL-1.1.txt
W: itext-manual wrong-file-end-of-line-encoding /usr/share/doc/itext-1.3/lgpl.txt
W: itext-manual wrong-file-end-of-line-encoding /usr/share/doc/itext-1.3/build.xml
W: itext-manual wrong-file-end-of-line-encoding /usr/share/doc/itext-1.3/MPL-1.1.txt
W: itext-manual hidden-file-or-dir /usr/share/doc/itext-1.3/ant/.ant.properties
W: itext-manual wrong-file-end-of-line-encoding
/usr/share/doc/itext-1.3/ant/.ant.properties
W: itext-manual wrong-file-end-of-line-encoding
/usr/share/doc/itext-1.3/ant/download.xml
W: itext-manual wrong-file-end-of-line-encoding
/usr/share/doc/itext-1.3/ant/site.xml
E: itext-manual standard-dir-owned-by-package /usr/share/doc
W: itext-manual wrong-file-end-of-line-encoding
/usr/share/doc/itext-1.3/ant/release.xml
W: itext-manual wrong-file-end-of-line-encoding
/usr/share/doc/itext-1.3/ant/compile.xml
This is the same error as in rssowl, please add dos2unix to the
BuildRequirements, and dos2unix them in the %install section.

I agree with katz that the jpp in the version is not elegant, but looking at the
standard FC development repo, it seems all java packages/libraries suffer from
this, so i guess its consitent in its own way :-)

Identation of the header part seems very confusing, your mixing tabs and spaces
i think, and my tabsize (4) is probably not the same as yours, please use spaces
instead of tabs, it avoids such confusion.

Would it maybe be an idea to combine the javadoc and manual packages? Normally
documentation is in the -devel package, but seeing how this has none thats not
an option. However having 2 'doc' packages seems a bit to much to me (but i have
no inside knowledge of these docs, so if there are very good reasons then thats
acceptable, but please put them in a comment in the specfile please)

I'll post the whole formal review list once these issues are addressed.
Comment 5 Anthony Green 2006-01-16 19:39:19 EST
(In reply to comment #4)
> Picking this one up on green's request, changing to FE-REVIEW. Green please
> assign the bug to me?

Done.  New versions of the files are here:

Spec Name or Url: http://people.redhat.com/green/FE/devel/itext.spec
SRPM Name or Url: http://people.redhat.com/green/FE/devel/itext-1.3-1jpp_4.src.rpm

> W: itext-javadoc dangerous-command-in-%post rm
> %post javadoc does confuse me, the scripplet:
>   rm -f %{_javadocdir}/%{name}                                                 
>                           
>   ln -s %{name}-%{version} %{_javadocdir}/%{name}
> 
> What are you trying to do here, isn't there a way to get the file locations
> correct in the %install without doing dangerous rm's and confusing links (with
> then not-owned) to directories? On removing this package this will result into a
> dead link

This was copied from the JPackage version of this package.  I just removed the
unversioned directory to avoid this whole problem.

> Would it maybe be an idea to combine the javadoc and manual packages? Normally
> documentation is in the -devel package, but seeing how this has none thats not
> an option. However having 2 'doc' packages seems a bit to much to me (but i have
> no inside knowledge of these docs, so if there are very good reasons then thats
> acceptable, but please put them in a comment in the specfile please)

I'm not sure if there is a good reason.  This is just standard practice for
JPackage packages.  javadoc always goes in its own package.

Thanks!

AG
Comment 6 Chris Chabot 2006-01-16 20:07:03 EST
Is it still worth having the %define section free in the spec file? Or is it
there to keep it as close to the JPackage one as possible? Kind of looks out of
place there :-)

Licence is often written as 'MPL' and not fully "Mozilla Public Licence" (just
as GPL/BSD/LGPL etc are written shorthand), mozilla its self uses MPL too so i
think thats the example to follow, also by examples of other packages i think
the proper format would be: MPL/LGPL

Still one inconsitent version in the changelog according to rpmlint:
W: itext incoherent-version-in-changelog 0:1.3-1jpp_4 1.3-1jpp_4

rpmlint Group warnings:
W: itext-javadoc non-standard-group Development/Documentation
W: itext-manual non-standard-group Development/Documentation
Its either Development/Libraries, or "Documentation", Development/Documentation
unfortunatly doesn't exist. Please use "Documenation" for the 2 sub packages
with the docs.

other rpmlint warning:
W: itext-manual hidden-file-or-dir /usr/share/doc/itext-1.3/ant/.ant.properties
But this is ignorable 

The package also owns / inclues:
/usr/share/java/itext
But this is an empty directory .. is there any reason for its being there? If
not please remove

review list so far:

MUST review items:
- Builds cleanly on FC5 devel.
- Source included matches upsteam source (md5sum)
- Package name meets guidelines
- spec file name is in %{name}.spec format
- Licence (MPL/LGPL) is fedora extra's compatible & is included
- Spec file is in (american) english
- Does not list buildrequires that are excepted in the package guidelines
- All build dependencies are listed
- No ldconfig needed
- All files have proper permissions
- Package is not relocatable
- No duplicate files in %files section (but empty dir present/owned)
- No missing files in %files section
- Has a proper %clean section with rm -rf $RPM_BUILD_ROOT
- Uses macro's described in PackagingGuidelines
- No entries in %doc that are required for standard program operation
- No -devel package needed (though using javadoc/manual packages there not devel
packages with .a/.so/.h files)
- No directory-ownerships needed
- No gui app, so no need for a desktop file

Should items:
- Includes upstream licence(s) file (MPL-1.1.txt and lgpl.txt)
- No insane scriplets
- No unnescesarry requires

I'm running a mock build to verify in the meantime, but that will take a few
more minutes to complete. Meantime please address the above mentioned issues and
we'll be very close to FE-ACCEPT!
Comment 7 Chris Chabot 2006-01-16 20:20:35 EST
Mock build unfortunatly failed, i think there's a build requires missing that
causes javadoc to become upset.

RPM building fails on:
+ mkdir -p /var/tmp/itext-1.3-1jpp_4-root-mockbuild/usr/share/javadoc/itext-1.3
+ cp -pr 'build/docs/*'
/var/tmp/itext-1.3-1jpp_4-root-mockbuild/usr/share/javadoc/itext-1.3
cp: cannot stat `build/docs/*': No such file or directory

Looking at the (huge) build log what jumps out is:
  [javadoc] WARNING: Cannot locate class java.io.IOException referenced in class
com.lowagie.bc.asn1.DERU

Well and this goes on and on .. i've made a text file from the log with only the
javadoc build errors (missing-classes.txt) with:

  cat build.log |grep  "WARNING: Cannot locate class"  > missing-classes.txt

which i will attach together with the complete mock build log.

These will be have to addressed before i can approve
Comment 8 Chris Chabot 2006-01-16 20:22:04 EST
Created attachment 123277 [details]
Missing classes errors with javadoc

Grep on build log of Missing Classes warnings by javadoc
Comment 9 Chris Chabot 2006-01-16 20:23:16 EST
Created attachment 123278 [details]
Complete buildlog of itext in fedora-development-i386 mock build

Complete buildlog of itext in fedora-development-i386 mock build
Comment 10 Anthony Green 2006-01-16 20:35:31 EST
(In reply to comment #7)
> Looking at the (huge) build log what jumps out is:
>   [javadoc] WARNING: Cannot locate class java.io.IOException referenced in class
> com.lowagie.bc.asn1.DERU
> 
> Well and this goes on and on .. i've made a text file from the log with only the
> javadoc build errors (missing-classes.txt) with:

This is normal.  I'm trying to figure out what the real problems is right now.

AG
Comment 11 Chris Chabot 2006-01-16 20:40:35 EST
Looking at the build logs there's also a part with HTTP connections and
exceptions, or so it seems, at this part:

  [javadoc] java.lang.NullPointerException
  [javadoc]    at gnu.java.net.PlainSocketImpl.connect (libgcj.so.7)
  [javadoc]    at java.net.Socket.connect (libgcj.so.7)
  [javadoc]    at java.net.Socket.connect (libgcj.so.7)
  [javadoc]    at gnu.java.net.protocol.http.HTTPConnection.getSocket (libgcj.so.7)
  [javadoc]    at gnu.java.net.protocol.http.HTTPConnection.getOutputStream
(libgcj.so.7)
  [javadoc]    at gnu.java.net.protocol.http.Request.dispatch (libgcj.so.7)
  [javadoc]    at gnu.java.net.protocol.http.HTTPURLConnection.connect (libgcj.so.7)
  [javadoc]    at gnu.java.net.protocol.http.HTTPURLConnection.getInputStream
(libgcj.so.7)
  [javadoc]    at java.net.URL.openStream (libgcj.so.7)
  [javadoc]    at gnu.classpath.tools.doclets.htmldoclet.ExternalDocSet.load
(lib-gnu-classpath-tools-gjdoc.so.0)
  [javadoc]    at gnu.classpath.tools.doclets.htmldoclet.HtmlDoclet.run
(lib-gnu-classpath-tools-gjdoc.so.0)
  [javadoc]    at gnu.classpath.tools.doclets.AbstractDoclet.startInstance
(lib-gnu-classpath-tools-gjdoc.so.0)
  [javadoc]    at gnu.classpath.tools.doclets.AbstractDoclet.start
(lib-gnu-classpath-tools-gjdoc.so.0)
  [javadoc]    at java.lang.reflect.Method.invoke (libgcj.so.7)
  [javadoc]    at gnu.classpath.tools.gjdoc.Main.startDoclet
(lib-gnu-classpath-tools-gjdoc.so.0)
  [javadoc]    at gnu.classpath.tools.gjdoc.Main.start
(lib-gnu-classpath-tools-gjdoc.so.0)
  [javadoc]    at gnu.classpath.tools.gjdoc.Main.main
(lib-gnu-classpath-tools-gjdoc.so.0)

Maybe thats related?

Anyhow no rush i think i'm calling it a night, will take a look at your fixing
results in about 9 hours during my first cup of coffee :-)
Comment 12 Chris Chabot 2006-01-16 20:45:43 EST
Ps i'm not sure about this, just theoretical, but a mock build envirioment does
a chroot to a virtual install root, it does mount /proc and devfs, but it won't
have any host files, so if it tries to resolve the hostname and connect to it,
it would fail ..

Anyhow i have no idea how javadoc works, just guessing :-)

G'night!
Comment 13 Anthony Green 2006-01-16 20:49:46 EST
(In reply to comment #11)
> Looking at the build logs there's also a part with HTTP connections and
> exceptions, or so it seems, at this part:
>

Yes, I found this as well.  It was the problem.  I've added a patch to disable
external javadoc links, am rebuilding now.  Will post new files in 10min or so.

Thanks!

AG
Comment 14 Anthony Green 2006-01-16 21:03:41 EST
New files are here:
Spec Name or Url: http://people.redhat.com/green/FE/devel/itext.spec
SRPM Name or Url: http://people.redhat.com/green/FE/devel/itext-1.3-1jpp_5.src.rpm


(In reply to comment #6)
> Is it still worth having the %define section free in the spec file? Or is it
> there to keep it as close to the JPackage one as possible? Kind of looks out of
> place there :-)

I'm trying to make as few changes as possible, and will send those changes I
make back to the JPackage maintainer for itext.

> Licence is often written as 'MPL' and not fully "Mozilla Public Licence" (just
> as GPL/BSD/LGPL etc are written shorthand), mozilla its self uses MPL too so i
> think thats the example to follow, also by examples of other packages i think
> the proper format would be: MPL/LGPL

Ok.

> Still one inconsitent version in the changelog according to rpmlint:
> W: itext incoherent-version-in-changelog 0:1.3-1jpp_4 1.3-1jpp_4

I can't make this one go away (although now it says _5).

> rpmlint Group warnings:
> W: itext-javadoc non-standard-group Development/Documentation
> W: itext-manual non-standard-group Development/Documentation
> Its either Development/Libraries, or "Documentation", Development/Documentation
> unfortunatly doesn't exist. Please use "Documenation" for the 2 sub packages
> with the docs.

Fixed.

> other rpmlint warning:
> W: itext-manual hidden-file-or-dir /usr/share/doc/itext-1.3/ant/.ant.properties
> But this is ignorable 
> 
> The package also owns / inclues:
> /usr/share/java/itext
> But this is an empty directory .. is there any reason for its being there? If
> not please remove

Removed.

Thanks!

AG
Comment 15 Chris Chabot 2006-01-17 05:01:36 EST
(In reply to comment #14)

> > Still one inconsitent version in the changelog according to rpmlint:
> > W: itext incoherent-version-in-changelog 0:1.3-1jpp_4 1.3-1jpp_4
> 
> I can't make this one go away (although now it says _5).

Actually the error is in the 0:... part of the version, the 0: is the 'epoch' of
the package, however this package doesn't have an Eproch: field so hence the
confusion (Epoch == NULL != 0).

Epochs in general are evil and to be avoided except in extreme cases (ie only
time its worth using is if forinstance you pushed a newer version to the repo's
which has a severe corruption error, and can't be fixed in the newer version,
then you release a older package with an Epoch: 1 to update over the newer package)

Remove the 0: and rpmlint should stop complaining.

Building and then mocking the package now, will post the full checklist in a few
minutes.
Comment 16 Chris Chabot 2006-01-17 05:20:28 EST
rpmlint is now completely quiet, not a single warning or error :-) All the
mentioned above issues have been addressed.

Formal review list:

MUST review items:
- Builds cleanly on FC5 devel.
- Source included matches upsteam source (md5sum)
- Package name meets guidelines
- spec file name is in %{name}.spec format
- Licence (MPL/LGPL) is fedora extra's compatible & is included
- Spec file is in (american) english
- Does not list buildrequires that are excepted in the package guidelines
- All build dependencies are listed
- No ldconfig needed
- All files have proper permissions
- Package is not relocatable
- No duplicate files in %files section
- No missing files in %files section
- Has a proper %clean section with rm -rf $RPM_BUILD_ROOT
- Uses macro's described in PackagingGuidelines
- No entries in %doc that are required for standard program operation
- No -devel package needed (though using javadoc/manual packages there not devel
packages with .a/.so/.h files)
- No directory-ownerships needed
- No gui app, so no need for a desktop file

Should items:
- Includes upstream licence(s) file (MPL-1.1.txt and lgpl.txt)
- No insane scriplets
- No unnescesarry requires
- Mock builds cleanly now too

Great work! FE-APPROVED!

When you have cvs-import'ed, make tag && make build this package please close
the bug with NEXTRELEASE as resolution (and now you can import rssowl too and do
the same for it as soon as this is done building/being pushed to extras-development)
Comment 17 Anthony Green 2006-01-17 09:25:13 EST
Thanks for you help Chris!

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