Spec URL: http://people.redhat.com/ifoox/extras/ant-contrib.spec SRPM URL: http://people.redhat.com/ifoox/extras/ant-contrib-1.0b2-1jpp_1fc.src.rpm Description: The Ant-Contrib project is a collection of tasks (and at one point maybe types and other tools) for Apache Ant. I am submitting this package with several other packages (5 in total), and these are my first packages for Extras, so they may need a sponsor.
Igor, Judging from the large list of package review requests you are seriously interestd into becoming an FE contributer. In order to get sponsored you must first understand that things are currently organised in FE in such a way that once you are sponsored you get full CVS access to all packages. Thus I would like to be sure about your packaging skills before sponsoring you. I would like to propose the following: -you choose 3 packages which you prefer to have reviewed. -We work together to get these 3 reviewed and approved -Once these 3 packages are approvable you can create an account and I'll sponsor you. Does that sound like a plan? And if it does which 3 packages would you prefer to get reviewed?
Hi Hans, Thanks for looking at my submissions. Since all the packages are intertwined, and the purpose is to get Jython (#193898) built for FE, I guess we should start with the base dependencies. So I think the packages that make most sense are ht2html (#193889), ant-contrib (#this), and libreadline-java (#193896). Let me know what needs to be done. Thanks, Igor
Ok, I'll start reviewing those packages then starting with this one and sorry for being somewhat slow, I'm currently rather busy with work. Ok, here we go this is the first java package I'm reviewing so feel free to have a different opinion in certain cases: MUST: ===== * rpmlint output is: W: ant-contrib non-standard-group Development/Libraries/Java W: ant-contrib non-standard-group Development/Libraries/Java W: ant-contrib class-path-in-manifest /usr/share/java/ant-contrib-1.0b2.jar W: ant-contrib-javadoc non-standard-group Development/Documentation W: ant-contrib-javadoc dangerous-command-in-%post rm W: ant-contrib-manual non-standard-group Development/Documentation W: ant-contrib-manual wrong-file-end-of-line-encoding /usr/share/doc/ant-contrib-1.0b2/tasks/for.html W: ant-contrib-manual wrong-file-end-of-line-encoding /usr/share/doc/ant-contrib-1.0b2/tasks/foreach.html These all must be fixed! * Package and spec file named appropriately * Packaged according to packaging guidelines * License ok, license file included * spec file is legible and in Am. English. * Couldn't very if source matches upstream, sf.net gives a 500 internal serv error. * Compiles and builds on devel-x86_64 * BR: ok (see below) * No locales * No shared libraries * Not relocatable * Package owns / or requires all dirs (with some strangeness see Must fix below) * No duplicate files & Permissions ok * %clean & macro usage OK * Contains code only * %doc does not affect runtime, and isn't large enough to warrent a sub package! * no -devel package needed, no libs / .la files. * no gui -> no .desktop file required MUST fix: ========= * All rpmlint messages, see above * Remove the unused section %define * Remove "%define base_name ant-contrib", replace "Name: %{base_name}" with "Name: ant-contrib" and replace any uses of %base_name with %name I so no reason whatsoever for the existence and use of this macro accept obfuscation * For indentation / lining up the list with Name, Version .... BuildRoot you use a mix of spaces and tabs and you seem to have your tabsize set to something else then 8. Please just spaces everywhere, the indentation is a mess know in my editor. * Source1 isn't used anywhere, remove it * Remove Epoch: 0, you should not explicitly set Epoch to 0. * 1.0b2 contains alphanumeric, I don't know what the exact version scheme of upstream is, does b2 stands for beta 2, or was there first a 1.0 then a 1.0a then 1.0b, 1.0b1 and finally 1.0b2? Anyways unless upstreams creative numbering is as described above (1.0 -> 1.0a -> etc) it will break rpm's version comparison, please in that case use just 1.0 as version and and encode the additioan b2 into the release tag as described here: http://fedoraproject.org/wiki/Packaging/NamingGuidelines#head-e104844825856d7c45f2f0241586985c0495966b Also see the note about Release below under Should fix. * Replace "%setup -q -c -n %{base_name}-%{version}" with "%setup -q -n %{name}" and remove all the pushd popd nonsense as that then no longer is nescesarry * Remove the 2 find lines from %setup, the first is total nonsense and the second one doesn't do anything either as there are no jar files included. * Don't use cp to make manual backups of patched files (the 2 .sav files created). Instead pass " -z .backupext" to the %patch commands * For the manual subpackage you create %{_docdir}/%{name}-%{version} and then copy the docs there and next you put %{_docdir}/%{name}-%{version} under %files. This isn't nescesarry if you specify %doc a dir releative to %{builddir} (default /usr/src/redhat/BUILD/ant-contrib for this package) then it will create the dir, copy the files and at them to %files themselves. So: -drop the installing of these files from %install -under "%files manual" replace "%doc %{_docdir}/%{name}-%{version}" with "%doc build/docs/*". Since the license is already included and the index.html under build/docs/ contains install instructions, which we usually don't package as the rpm does the installing for the user, I would even like to plea to change this too: "%doc build/docs/tasks/*" * Don't put the manual in a seperate subpackage, its only 200k and people who really need the diskspace can tell rpm not to install anything marked %doc. * whats this with this symlink ghosting rm-ing black voodoo, why not just plain package the symlink, why is the symlink there at all? Should fix: =========== * We (Fedora) don't support building java packages using the JDK, I've checked a couple of other packages an no other has a gcj_support conditional. Please concider removing this and only leaving the gcj code in that will make things much easier to read. * The Xjpp_Yfc Release fields in other packages are only used AFAIK to allow smooth upgrade from jpackage packages to Fedora ones, since you've upgraded to a newer upstream version upgrading from jpackage to FE should nnot be a problem please use a regular 1%{?dist} release instead of 1jpp_1fc. * Why the non standard %defattr(0644,root,root,0755) under %files why not just %defattr(-,root,root,-) ? * Redundant BR (must ne removed): ant, alreayd implied by ant-junit. * Are you sure it will only build with this very specific version of junit that looks like an error to me.
Hi Hans, thanks for taking the time to review this. Here are the updated files: http://people.redhat.com/ifoox/extras/ant-contrib.spec http://people.redhat.com/ifoox/extras/ant-contrib-1.0-1.b2.src.rpm (In reply to comment #3) > MUST: > ===== > * rpmlint output is: > W: ant-contrib non-standard-group Development/Libraries/Java > W: ant-contrib non-standard-group Development/Libraries/Java > W: ant-contrib class-path-in-manifest /usr/share/java/ant-contrib-1.0b2.jar > W: ant-contrib-javadoc non-standard-group Development/Documentation > W: ant-contrib-javadoc dangerous-command-in-%post rm > W: ant-contrib-manual non-standard-group Development/Documentation > W: ant-contrib-manual wrong-file-end-of-line-encoding > /usr/share/doc/ant-contrib-1.0b2/tasks/for.html > W: ant-contrib-manual wrong-file-end-of-line-encoding > /usr/share/doc/ant-contrib-1.0b2/tasks/foreach.html > These all must be fixed! > * Package and spec file named appropriately > * Packaged according to packaging guidelines > * License ok, license file included > * spec file is legible and in Am. English. > * Couldn't very if source matches upstream, sf.net gives a 500 internal serv > error. > * Compiles and builds on devel-x86_64 > * BR: ok (see below) > * No locales > * No shared libraries > * Not relocatable > * Package owns / or requires all dirs (with some strangeness see Must fix below) > * No duplicate files & Permissions ok > * %clean & macro usage OK > * Contains code only > * %doc does not affect runtime, and isn't large enough to warrent a sub package! > * no -devel package needed, no libs / .la files. > * no gui -> no .desktop file required These are all fixed except: > W: ant-contrib class-path-in-manifest /usr/share/java/ant-contrib-1.0b2.jar Do you know what the problem with a Class-Path element in a jar's manifest is? I'm not entirely sure why rpmlint is complaining here. > MUST fix: > ========= > * All rpmlint messages, see above > * Remove the unused section %define > * Remove "%define base_name ant-contrib", replace "Name: %{base_name}" > with "Name: ant-contrib" and replace any uses of %base_name with %name > I so no reason whatsoever for the existence and use of this macro accept > obfuscation > * For indentation / lining up the list with Name, Version .... BuildRoot you > use a mix of spaces and tabs and you seem to have your tabsize set to > something else then 8. Please just spaces everywhere, the indentation is a > mess know in my editor. > * Source1 isn't used anywhere, remove it > * Remove Epoch: 0, you should not explicitly set Epoch to 0. > * 1.0b2 contains alphanumeric, I don't know what the exact version scheme > of upstream is, does b2 stands for beta 2, or was there first a 1.0 then a > 1.0a then 1.0b, 1.0b1 and finally 1.0b2? Anyways unless upstreams creative > numbering is as described above (1.0 -> 1.0a -> etc) it will break rpm's > version comparison, please in that case use just 1.0 as version and and encode > the additioan b2 into the release tag as described here: > http://fedoraproject.org/wiki/Packaging/NamingGuidelines#head-e104844825856d7c45f2f0241586985c0495966b > Also see the note about Release below under Should fix. > * Replace "%setup -q -c -n %{base_name}-%{version}" with > "%setup -q -n %{name}" and remove all the pushd popd nonsense as that then > no longer is nescesarry > * Remove the 2 find lines from %setup, the first is total nonsense and the > second one doesn't do anything either as there are no jar files included. > * Don't use cp to make manual backups of patched files (the 2 .sav files > created). Instead pass " -z .backupext" to the %patch commands > * For the manual subpackage you create %{_docdir}/%{name}-%{version} > and then copy the docs there and next you put %{_docdir}/%{name}-%{version} > under %files. This isn't nescesarry if you specify %doc a dir releative to > %{builddir} (default /usr/src/redhat/BUILD/ant-contrib for this package) then > it will create the dir, copy the files and at them to %files themselves. > So: > -drop the installing of these files from %install > -under "%files manual" replace "%doc %{_docdir}/%{name}-%{version}" with > "%doc build/docs/*". Since the license is already included and the index.html > under build/docs/ contains install instructions, which we usually don't > package as the rpm does the installing for the user, I would even like to > plea to change this too: "%doc build/docs/tasks/*" > * Don't put the manual in a seperate subpackage, its only 200k and people who > really need the diskspace can tell rpm not to install anything marked %doc. > * whats this with this symlink ghosting rm-ing black voodoo, why not just plain > package the symlink, why is the symlink there at all? These are all fixed as you described above. As for the symlink, I'm not sure why there's a symlink from a versioned docs directory to an unversioned one, but I decided not to remove it. I did take out all the weirdness with %ghosting and removing the directory and relinking. > > Should fix: > =========== > * The Xjpp_Yfc Release fields in other packages are only used AFAIK to allow > smooth upgrade from jpackage packages to Fedora ones, since you've upgraded > to a newer upstream version upgrading from jpackage to FE should nnot be a > problem please use a regular 1%{?dist} release instead of 1jpp_1fc. > * Why the non standard %defattr(0644,root,root,0755) under %files why not just > %defattr(-,root,root,-) ? > * Redundant BR (must ne removed): ant, alreayd implied by ant-junit. These are fixed. > * Are you sure it will only build with this very specific version of junit that > looks like an error to me. I'm not sure, I'll look into this. > * We (Fedora) don't support building java packages using the JDK, I've checked a > couple of other packages an no other has a gcj_support conditional. Please > concider removing this and only leaving the gcj code in that will make things > much easier to read. This is done in some packages in FC and the reason is that these packages are also built for RHEL, which currently doesn't use GCJ for java packages. It makes it significantly easier to maintain a single spec. Although I do agree that it makes the spec harder to read, I think the benefit for the maintainer of keeping one spec file overweights that.
(In reply to comment #4) > Hi Hans, thanks for taking the time to review this. > > Here are the updated files: > http://people.redhat.com/ifoox/extras/ant-contrib.spec > http://people.redhat.com/ifoox/extras/ant-contrib-1.0-1.b2.src.rpm > > > These are all fixed except: > > W: ant-contrib class-path-in-manifest /usr/share/java/ant-contrib-1.0b2.jar > > Do you know what the problem with a Class-Path element in a jar's manifest is? > I'm not entirely sure why rpmlint is complaining here. > Nope, please ask / discuss this on f-e-l this is my first java package review. About your fixes, they mostly look good, but: * you now have this: %if %{gcj_support} if [ -x %{_bindir}/rebuild-gcj-db ] then %{_bindir}/rebuild-gcj-db fi %endif Twice under %postun, please remove it once. * please dont use a patch like this: Patch3: ant-contrib-fileendings.patch instead do : "sed -i s/\r// <file1> <file2>" on the problem files. * You now have Release: 1.%{beta_number} that should be 0.1.%{beta_number}, so that you can use "Release: 1" for the final. (see wiki). * These must be removed (I missed them last time): Vendor: JPackage Project Distribution: JPackage > > These are all fixed as you described above. > As for the symlink, I'm not sure why there's a symlink from a versioned > docs directory to an unversioned one, but I decided not to remove it. I > did take out all the weirdness with %ghosting and removing the directory > and relinking. > OK. > This is done in some packages in FC and the reason is that these packages are > also built for RHEL, which currently doesn't use GCJ for java packages. It makes > it significantly easier to maintain a single spec. Although I do agree that it > makes the spec harder to read, I think the benefit for the maintainer of keeping > one spec file overweights that. > Thats a good reason, I've no problem with keeping gcj_support in that case.
Sorry for not responding for long, New files: http://people.redhat.com/ifoox/extras/ant-contrib.spec http://people.redhat.com/ifoox/extras/ant-contrib-1.0-0.1.b2.src.rpm I've fixed all the problems that you've mentioned, except the class-path in manifest file rpmlint problem, about which I sent an email to f-e-l. Thanks, Igor
(In reply to comment #6) > Sorry for not responding for long, > > New files: > http://people.redhat.com/ifoox/extras/ant-contrib.spec > http://people.redhat.com/ifoox/extras/ant-contrib-1.0-0.1.b2.src.rpm Is this file corrupt? I get the following why I try to install.. error: unpacking of archive failed on file /usr/src/redhat/SOURCES/ant-contrib-1.0b2-src.tar.gz;44b6deeb: cpio: read
Your SRPM indeed seems corrupt, but thats no problem since all I need in this stage of the review is the spec. Everything looks good now. About the classpath in the .jar. I've followed the discusion on f-e-l and the java knowledge I once possesed is slowly coming back. I think that the classpath in this caseis completly useless. A classpath in a manifest is afaik to indicate that that jar needs other classes/jars loaded to function, so refering to yourself is useless. Could you try completly removing the classpath line from the jar and then see if things till works. Except for the classpath issue this package is approvable. I would like us to walk through the review of one other package before I sponsor you, which one would you like me to review?
Seems like my upload was not successful, I updated the file and it's working now. Hans, I agree that ant-contrib.jar referring to itself seems very useless, I'll try removing that later today and see how it flies.
Hans, h2html seems close: https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=193889 I think Igor is ready for sponsorship now, and I'll take care of it if you want. Either way is fine with me.
New files: http://people.redhat.com/ifoox/extras/ant-contrib-1.0-0.2.b2.src.rpm http://people.redhat.com/ifoox/extras/ant-contrib.spec I removed the class-path from the manifest file and it seems to be fine.
(In reply to comment #10) > Hans, h2html seems close: > https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=193889 > > I think Igor is ready for sponsorship now, and I'll take care of it if you want. > Either way is fine with me. I'm rather busy with other FE stuff at the moment, so if you could sponsor Igor and help him with the other reviews that would be great!
I see that you've been sponsored by Tibbs some time ago, as I alreadyu said this packages looks good -> Approved! Feel free to import and build this.
After a long delay I was planning to build this package but ran into another problem, maybe someone here will be able to figure out what's going on. Here is the new src.rpm: http://people.redhat.com/ifoox/extras/ant-contrib-1.0-0.3.b2.src.rpm When I run rpmlint on the resulting binary I get the following warning: W: ant-contrib unstripped-binary-or-object /usr/lib/gcj/ant-contrib/ant-contrib-1.0.jar.so I'm not really sure why rpmbuild is not stripping this object as always, and also doesn't produce a -debuginfo pacakge. Any suggestions?
Check the permissions on that file. They must be 755 on a dynamic lib for rpm to properly strip/create debuginfo.
$ ll /var/tmp/ant-contrib-1.0-0.3.b2-root-ifoox/usr/lib/gcj/ant-contrib/ant-contrib-1.0.jar.so -rwxr-xr-x 1 ifoox ifoox 1356724 Sep 2 15:53 /var/tmp/ant-contrib-1.0-0.3.b2-root-ifoox/usr/lib/gcj/ant-contrib/ant-contrib-1.0.jar.so So it looks like valid 755 permissions.
It seems like this is a problem with my system. Every source RPM I try to build does not get a -debuginfo package. Building this on a different machine produces normal results so I have imported this into CVS and built it for the devel branch . I'll build it for FC5 as well as soon as the branch gets crated. Closing.
(In reply to comment #17) > It seems like this is a problem with my system. Every source RPM I try to build > does not get a -debuginfo package. Installing the redhat-rpm-config package should fix that.
Can you build this for EL-5? I need it to update jmol to the 11.8 series.
*** Bug 227027 has been marked as a duplicate of this bug. ***
Package Change Request ====================== Package Name: ant-contrib New Branches: EL-5 Owners: orion Alexander - do you want to own the EL branch too?
I don't see any answer after a week here, so going ahead with the request. cvs done.
EL-5 update request has been made. Thanks.