Bug 225238 - Merge Review: adaptx
Merge Review: adaptx
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Andrew Overholt
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-29 15:59 EST by Nobody's working on this, feel free to take it
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-02-26 11:58:11 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
overholt: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-29 15:59:18 EST
Fedora Merge Review: adaptx

http://cvs.fedora.redhat.com/viewcvs/devel/adaptx/
Comment 1 Deepak Bhole 2007-02-07 17:54:53 EST
Proposed spec file and SRPM are here:

http://people.redhat.com/dbhole/fedora/
Comment 2 Andrew Overholt 2007-02-08 18:06:15 EST
I'll take this one.
Comment 3 Andrew Overholt 2007-02-09 16:44:04 EST
MUST:
X rpmlint on adaptx srpm gives no output

W: adaptx invalid-license Exolab Software License

I've emailed fedora-maintainers about this.  It looks like it'll be okay but I
want to confirm first.

* package is named appropriately
* specfile name matches %{name}
X package meets packaging guidelines.

. BuildRoot incorrect.  As per this:

http://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot

it should be:

%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

. do we need section free?  I think it can safely be removed.
. can the Summary be expanded a bit?
. the commented-out build-classpath line can probably be removed.  was the
dependency on js removed?
. are the two patches still necessary?
. do we need the explicit Epoch?  I can't find anything in the guidelines about
this but I think it's a bit verbose.  After speaking with others, we've come
to the conclusion that it doesn't violate the guidelines ... but I still don't
like it personally :)

X license field matches the actual license.

. see above

X license is open source-compatible.

. again, see above.

* license text included in package and marked with %doc
* specfile written in American English
* specfile is legible
X source files match upstream
. I assume upstream doesn't provide source drops?  The svn export is fine in
this case (please use an SVN tag), but can you put exact instructions for how to
generate the tarball?  Preferably something that can be duplicated by just
removing leading #'s.  I'd also like to see the comment be before the Source
entry but that's just personal preference :)  Also, I don't think the RHCLEAN is
necessary because I think we can re-distribute what binary jars they include
upstream.  As long as we symlink to our built ones, it should be fine.  

* package successfully compiles and builds on at least x86 (it's building on
the other arches in Fedora Core presently)

* BuildRequires are proper
* no locale data so no find_lang necessary
* package is not relocatable
* package owns all directories and files
. I've said in other reviews how I don't like how the javadoc symlinking is
  being done, but this won't hold up the review.
* no %files duplicates
* file permissions are fine; %defattrs present
* %clean present
* macro usage is consistent
* package contains code
* -doc subpackage fine
. javadoc package present also
* %doc files don't affect runtime
* shared libraries are present, but no ldconfig required.
* no pkgconfig or header files
* no -devel package
* no .la files
* no desktop file
* not a web app.
* file ownership fine
? final provides and requires are sane

Do we need a java dependency somewhere?  How about a Requires on the things we
BR like log4j?

$ rpm -qp --provides i386/adaptx-0.9.13-4jpp.1.i386.rpm
adaptx-0.9.13.jar.so  
adaptx = 0:0.9.13-4jpp.1

$ rpm -qp --provides i386/adaptx-doc-0.9.13-4jpp.1.i386.rpm 
adaptx-doc = 0:0.9.13-4jpp.1

$ rpm -qp --provides i386/adaptx-javadoc-0.9.13-4jpp.1.i386.rpm 
adaptx-javadoc = 0:0.9.13-4jpp.1

$ rpm -qp --requires i386/adaptx-0.9.13-4jpp.1.i386.rpm
/bin/sh  
/bin/sh  
java-gcj-compat  
java-gcj-compat  
jpackage-utils  
jpackage-utils  
libc.so.6  
libc.so.6(GLIBC_2.1.3)  
libdl.so.2  
libgcc_s.so.1  
libgcc_s.so.1(GCC_3.0)  
libgcc_s.so.1(GLIBC_2.0)  
libgcj_bc.so.1  
libm.so.6  
libm.so.6(GLIBC_2.0)  
libpthread.so.0  
librt.so.1  
libz.so.1  
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rtld(GNU_HASH)  

$ rpm -qp --requires i386/adaptx-doc-0.9.13-4jpp.1.i386.rpm 
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1

$ rpm -qp --requires i386/adaptx-javadoc-0.9.13-4jpp.1.i386.rpm 
/bin/ln  
/bin/rm  
/bin/rm  
/bin/sh  
/bin/sh  
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1

SHOULD:
* package includes license text
* package builds on i386
  ... and others in brew ATM; I don't envision a problem here
X package functions
  . I don't know how to test this package
X package builds in mock
  my mock setup doesn't seem to be working but I don't anticipate any problems
  here as the package currently builds fine in brew
Comment 4 Andrew Overholt 2007-02-10 08:38:20 EST
(In reply to comment #3)
> MUST:
> X rpmlint on adaptx srpm gives no output
> 
> W: adaptx invalid-license Exolab Software License
> 
> I've emailed fedora-maintainers about this.  It looks like it'll be okay but I
> want to confirm first.

According to this:

https://www.redhat.com/archives/fedora-maintainers/2007-February/msg00367.html

we should just mark it as BSD.

So we have remaining:

> X package meets packaging guidelines.
> 
> . BuildRoot incorrect.  As per this:
> 
> http://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot
> 
> it should be:
> 
> %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
> 
> . do we need section free?  I think it can safely be removed.
> . can the Summary be expanded a bit?
> . the commented-out build-classpath line can probably be removed.  was the
> dependency on js removed?
> . are the two patches still necessary?

> X source files match upstream
> . I assume upstream doesn't provide source drops?  The svn export is fine in
> this case (please use an SVN tag), but can you put exact instructions for how to
> generate the tarball?  Preferably something that can be duplicated by just
> removing leading #'s.  I'd also like to see the comment be before the Source
> entry but that's just personal preference :)  Also, I don't think the RHCLEAN is
> necessary because I think we can re-distribute what binary jars they include
> upstream.  As long as we symlink to our built ones, it should be fine.  

> Do we need a java dependency somewhere?  How about a Requires on the things we
> BR like log4j?
Comment 5 Deepak Bhole 2007-02-10 17:29:30 EST
Thanks for the comments. I have updated the spec file and the srpm on the given
URL above.

I fixed the buildroot name, changed license to BSD, removed the commented
build-classpath line, expanded the summary a bit, updated source location to the
svn tag, and made the BR and Requires same (except for ant, which is strictly a BR).

1. I did not change the RHCLEAN. This is because I am unsure if it is really
okay to put jars in tarballs. That used to be the case in Fedora packages
before, and gbenson had updated all tarballs to remove the jars. Has the policy
changed again?

2. I did not remove the epoch. I don't recall why that exists there in the first
place, but I remember fnasser mentioning that it was necessary for packages to
work correctly. I will send him a mail asking about this.
Comment 6 Andrew Overholt 2007-02-11 18:45:41 EST
(In reply to comment #5)
> 1. I did not change the RHCLEAN. This is because I am unsure if it is really
> okay to put jars in tarballs. That used to be the case in Fedora packages
> before, and gbenson had updated all tarballs to remove the jars. Has the policy
> changed again?

I don't think that was ever an official policy.  We really shouldn't be mucking
with the drops unless there's some legal reason (encryption or closed source) to
do so.  In this case, if you don't want to ship the binary jars, just add the
removing of them to the steps used to make the tarball.  Drop the RHCLEAN.

> 2. I did not remove the epoch. I don't recall why that exists there in the first
> place, but I remember fnasser mentioning that it was necessary for packages to
> work correctly. I will send him a mail asking about this.

This is a JPackage thing to deal with older, broken RPMs.  It's not strictly
against the guidelines but it's unnecessary.  I won't hold up the review on it,
though.
Comment 7 Andrew Overholt 2007-02-11 18:51:54 EST
(In reply to comment #5)
> I fixed the buildroot name, changed license to BSD, removed the commented
> build-classpath line, expanded the summary a bit, updated source location to the
> svn tag, and made the BR and Requires same (except for ant, which is strictly
a BR).

Great, thanks.  The only thing I'd like to see changed is the instructions on
how to generate the tarball.  Something like:

# mkdir adaptx-0.9.13-src && cd adaptx-0.9.13-src
# svn export http://svn.codehaus.org/castor/adaptx/tags/0.9.13/
# mv 0.9.13/* .
# rmdir 0.9.13
# cd ..
# tar cjf adaptx-0.9.13-src.tar.bz2 adaptx-0.9.13-src

I also don't think the removal of CVS directories is necessary, is it?
 
Comment 8 Deepak Bhole 2007-02-11 23:09:15 EST
(In reply to comment #7)
> (In reply to comment #5)
> > I fixed the buildroot name, changed license to BSD, removed the commented
> > build-classpath line, expanded the summary a bit, updated source location to the
> > svn tag, and made the BR and Requires same (except for ant, which is strictly
> a BR).
> 
> Great, thanks.  The only thing I'd like to see changed is the instructions on
> how to generate the tarball.  Something like:
> 
> # mkdir adaptx-0.9.13-src && cd adaptx-0.9.13-src
> # svn export http://svn.codehaus.org/castor/adaptx/tags/0.9.13/
> # mv 0.9.13/* .
> # rmdir 0.9.13
> # cd ..
> # tar cjf adaptx-0.9.13-src.tar.bz2 adaptx-0.9.13-src
> 
> I also don't think the removal of CVS directories is necessary, is it?
>  

Done. I have removed the epoch, and added instructions on how to generate the
tarballs.

Comment 9 Andrew Overholt 2007-02-11 23:36:03 EST
APPROVED

Thanks, Deepak.  Now you just need to rebuild it in brew and when it hits
rawhide I'll close this RAWHIDE.
Comment 10 Deepak Bhole 2007-02-12 00:00:07 EST
Thanks for all the suggestions. AdaptX has been built in brew and this bug may
now be closed any time.

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