Fedora Merge Review: regexp http://cvs.fedora.redhat.com/viewcvs/devel/regexp/ Initial Owner: vivekl
I'll take this one.
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
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?
(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.
(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.
(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.
(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.
(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!
I see it in rawhide. Thanks!