Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 226366 - Merge Review: regexp
Summary: Merge Review: regexp
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Andrew Overholt
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-01-31 20:49 UTC by Nobody's working on this, feel free to take it
Modified: 2007-11-30 22:11 UTC (History)
1 user (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-03-12 17:33:54 UTC
overholt: fedora-review+


Attachments (Terms of Use)

Description Nobody's working on this, feel free to take it 2007-01-31 20:49:36 UTC
Fedora Merge Review: regexp

http://cvs.fedora.redhat.com/viewcvs/devel/regexp/
Initial Owner: vivekl@redhat.com

Comment 1 Andrew Overholt 2007-02-08 23:05:19 UTC
I'll take this one.

Comment 2 Andrew Overholt 2007-02-09 01:37:49 UTC
MUST:
X rpmlint on regexp srpm gives no output

W: regexp non-standard-group Development/Libraries/Java

Perhaps:  System Environment/Libraries ?

* 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?

* license field matches the actual license.
* license is open source-compatible.
* license text included in package and marked with %doc
* specfile written in American English
X specfile is legible
. do we still need the crazy gcj_support line?

X source files match upstream
. I can't find the tarball.  Also, Source0 can be the actual URL ending with the
tar.gz.

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

X BuildRequires are proper
. why is jpackage-utils in Requires(pre,post)?

* no locale data so no find_lang necessary
* package is not relocatable
X package owns all directories and files
. why is the javadoc symlink not just made in %install and then added to the
  %file section?
* no %files duplicates
* file permissions are fine; %defattrs present
* %clean present
* macro usage is consistent
* package contains code
* no large docs so no -doc subpackage
. javadoc package present
* %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
X final provides and requires are sane

$ rpm -qp --provides i386/regexp-1.4-3jpp.1.fc7.i386.rpm 
regexp-1.4.jar.so  
regexp = 0:1.4-3jpp.1.fc7

$ rpm -qp --provides i386/regexp-javadoc-1.4-3jpp.1.fc7.i386.rpm 
regexp-javadoc = 0:1.4-3jpp.1.fc7

Do we need a 'java' dependency somewhere?  Does the (erroneous, I think)
Requires(pre,post) on jpackage-utils imply a regular Requires on it?  Do we
need things in coreutils (rpm, ln) in Requires(post,postun)?

$ rpm -qp --requires i386/regexp-1.4-3jpp.1.fc7.i386.rpm 
/bin/sh  
/bin/sh  
java-gcj-compat  
java-gcj-compat  
jpackage-utils >= 0:1.6
jpackage-utils >= 0:1.6
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  
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rtld(GNU_HASH)  

$ rpm -qp --requires i386/regexp-javadoc-1.4-3jpp.1.fc7.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 3 Kevin Fenzi 2007-02-09 03:58:09 UTC
Hey Andrew: 

The current way we are doing these reviews is described at:
http://fedoraproject.org/wiki/WarrenTogami/ReviewWithFlags

So, you should set the 'fedora-review' flag to - and reassign back to the 
owner/submitter to fix items you see in your review. Then when they do so, 
they should add a comment, change 'fedora-review' to ? and reassign back to you
to look over. Once you approve the package reassign the review back to the
submitter and set the 'fedora-review' flag to + 
Blocker bugs aren't being used for these reviews. 

Can you set the assigned and flags as you see fit?

Comment 4 Andrew Overholt 2007-02-09 17:18:17 UTC
(In reply to comment #3)
> Can you set the assigned and flags as you see fit?

Definitely.  I totally forgot about the new flag-based reviews.  Sorry.

Comment 5 Vivek Lakshmanan 2007-02-09 19:03:42 UTC
(In reply to comment #2)
> MUST:
> X rpmlint on regexp srpm gives no output
> W: regexp non-standard-group Development/Libraries/Java
> Perhaps:  System Environment/Libraries ?
It seems use of the existing group is acceptable:
https://www.redhat.com/archives/fedora-packaging/2007-February/msg00070.html
> X package meets packaging guidelines.
> . BuildRoot incorrect.  As per this:
> %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
Amended.
> . do we need section free?
Its a redundant JPackage artifact, removed.
> X specfile is legible
> . do we still need the crazy gcj_support line?
AFAICR the incantation was added so native compilation (i.e. arch dependence)
could be specified on a build machine directly without the need to modify spec
files. However, brew prevents the use of machine specific settings, hence the
use of the %define at the top. However, if the packages are built on mock, such
settings can be provided on the build machine and the hardcoded %define can be
removed. 
> X source files match upstream
> . I can't find the tarball.  Also, Source0 can be the actual URL ending with the
> tar.gz.
Really? With
Source0:http://www.apache.org/dist/jakarta/regexp/jakarta-regexp-%{version}.tar.gz
wget http://www.apache.org/dist/jakarta/regexp/jakarta-regexp-1.4.tar.gz  brings
in the tar ball fine for me. Note the replacement of %{version} in the URL.
Surely the use of the macro is not a problem?

> X BuildRequires are proper
> . why is jpackage-utils in Requires(pre,post)?
According to the guidelines, all directories created by the package must be
owned by the package or the package must require a package that provides the
directory. Directories like %{_javadir} and %{_javadocdir} (/usr/share/java,
/usr/share/javadoc) are provided by jpackage-utils and since the package tries
to install/uninstall things to these directories, I think the presence of these
directories ought to be mandated for the package to be installed/uninstalled.

> X package owns all directories and files
> . why is the javadoc symlink not just made in %install and then added to the
>   %file section?
Fixed. The %pre and %post scriptlets for the javadoc are there for multiple
versions of the javadoc package to coexist and the unversioned symlink allows
crosslinking of javadocs.

> X final provides and requires are sane
> Do we need a 'java' dependency somewhere?  Does the (erroneous, I think)
> Requires(pre,post) on jpackage-utils imply a regular Requires on it?  Do we
> need things in coreutils (rpm, ln) in Requires(post,postun)?
Added the Requires on java
The Requires(x) on jpackage-utils has been commented on above. As far as the
question of /bin/rm and /bin/ln in the requires(x) is concerned, this is to
ensure that rpm transactions ensure these are present before the
installation/uninstallation of the package since the %pre and %postun scripts
use them.


Comment 6 Andrew Overholt 2007-02-09 19:28:32 UTC
(In reply to comment #5)
> (In reply to comment #2)
> > MUST:
> > X rpmlint on regexp srpm gives no output
> > W: regexp non-standard-group Development/Libraries/Java
> > Perhaps:  System Environment/Libraries ?
> It seems use of the existing group is acceptable:
> https://www.redhat.com/archives/fedora-packaging/2007-February/msg00070.html

Okay.

> > X source files match upstream
> > . I can't find the tarball.  Also, Source0 can be the actual URL ending with the
> > tar.gz.
> Really?

Sorry, I accidentally copied that from another review :)

> > X BuildRequires are proper
> > . why is jpackage-utils in Requires(pre,post)?
> According to the guidelines, all directories created by the package must be
> owned by the package

Yes, I agree with your reasoning but let's just remove the javadoc symlinking in
%post{,un} and then these requirements can go away.

> > X package owns all directories and files
> > . why is the javadoc symlink not just made in %install and then added to the
> >   %file section?
> Fixed. The %pre and %post scriptlets for the javadoc are there for multiple
> versions of the javadoc package to coexist and the unversioned symlink allows
> crosslinking of javadocs.

I don't think this is worth the complexity of the the %posts.  Do you agree?

> > X final provides and requires are sane
> > Do we need a 'java' dependency somewhere?  Does the (erroneous, I think)
> > Requires(pre,post) on jpackage-utils imply a regular Requires on it?  Do we
> > need things in coreutils (rpm, ln) in Requires(post,postun)?
> Added the Requires on java

Great, thanks.

> As far as the
> question of /bin/rm and /bin/ln in the requires(x) is concerned, this is to
> ensure that rpm transactions ensure these are present before the
> installation/uninstallation of the package since the %pre and %postun scripts
> use them.

Yeah, I'm just not sure if things in coreutils need to be worried about for
Requires(post,postun).  I'll ask on fedora-packaging and we can go from there.

Thanks, Vivek.


Comment 7 Andrew Overholt 2007-02-09 21:39:30 UTC
(In reply to comment #6)
> > > X source files match upstream
> > > . I can't find the tarball.  Also, Source0 can be the actual URL ending
with the
> > > tar.gz.
> > Really?
> 
> Sorry, I accidentally copied that from another review :)

The md5sums match.

> > > X BuildRequires are proper
> > > . why is jpackage-utils in Requires(pre,post)?
> > According to the guidelines, all directories created by the package must be
> > owned by the package
> 
> Yes, I agree with your reasoning but let's just remove the javadoc symlinking in
> %post{,un} and then these requirements can go away.

Okay, this isn't holding up the review, but I still don't like it :).
> > > X final provides and requires are sane
> > > Do we need a 'java' dependency somewhere?  Does the (erroneous, I think)
> > > Requires(pre,post) on jpackage-utils imply a regular Requires on it?  Do we
> > > need things in coreutils (rpm, ln) in Requires(post,postun)?
> > Added the Requires on java

I asked about the Requires(x) on coreutils things and the answer was to err on
the safe side so those are fine.  I don't like the JPackage-style %{__rm} but
again, that's not going to hold up the review.

APPROVED.  Thanks, Vivek!

As per https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225928#c7 , please
rebuild this package in Brew and when I've confirmed that the updated package
has hit Rawhide, I'll close this bug as RAWHIDE.

Comment 8 Vivek Lakshmanan 2007-03-11 22:56:41 UTC
(In reply to comment #7)
> APPROVED.  Thanks, Vivek!
> 
> As per https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=225928#c7 , please
> rebuild this package in Brew and when I've confirmed that the updated package
> has hit Rawhide, I'll close this bug as RAWHIDE.
Hey Andrew,
http://mirror.linux.duke.edu/pub/fedora/linux/core/development/i386/os/Fedora/regexp-1.4-3jpp.1.fc7.i386.rpm
suggests it is available in the mirrors now. Can you close the bug (just trying
to gt rid of BZ clutter...)
Thanks!

Comment 9 Andrew Overholt 2007-03-12 17:33:54 UTC
I see it in rawhide.  Thanks!


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