Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 176981
Summary: | Review Request: itext - a PDF creation library in java | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Anthony Green <green> | ||||||
Component: | Package Review | Assignee: | Chris Chabot <chabotc> | ||||||
Status: | CLOSED NEXTRELEASE | QA Contact: | David Lawrence <dkl> | ||||||
Severity: | medium | Docs Contact: | |||||||
Priority: | medium | ||||||||
Version: | rawhide | CC: | chabotc, fedora-extras-list, katzj | ||||||
Target Milestone: | --- | ||||||||
Target Release: | --- | ||||||||
Hardware: | All | ||||||||
OS: | Linux | ||||||||
Whiteboard: | |||||||||
Fixed In Version: | Doc Type: | Bug Fix | |||||||
Doc Text: | Story Points: | --- | |||||||
Clone Of: | Environment: | ||||||||
Last Closed: | 2006-01-17 14:25:13 UTC | Type: | --- | ||||||
Regression: | --- | Mount Type: | --- | ||||||
Documentation: | --- | CRM: | |||||||
Verified Versions: | Category: | --- | |||||||
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||||
Cloudforms Team: | --- | Target Upstream Version: | |||||||
Embargoed: | |||||||||
Bug Depends On: | |||||||||
Bug Blocks: | 163779, 176982 | ||||||||
Attachments: |
|
Description
Anthony Green
2006-01-04 22:45:46 UTC
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. (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. 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 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. (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 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! 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 Created attachment 123277 [details]
Missing classes errors with javadoc
Grep on build log of Missing Classes warnings by javadoc
Created attachment 123278 [details]
Complete buildlog of itext in fedora-development-i386 mock build
Complete buildlog of itext in fedora-development-i386 mock build
(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 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 :-) 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! (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 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 (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. 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) Thanks for you help Chris! |