Bug 225254

I'll be happy to review this package, please look for full review shortly.

OK - Package meets naming and packaging guidelines
Ok - Spec file matches base package name.
OK - Spec has consistent macro usage.
ISSUE (4) - Meets Packaging Guidelines.
OK - License
OK - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
a2e2e54d65e9eae961f7171335cf2550  rpmbuild/SOURCES/apr-util-1.2.8.tar.gz
a2e2e54d65e9eae961f7171335cf2550  reviews/apr-util/apr-util-1.2.8.tar.gz

ISSUE (6) - BuildRequires correct
OK - Spec handles locales/find_lang
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
ISSUE (1) - Package has correct buildroot
OK - Package is code or permissible content.
OK - Doc subpackage needed/used.
OK - Packages %doc files don't affect runtime.

OK - Headers/static libs in -devel subpackage.
OK - Spec has needed ldconfig in post and postun
OK - .pc files in -devel subpackage/requires pkgconfig
OK - .so files in -devel subpackage.
OK - -devel package Requires: %{name} = %{version}-%{release}
ISSUE (7) - .la files are removed.

OK - Package compiles and builds on at least one arch.
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
OK - Package owns all the directories it creates.
ISSUE (5) - rpmlint output.
OK - Should build in mock.
OK - Should have sane scriptlets.
OK - Should have subpackages require base package with fully versioned depend.
ISSUE (2) - Should have dist tag
OK - Should package latest version
ISSUE (3) - check for outstanding bugs on package. (For core merge reviews)

Issues:

1. Build root is not one of the recommended options
    
    %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

2. It is recommended to use %{?dist} in Release:

3. There is an outstanding bug for apr-util, but it may not apply to
   Fedora.

4. Conflicts: should not be used.  Perhaps Requires: subversion >=
   0.20.1-2 instead? 

5 rpmlint output

W: apr-util invalid-license Apache Software License 2.0
W: apr-util-debuginfo invalid-license Apache Software License 2.0
W: apr-util-devel invalid-license Apache Software License 2.0
W: apr-util-mysql invalid-license Apache Software License 2.0
W: apr-util-pgsql invalid-license Apache Software License 2.0

    probably false positives

E: apr-util use-of-RPM_SOURCE_DIR

    in %prep use cp ${SOURCE1} dbd instead of
    cp $RPM_SOURCE_DIR/apr_dbd_mysql.c dbd

W: apr-util-devel no-documentation
W: apr-util-mysql no-documentation
W: apr-util-pgsql no-documentation

    Is there any documentation to include?


6. Build Requires
    doxygen is in BuildRequires but it does not seem to be used in the
    build process.  Is a step missing in the build process to create the
    documentation and then put %doc for each of the -devel -mysql and
    -pgsql sub packages ?

7. .la files exist and must be removed

(In reply to comment #2)

> 4. Conflicts: should not be used.  Perhaps Requires: subversion >=
>    0.20.1-2 instead? 

I think the Conflicts: should just be removed completely since apr-util does not
actually depend on subversion.

Thanks for the review!  Please see 1.2.8-7 in CVS/Raw Hide soon.

> 1. Build root is not one of the recommended options

The buildroot used does meet the mandatory requirements.

> 2. It is recommended to use %{?dist} in Release:

This is not mandatory.

> 3. There is an outstanding bug for apr-util, but it may not apply to
>    Fedora.

Not relevant to review process.

> 4. Conflicts: should not be used.  Perhaps Requires: subversion >=
>    0.20.1-2 instead? 

Removed, as suggested.

> 5 rpmlint output
> 
> W: apr-util invalid-license Apache Software License 2.0

No standard for License tags exists yet.

> E: apr-util use-of-RPM_SOURCE_DIR

This is not prohibited by packaging guidelines, see f-m@ flamewar.

> W: apr-util-devel no-documentation

All expected.

> 6. Build Requires
>     doxygen is in BuildRequires but it does not seem to be used in the

Removed.

> 7. .la files exist and must be removed

The .la file is part of the defined build interface for this package.

(In reply to comment #4)
Thanks for the changes.  Everything is good except for (7)

> > E: apr-util use-of-RPM_SOURCE_DIR
> 
> This is not prohibited by packaging guidelines, see f-m@ flamewar.

Understood, since the spec does not write to RPM_SOURCE_DIR then it is ok.

 
> > 7. .la files exist and must be removed
> 
> The .la file is part of the defined build interface for this package.

According to http://fedoraproject.org/wiki/Packaging/ReviewGuidelines 

- MUST: Packages must NOT contain any .la libtool archives, these should be
removed in the spec.

Accordingly I cannot give final approval until this is addressed.

Jeremy, you're wrong. There ARE cases, where you NEED a .la file (e.g. neon, 
ImageMagick just to name a few, to avoid breakings of e.g. rpm or convert).

And if Joe says .la is needed, we IMHO really should trust him (he maintains 
neon package and knows how to handle .la files, because rpm depends on neon and 
you can't simply drop the .la file there, too).

Created attachment 151985 [details]
move static-only libs to Libs.private section

similar to pkgconfig patch provided for apr

I'm unable to continue reviewing this package.  Someone else should probably
take it over.

CVS admins, please remove tag  apr-util-1_3_4-8_fc10. It was created by mistake.

We don't remove tags from cvs. Just bump the release and retag, or use 'make force-tag'.

Please explain why the patch from comment:8 cannot be used or apply it. Also the patches need an explanation wrt. their upstream status:
http://fedoraproject.org/wiki/Packaging/Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment

Please remove NotReady from the whiteboard, once this is done.

Fresh review:

Good:

- rpmlint checks return: 

apr-util-devel.x86_64: W: no-documentation
The package contains no documentation (README, doc, etc). You have to include
documentation files.

apr-util-devel.x86_64: W: no-manual-page-for-binary apu-1-config
Each executable in standard binary directories should have a man page.

apr-util-freetds.x86_64: W: no-documentation
The package contains no documentation (README, doc, etc). You have to include
documentation files.

apr-util-ldap.x86_64: W: no-documentation
The package contains no documentation (README, doc, etc). You have to include
documentation files.

apr-util-mysql.x86_64: W: no-documentation
The package contains no documentation (README, doc, etc). You have to include
documentation files.

apr-util-nss.x86_64: W: spelling-error Summary(en_US) crytpo -> crypt
The value of this tag appears to be misspelled. Please double-check.

apr-util-nss.x86_64: W: spelling-error %description -l en_US crypto -> crypt, crypts, crypt o
The value of this tag appears to be misspelled. Please double-check.

apr-util-nss.x86_64: W: no-documentation
The package contains no documentation (README, doc, etc). You have to include
documentation files.

apr-util-odbc.x86_64: W: no-documentation
The package contains no documentation (README, doc, etc). You have to include
documentation files.

apr-util-openssl.x86_64: W: spelling-error Summary(en_US) crytpo -> crypt
The value of this tag appears to be misspelled. Please double-check.

apr-util-openssl.x86_64: W: spelling-error %description -l en_US crypto -> crypt, crypts, crypt o
The value of this tag appears to be misspelled. Please double-check.

apr-util-openssl.x86_64: W: no-documentation
The package contains no documentation (README, doc, etc). You have to include
documentation files.

apr-util-pgsql.x86_64: W: no-documentation
The package contains no documentation (README, doc, etc). You have to include
documentation files.

apr-util-sqlite.x86_64: W: no-documentation
The package contains no documentation (README, doc, etc). You have to include
documentation files.

Mostly ignorable.

- package meets naming guidelines
- package meets packaging guidelines
- license ( ASL 2.0 ) OK, text in %doc, matches source
- spec file legible, in am. english
- source matches upstream
- package compiles on devel (x86)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file
- devel package ok
- no .la files

Patch from comment * should be applied.

- post/postun ldconfig ok
- devel requires base package n-v-r 

So it's pretty much just the static lib issue.  Let me know if you want me to commit anything.

Ping?  I'll apply the patch and close if that's ok with you?

No response, applied updated version of the patch and built in rawhide, APPROVED, closing.