Bug 246138

I've fixed things up a bit to be more consistent with our other packages and how
they generate source tarballs.  I'll attach a patch for the specfile and for the
tarball-generating script.  The patch to disable jregexp and jakarta-regexp will
need to be fixed to include jakarta-regexp support as we have a package of that
in Fedora:  regexp :).  There are also a few FIXMEs in the specfile (with the
patch applied).  Let me know when you've had a chance to look at those and we
can do the review then.  Thanks.

Created attachment 158554 [details]
specfile patch

Created attachment 158555 [details]
patch to tarball-generating script

I take a look on this tomorrow and normally I should be able to update the
review. There is just little work to do to update the patch because you have
done the big part of the job :)

Many thanks!

SPEC: http://download.tuxfamily.org/borsalino/7/SPECS/eclipse-QuickREx.spec
SRPM:
http://download.tuxfamily.org/borsalino/7/SRPMS/eclipse-QuickREx-3.5.0-2.fc7.src.rpm

It's done, here are the updated spec and srpms. Updating the patch was more
trivial that I have think.

PS: I have forgot to say that I'm seeking a sponsor.

Created attachment 158881 [details]
patch to fix remaining issues

This patch gets rid of an rpmlint warning about no documentation and fixes the
symlinks to the jakarta jars.  After it's applied, I think we should be good to
go with a review.

Before we do the review, though, do we really want the ugly capitalization in
the name?  Will it terribly upset upstream if we just call it eclipse-quickrex?
 We can add a virtual Provide for eclipse-QuickREx.  Let me know what you
think.

SPEC: http://download.tuxfamily.org/borsalino/7/SPECS/eclipse-quickrex.spec
SRPMS
http://download.tuxfamily.org/borsalino/7/SRPMS/eclipse-quickrex-3.5.0-2.fc7.src.rpm

I'm think that upstream guy should no worry about the name but to be sure I have
add a Provides tag. Above patch is applied and I have also renaming some other
things according with that.

Hi Alphonse,

I've finished the review.  Lines prefixed with a '?' are where I have a
question.  Those beginning with a '*' are fine and those marked with an
'X' indicate they must be fixed.  The 'MUST' and 'SHOULD' headers just
reflect the sections here:

http://fedoraproject.org/wiki/Packaging/ReviewGuidelines?action=show&redirect=PackageReviewGuidelines

MUST:
? package is named appropriately
 - can we get confirmation from upstream about the capitalization issue?
   I just don't want to go against their wishes.  Otherwise, it's fine.
* license field matches the actual license.
* license is open source-compatible
* specfile name matches %{name}
X verify source and patches (md5sum matches upstream, know what the patches do)
 - while I can't verify the md5sum of your tarball, I don't get any
   differences on a diff of the exploded tarball so I think we're fine.
   The instructions are also clear.
 - my only concern is the build.properties and feature.xml files -- did
   upstream author these or did you?  can they not be included upstream?
   I thought package-build worked fine with packages that didn't have
   features - does it not?  I guess I just want to know what the purpose
   of these files is and whether or not they will go upstream at some
   point :) .
* no typos in the summary or description
* buildroot fine, although this is now the most recommended value:
 %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)
* %{?dist} used properly
* license text included in package and marked with %doc
* packages meets FHS (http://www.pathname.com/fhs/)
X rpmlint on <this package>.srpm gives no output
 $ rpmlint ../SRPMS/eclipse-quickrex-3.5.0-2.fc7.src.rpm 
 eclipse-quickrex.src:145: W: strange-permission fetch-quickrex.sh 0764

 Can we make it 0755 or something?

X changelog fine except for extra space in first line:
  * Thu Jul 5 2007 Alphonse Van Assche  <alcapcom> 3.5.0-2
                                       ^
* Packager tag not used
* Vendor tag not used
* Distribution tag should not be used
* use License and not Copyright 
* Summary tag does not end in a period
* no PreReq
* specfile is legible
* package successfully compiles and builds on at least x86
* BuildRequires are proper
* summary should be a short and concise description of the package
* description expands upon summary
* make sure lines are <= 80 characters
 - lines that are > 80 are necessary IMO
* specfile written in American English
* no -doc sub-package necessary
* no static libraries
* no rpath
* no config files
* not a GUI app
* no -devel sub-package necessary
X macros used appropriately and consistently
 - %{buildroot} and $RPM_BUILD_ROOT -- pick one :)
* no %makeinstall
* install section begins with rm -rf $RPM_BUILD_ROOT or %{buildroot}
* no locale data
X consider using cp -p to preserve timestamps
* Requires(pre,post) split into two separate lines
* package not relocatable
* package contains code and documentation
* package owns all directories and files
* no %files duplicates
* file permissions okay; %defattrs present
* %clean present
* %doc files do not affect runtime
* not a web app
* final provides and requires of the binary RPMs fine

  $ rpm -qp --provides ../RPMS/i386/eclipse-quickrex-3.5.0-2.fc7.i386.rpm 
  QuickREx.jar.so  
  eclipse-QuickREx = 3.5.0-2.fc7
  eclipse-quickrex = 3.5.0-2.fc7

  $ rpm -qp --requires ../RPMS/i386/eclipse-quickrex-3.5.0-2.fc7.i386.rpm 
  /bin/sh  
  /bin/sh  
  eclipse-platform >= 3.2.1
  jakarta-oro  
  java-gcj-compat  
  java-gcj-compat  
  libc.so.6  
  libc.so.6(GLIBC_2.1.3)  
  libdl.so.2  
  libgcc_s.so.1  
  libgcc_s.so.1(GCC_3.0)  
  libgcj_bc.so.1  
  libm.so.6  
  libpthread.so.0  
  librt.so.1  
  libz.so.1  
  regexp  
  rpmlib(CompressedFileNames) <= 3.0.4-1
  rpmlib(PayloadFilesHavePrefix) <= 4.0-1
  rpmlib(VersionedDependencies) <= 3.0.3-1
  rtld(GNU_HASH)  

* rpmlint output when run on the binary RPMs
  $ rpmlint ../RPMS/i386/eclipse-quickrex-3.5.0-2.fc7.i386.rpm 
  eclipse-quickrex.i386: W: dangling-symlink
 
/usr/share/eclipse/plugins/de.babe.eclipse.plugins.QuickREx_3.5.0/lib/jakarta-regexp-1.4.jar
  /usr/share/java/regexp.jar
  eclipse-quickrex.i386: W: symlink-should-be-relative
 
/usr/share/eclipse/plugins/de.babe.eclipse.plugins.QuickREx_3.5.0/lib/jakarta-regexp-1.4.jar
  /usr/share/java/regexp.jar
  eclipse-quickrex.i386: W: dangling-symlink
 
/usr/share/eclipse/plugins/de.babe.eclipse.plugins.QuickREx_3.5.0/lib/jakarta-oro-2.0.8.jar
  /usr/share/java/jakarta-oro-2.0.8.jar
  eclipse-quickrex.i386: W: symlink-should-be-relative
 
/usr/share/eclipse/plugins/de.babe.eclipse.plugins.QuickREx_3.5.0/lib/jakarta-oro-2.0.8.jar
  /usr/share/java/jakarta-oro-2.0.8.jar

 - I think these are fine and I've never been told otherwise :).

SHOULD:
* package should include license text in the package and mark it with %doc
* package should build on i386
? package should build in mock
 - I didn't try but I don't anticipate any problems.  Alphonse, can you
   try this?
* package functions as expected (as far as I can tell)

SPEC: http://download.tuxfamily.org/borsalino/7/SPECS/eclipse-quickrex.spec
SRPMS
http://download.tuxfamily.org/borsalino/7/SRPMS/eclipse-quickrex-3.5.0-3.fc7.src.rpm

(In reply to comment #8)
> MUST:
> ? package is named appropriately
>  - can we get confirmation from upstream about the capitalization issue?
>    I just don't want to go against their wishes.  Otherwise, it's fine.
Have just send a mail to Bastian (upstream guy), normally that should be ok.

> X verify source and patches (md5sum matches upstream, know what the patches do)
>  - while I can't verify the md5sum of your tarball, I don't get any
>    differences on a diff of the exploded tarball so I think we're fine.
>    The instructions are also clear.
>  - my only concern is the build.properties and feature.xml files -- did
>    upstream author these or did you?  can they not be included upstream?
>    I thought package-build worked fine with packages that didn't have
>    features - does it not?  I guess I just want to know what the purpose
>    of these files is and whether or not they will go upstream at some
>    point :) .
I have send both to Bastian and he should include a feature for the next
release, so I have fake it for the moment.

> X rpmlint on <this package>.srpm gives no output
>  $ rpmlint ../SRPMS/eclipse-quickrex-3.5.0-2.fc7.src.rpm 
>  eclipse-quickrex.src:145: W: strange-permission fetch-quickrex.sh 0764
> 
>  Can we make it 0755 or something?
fixed
 
> X changelog fine except for extra space in first line:
>   * Thu Jul 5 2007 Alphonse Van Assche  <alcapcom> 3.5.0-2
fixed

> X macros used appropriately and consistently
>  - %{buildroot} and $RPM_BUILD_ROOT -- pick one :)
fixed

> X consider using cp -p to preserve timestamps
fixed
> * rpmlint output when run on the binary RPMs
>   $ rpmlint ../RPMS/i386/eclipse-quickrex-3.5.0-2.fc7.i386.rpm 
>   eclipse-quickrex.i386: W: dangling-symlink
>  
>
/usr/share/eclipse/plugins/de.babe.eclipse.plugins.QuickREx_3.5.0/lib/jakarta-regexp-1.4.jar
>   /usr/share/java/regexp.jar
>   eclipse-quickrex.i386: W: symlink-should-be-relative
>  
>
/usr/share/eclipse/plugins/de.babe.eclipse.plugins.QuickREx_3.5.0/lib/jakarta-regexp-1.4.jar
>   /usr/share/java/regexp.jar
>   eclipse-quickrex.i386: W: dangling-symlink
>  
>
/usr/share/eclipse/plugins/de.babe.eclipse.plugins.QuickREx_3.5.0/lib/jakarta-oro-2.0.8.jar
>   /usr/share/java/jakarta-oro-2.0.8.jar
>   eclipse-quickrex.i386: W: symlink-should-be-relative
>  
>
/usr/share/eclipse/plugins/de.babe.eclipse.plugins.QuickREx_3.5.0/lib/jakarta-oro-2.0.8.jar
>   /usr/share/java/jakarta-oro-2.0.8.jar
> 
>  - I think these are fine and I've never been told otherwise :).
:)
> ? package should build in mock
>  - I didn't try but I don't anticipate any problems.  Alphonse, can you
>    try this?
Naturally ;-), yeah the package build nicely with mock.

I have run rpmlint on the binary package build in mock and rpmlint complain only 
about the symlinks stuffs.

Thanks for the review

 

The permissions on the fetch script are still 0764.  However, rpmlint warns when
they are 0755 as well so if you change them to 755, we'll be good to go!

Have you heard back from Bastian yet on the naming issue?

Bastian have confirm me that he don't had problems wih the name, Oups I don't
had re-build the srpms with the fixed perms, ti's fixed.

SPEC: http://download.tuxfamily.org/borsalino/7/SPECS/eclipse-quickrex.spec
SRPMS
http://download.tuxfamily.org/borsalino/7/SRPMS/eclipse-quickrex-3.5.0-3.fc7.src.rpm

Cool.  I've done a quick grep through the code for "Copyright", "License",
"Proprietary", "Confidential" and "Sun" and things seem okay, so let's go ahead
with this.

Approved!  Thanks, Alphonse.

I don't know where to go from here with sponsorship.  I'm willing to sponsor
you, Alphonse, but what's the next step?

Cool :)
Thanks for the review and sponsorship Andrew, Have a good evening.

This is confusing; what happened to this bug?  overholt approved it, but nobody
ever made a CVS request and now it's been closed.  But tickets that are closed
should never block FE-NEEDSPONSOR.

So, did Alphonse ever get sponsored?  Why was this package never imported?

Sorry Jason, it is a mistake of me about the procedure. 

New Package CVS Request
=======================
Package Name: eclipse-quickrex
Short Description: eclipse-quickrex is a regular-expression test Eclipse Plug-In
Owners: alcapcom
Branches: FC-6 F-7
InitialCC: alcapcom

I'd like to be on the InitialCC or at least on CC after the package is imported.

cvs done. Added initialCC per comment #17. 
Also, changed the review Summary for easy tracking and removed needsponsor.