Bug 246138 - Review Request: eclipse-quickrex - QuickREx is a regular-expression test Eclipse Plug-In
Review Request: eclipse-quickrex - QuickREx is a regular-expression test Ecli...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Andrew Overholt
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-06-28 15:10 EDT by Alphonse Van Assche
Modified: 2007-11-30 17:12 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-08-02 09:28:06 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
overholt: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
specfile patch (3.94 KB, patch)
2007-07-04 17:15 EDT, Andrew Overholt
no flags Details | Diff
patch to tarball-generating script (902 bytes, text/x-patch)
2007-07-04 17:15 EDT, Andrew Overholt
no flags Details
patch to fix remaining issues (1.08 KB, patch)
2007-07-10 14:47 EDT, Andrew Overholt
no flags Details | Diff

  None (edit)
Description Alphonse Van Assche 2007-06-28 15:10:58 EDT
Spec URL: http://download.tuxfamily.org/borsalino/7/SPECS/eclipse-QuickREx.spec
SRPM URL: http://download.tuxfamily.org/borsalino/7/SRPMS/eclipse-QuickREx-3.5.0-1.27062007cvs.fc7.src.rpm
Description:
QuickREx gives you a view in which you can enter test-texts and try
regular expressions.The expressions are evaluated against the test-text on the
fly, matches are highlighted and you can navigate between the matches.
You can also navigate through groups within each match (if groups are defined
in your regular expression). Regular Expressions are edited either within the
view or within a composition-dialog which offers the most useful
building-blocks grouped in categories and accompanied by short explanations.

This version of QuickREx use only the JDK and Jakarta ORO regular
expression API. We have drop jregex and jakarta-regxep support because of
the fact that there isn't Fedora packages for both of them and they 
given't nothing more than the two supported other API.
Comment 1 Andrew Overholt 2007-07-04 17:14:24 EDT
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.
Comment 2 Andrew Overholt 2007-07-04 17:15:02 EDT
Created attachment 158554 [details]
specfile patch
Comment 3 Andrew Overholt 2007-07-04 17:15:43 EDT
Created attachment 158555 [details]
patch to tarball-generating script
Comment 4 Alphonse Van Assche 2007-07-04 17:29:15 EDT
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!

Comment 5 Alphonse Van Assche 2007-07-05 05:47:23 EDT
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.

Comment 6 Andrew Overholt 2007-07-10 14:47:28 EDT
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.
Comment 7 Alphonse Van Assche 2007-07-11 07:57:57 EDT
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.
Comment 8 Andrew Overholt 2007-07-11 14:42:19 EDT
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@gmail.com> 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)
Comment 9 Alphonse Van Assche 2007-07-11 17:52:13 EDT
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@gmail.com> 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

 
Comment 10 Andrew Overholt 2007-07-13 11:32:52 EDT
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?
Comment 11 Alphonse Van Assche 2007-07-14 07:19:38 EDT
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

Comment 12 Andrew Overholt 2007-07-16 14:34:23 EDT
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?
Comment 13 Alphonse Van Assche 2007-07-16 16:37:11 EDT
Cool :)
Thanks for the review and sponsorship Andrew, Have a good evening.
Comment 14 Jason Tibbitts 2007-08-02 10:51:04 EDT
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?
Comment 15 Alphonse Van Assche 2007-08-02 11:03:00 EDT
Sorry Jason, it is a mistake of me about the procedure. 
Comment 16 Alphonse Van Assche 2007-08-02 14:28:40 EDT
New Package CVS Request
=======================
Package Name: eclipse-quickrex
Short Description: eclipse-quickrex is a regular-expression test Eclipse Plug-In
Owners: alcapcom@gmail.com
Branches: FC-6 F-7
InitialCC: alcapcom@gmail.com
Comment 17 Andrew Overholt 2007-08-02 14:51:03 EDT
I'd like to be on the InitialCC or at least on CC after the package is imported.
Comment 18 Kevin Fenzi 2007-08-02 15:48:24 EDT
cvs done. Added initialCC per comment #17. 
Also, changed the review Summary for easy tracking and removed needsponsor. 

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