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: ant-contrib - A collection of tasks for Ant https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=193894 j.w.r.degoede@xxxxxx changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED AssignedTo|bugzilla-sink@xxxxxxxxxxxxx |j.w.r.degoede@xxxxxx CC|j.w.r.degoede@xxxxxx | OtherBugsDependingO|163776 |163778 nThis| | ------- Additional Comments From j.w.r.degoede@xxxxxx 2006-06-23 06:11 EST ------- 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. -- Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are the QA contact for the bug, or are watching the QA contact. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@xxxxxxxxxx http://www.redhat.com/mailman/listinfo/fedora-package-review