Bug 193894 - Review Request: ant-contrib - A collection of tasks for Ant
Review Request: ant-contrib - A collection of tasks for Ant
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Hans de Goede
Fedora Package Reviews List
:
: 227027 (view as bug list)
Depends On:
Blocks: FE-ACCEPT 193897
  Show dependency treegraph
 
Reported: 2006-06-02 15:06 EDT by Igor Foox
Modified: 2009-12-22 11:49 EST (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-09-03 21:45:54 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Igor Foox 2006-06-02 15:06:18 EDT
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.
Comment 1 Hans de Goede 2006-06-17 08:57:27 EDT
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?
Comment 2 Igor Foox 2006-06-21 10:42:49 EDT
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
Comment 3 Hans de Goede 2006-06-23 06:11:06 EDT
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.
Comment 4 Igor Foox 2006-06-27 13:20:06 EDT
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.
Comment 5 Hans de Goede 2006-06-29 11:54:40 EDT
(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.
Comment 6 Igor Foox 2006-07-12 14:34:37 EDT
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
Comment 7 Anthony Green 2006-07-13 19:54:35 EDT
(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
Comment 8 Hans de Goede 2006-07-14 17:01:04 EDT
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?


Comment 9 Igor Foox 2006-07-17 10:10:29 EDT
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.
Comment 10 Jason Tibbitts 2006-07-20 14:42:20 EDT
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.
Comment 11 Igor Foox 2006-07-20 14:56:18 EDT
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.
Comment 12 Hans de Goede 2006-07-20 15:29:53 EDT
(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!

Comment 13 Hans de Goede 2006-08-09 15:21:46 EDT
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.
Comment 14 Igor Foox 2006-09-02 13:53:45 EDT
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?
Comment 15 Kevin Fenzi 2006-09-02 14:02:59 EDT
Check the permissions on that file. They must be 755 on a dynamic lib for rpm 
to properly strip/create debuginfo. 
Comment 16 Igor Foox 2006-09-02 16:00:42 EDT
$ 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. 
Comment 17 Igor Foox 2006-09-03 21:45:54 EDT
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. 
Comment 18 Paul Howarth 2006-09-06 06:21:28 EDT
(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.
Comment 19 Susi Lehtola 2009-10-05 18:15:47 EDT
Can you build this for EL-5? I need it to update jmol to the 11.8 series.
Comment 20 Jason Tibbitts 2009-10-07 20:40:47 EDT
*** Bug 227027 has been marked as a duplicate of this bug. ***
Comment 21 Orion Poplawski 2009-12-11 17:02:51 EST
Package Change Request
======================
Package Name: ant-contrib
New Branches: EL-5
Owners: orion

Alexander - do you want to own the EL branch too?
Comment 22 Kevin Fenzi 2009-12-21 15:12:01 EST
I don't see any answer after a week here, so going ahead with the request. 

cvs done.
Comment 23 Orion Poplawski 2009-12-22 11:49:17 EST
EL-5 update request has been made.  Thanks.

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