Bug 226366 - Merge Review: regexp
Merge Review: regexp
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-31 15:49 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-03-12 13:33:54 EDT
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-31 15:49:36 EST
Fedora Merge Review: regexp

http://cvs.fedora.redhat.com/viewcvs/devel/regexp/
Initial Owner: vivekl@redhat.com
Comment 1 Andrew Overholt 2007-02-08 18:05:19 EST
I'll take this one.
Comment 2 Andrew Overholt 2007-02-08 20:37:49 EST
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-08 22:58:09 EST
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 12:18:17 EST
(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 14:03:42 EST
(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 14:28:32 EST
(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 16:39:30 EST
(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 18:56:41 EDT
(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 13:33:54 EDT
I see it in rawhide.  Thanks!

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