Bug 225254 - Merge Review: apr-util
Merge Review: apr-util
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jon Ciesla
Fedora Package Reviews List
NotReady
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-29 16:01 EST by Nobody's working on this, feel free to take it
Modified: 2013-02-07 10:12 EST (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-02-07 10:12:50 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
limburgher: fedora‑review+
kevin: fedora‑cvs-


Attachments (Terms of Use)
move static-only libs to Libs.private section (462 bytes, patch)
2007-04-09 08:08 EDT, Rex Dieter
no flags Details | Diff

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-29 16:01:54 EST
Fedora Merge Review: apr-util

http://cvs.fedora.redhat.com/viewcvs/devel/apr-util/
Comment 1 Jeremy Hinegardner 2007-03-29 22:51:15 EDT
I'll be happy to review this package, please look for full review shortly.
Comment 2 Jeremy Hinegardner 2007-03-30 01:00:51 EDT
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
Comment 3 Jeremy Hinegardner 2007-03-30 01:14:50 EDT
(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.
Comment 4 Joe Orton 2007-04-05 12:11:16 EDT
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.
Comment 5 Jeremy Hinegardner 2007-04-08 01:55:38 EDT
(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.

Comment 6 Robert Scheck 2007-04-08 05:57:27 EDT
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).
Comment 7 Robert Scheck 2007-04-08 05:59:57 EDT
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).
Comment 8 Rex Dieter 2007-04-09 08:08:31 EDT
Created attachment 151985 [details]
move static-only libs to Libs.private section

similar to pkgconfig patch provided for apr
Comment 9 Jeremy Hinegardner 2008-01-20 12:31:28 EST
I'm unable to continue reviewing this package.  Someone else should probably
take it over.
Comment 10 Bojan Smojver 2008-08-22 22:19:52 EDT
CVS admins, please remove tag  apr-util-1_3_4-8_fc10. It was created by mistake.
Comment 11 Kevin Fenzi 2008-08-23 00:26:24 EDT
We don't remove tags from cvs. Just bump the release and retag, or use 'make force-tag'.
Comment 12 Till Maas 2009-09-16 19:36:29 EDT
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.
Comment 13 Jon Ciesla 2012-04-03 13:13:21 EDT
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.
Comment 14 Jon Ciesla 2012-04-26 08:50:20 EDT
Ping?  I'll apply the patch and close if that's ok with you?
Comment 15 Jon Ciesla 2013-02-07 10:12:50 EST
No response, applied updated version of the patch and built in rawhide, APPROVED, closing.

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