[Bug 589504] Review Request: apache-commons-compress - Java API for working with tar, zip and bzip2 files

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=589504

Alexander Kurtakov <akurtako at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|fedora-review?              |fedora-review+

--- Comment #13 from Alexander Kurtakov <akurtako at redhat.com> 2010-05-10 07:26:35 EDT ---
(In reply to comment #12)
> Spec URL: http://red.fedorapeople.org/SRPMS/apache-commons-compress.spec
> SRPM URL:
> http://red.fedorapeople.org/SRPMS/apache-commons-compress-1.0-4.fc12.src.rpm
> 
> Thanks for the review Alexander!
> 
> (In reply to comment #11)
> > Other problems:
> > * You should not install in %build section. Please just remove the "install -d
> > $MAVEN_REPO_LOCAL" line.
> 
> This line looks important to me. Also, I see no reason not to install something
> into the unpacked sources if necessary for the build. After all, mvn-jpp will
> write to that dir as well. Also, the maven template from the guidelines does
> the same (with mkdir instead of install):
> http://fedoraproject.org/wiki/Packaging:Java#maven_2
> 
> If you have a reason to believe mkdir would fit better I'm willing to replace
> the install by that, tho.

Well, using install in %build section can make someone believe that you are
putting smth in the buildroot, which you are certainly not doing. Using mkdir
is preferred to not confuse people.

> 
> > * All your symlinks are broken "ln -s %{name}-%{version}.jar" should be "ln -s
> > %{shortname}-%{version}.jar
> 
> Awww, very good catch! Should have paid more attention as I changed that
> symlinking part a bit recently :/ FIXED.
> 
> > * You should not use %define but %global. See
> > http://fedoraproject.org/wiki/Packaging:Guidelines#.25global_preferred_over_.25define
> 
> Right, forgot to check that for the new renamed version - and the jakarta-*
> thingy is older than this rule :) FIXED.
> 
> > * You can make use of the %{name} macro on a few places (if you want to) -
> > Javadoc subpackage Summary and Description.    
> 
> True, FIXED.    

Thanks,
This package is APPROVED.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.