[Bug 193894] Review Request: ant-contrib - A collection of tasks for Ant

Tue Jun 27 17:28:34 UTC 2006

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

------- Additional Comments From ifoox at redhat.com  2006-06-27 13:20 EST -------
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.

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