[Bug 226377] Merge Review: rpm

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Merge Review: rpm


https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226377


kevin@xxxxxxxxx changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
         AssignedTo|nobody@xxxxxxxxxxxxxxxxx    |pnasrat@xxxxxxxxxx
                 CC|                            |kevin@xxxxxxxxx
               Flag|                            |fedora-review-




------- Additional Comments From kevin@xxxxxxxxx  2007-02-22 02:20 EST -------
OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
OK - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
OK - License (GPL)
OK - License field in spec matches
See below - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
e24ce468082479fe850c9d6563f56db5  rpm-4.4.2.tar.gz
e24ce468082479fe850c9d6563f56db5  rpm-4.4.2.tar.gz.1
See below - BuildRequires correct
See below - Spec handles locales/find_lang
OK - Package is relocatable and has a reason to be.
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
See below - Package has correct buildroot
OK - Package is code or permissible content.
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 - .so files in -devel subpackage.
OK - -devel package Requires: %{name} = %{version}-%{release}
OK - .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.
See below - No rpmlint output.
See below - final provides and requires are sane:

SHOULD Items:

OK - Should build in mock.
OK - Should build on all supported archs
OK - Should function as described.
OK - Should have sane scriptlets.
OK - Should have subpackages require base package with fully versioned depend.
OK - Should have dist tag
OK - Should package latest version
147 outstanding open bugs - check for outstanding bugs on package.

1. There are a number of different liceses here:
main part - GPL and some LGPL
db - BSDish
zlib - weird zlib license.
I assume they can all handle being released GPL as a whole?
Can you include a copy of the GPL COPYING file?

2. Is there any way that find_lang could be used?
I guess it would need to be called from the local ./scripts/find-lang.sh  ?

3. Please use the one true buildroot:
      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

4. Are all the "%ifos linux" conditionals needed? Is this spec used
on non linux systems very often?

5. Per http://www.fedoraproject.org/wiki/PackagingDrafts/Conflicts
(which is only a draft it's true), could you change the
Conflicts: patch < 2.5
to
Requires: patch > 2.5

or since this has been true since fc1, can we just remove that?
Also in the -build subpackage there's a 'Requires: patch >= 2.5'

6. BuildRequires: sed isn't needed, thats in the base build exceptions.

7. What does the "#XXX: lua fix this" comment mean?

8. Can this conditional be removed now:
# XXX Red Hat 5.2 has not bzip2 or python
%if %{with_bzip2}

9. In both build and install there is:
# XXX rpm needs functioning nptl for configure tests
unset LD_ASSUME_KERNEL || :

Perhaps instead it could bomb out of the build if someone is stupid
enough to have that set these days? Right now if they do have it set
and they build rpm, it will magically be unset after the build.

10. Since there's a cron.daily file, shouldn't there also be
Requires: crontab

11. Likewise for logrotate, should there be a 'Requires: logrotate'

12. This block in install doesn't seem to be doing anything:

%if %{with_apidocs}
gzip -9n apidocs/man/man*/* || :
%endif

from build.log:

+ gzip -9n 'apidocs/man/man*/*'
gzip: apidocs/man/man*/*: No such file or directory

The directory in the devel subpackage is empty.

13. The python subpackage should not need to call ldconfig on post/postun.
This block can be removed:
%if %{with_python_subpackage}
%post python -p /sbin/ldconfig
%postun python -p /sbin/ldconfig
%endif

14. rpmlint has a few things to say:

First, there are 240 lines of 'non-standard-uid' or 'non-standard-gid'
These can all be ignored in this case.

a) 
W: popt no-url-tag
W: rpm no-url-tag
W: rpm-build no-url-tag
W: rpm-libs no-url-tag
W: rpm-python no-url-tag
W: rpm-devel no-url-tag

Suggest: add a "URL: http://www.rpm.org"; ? 

b)
W: popt summary-ended-with-dot A C library for parsing command line parameters.
W: rpm summary-ended-with-dot The RPM package management system.
W: rpm summary-ended-with-dot The RPM package management system.
W: rpm-build summary-ended-with-dot Scripts and executable programs used to
build packages. 
W: rpm-devel summary-ended-with-dot Development files for manipulating RPM packages.
W: rpm-libs summary-ended-with-dot Libraries for manipulating RPM packages.
W: rpm-python summary-ended-with-dot Python bindings for apps which will
manipulate RPM packages.

Suggest: remove . at end of summary.

c)
W: popt devel-file-in-non-devel-package /usr/lib64/libpopt.a
W: popt devel-file-in-non-devel-package /usr/include/popt.h
W: popt devel-file-in-non-devel-package /usr/lib64/libpopt.so

There's a comment that says:
# XXX These may end up in popt-devel but it hardly seems worth the effort.
%{__libdir}/libpopt.a
%{__libdir}/libpopt.so
%{__includedir}/popt.h
Is it still not worth the effort? Or perhaps now is a good time?
Or perhaps they can just be removed if nothing is using them?

d)
W: rpm prereq-use fileutils shadow-utils

fileutils was replaced a while back with coreutils,
which is in the min build root exceptions. For shadow-utils,
you could do: 
Requires(pre): shadow-utils
Requires(postun): shadow-utils

e)
W: rpm macro-in-%changelog ghost

Suggest: Change "%ghost" in the changelog to "%%ghost"

f)
W: rpm mixed-use-of-spaces-and-tabs (spaces: line 267, tab: line 1)

Suggest: pick tabs or spaces.

g)
W: rpm patch-not-applied Patch9: rpm-4.4.2-contextverify.patch
W: rpm patch-not-applied Patch12: rpm-4.4.2-exclude.patch

Suggest: should those patches just be removed?

h)
E: rpm obsolete-not-provided rpm-perl

Suggest: Do we need to have this obsolete around anymore?

i)
W: rpm file-not-utf8 /usr/share/man/sk/man8/rpm.8.gz
W: rpm file-not-utf8 /usr/share/man/pl/man8/rpm.8.gz
W: rpm file-not-utf8 /usr/share/man/pl/man8/rpmbuild.8.gz
W: rpm file-not-utf8 /usr/share/man/pl/man8/rpmdeps.8.gz
W: rpm file-not-utf8 /usr/share/man/pl/man8/rpmgraph.8.gz
W: rpm file-not-utf8 /usr/share/man/pl/man1/gendiff.1.gz
W: rpm file-not-utf8 /usr/share/man/pl/man8/rpmcache.8.gz
W: rpm file-not-utf8 /usr/share/man/pl/man8/rpm2cpio.8.gz

Suggest: Perhaps run iconv on those?

j)
E: rpm script-without-shebang /usr/lib/rpm/rpm.xinetd

Suggest: should this be shipped at all? or installed in /etc/xinetd.d/ ?
Or at the very least it should be mode 644.

k)
E: rpm script-without-shebang /usr/lib/rpm/rpm.log

Suggest: This is a duplicate of /etc/logrotate.d/rpm, and can be removed?

l)
E: rpm executable-marked-as-config-file /etc/cron.daily/rpm

Suggest: Don't mark that a config file.

m)
E: rpm-build script-without-shebang /usr/lib/rpm/magic
E: rpm-build script-without-shebang /usr/lib/rpm/magic.mime
E: rpm-build script-without-shebang /usr/lib/rpm/config.site

Suggest: all 3 of those should be mode 644?

n)
W: rpm-debuginfo spurious-executable-perm /usr/src/debug/rpm-4.4.2/rpmqv.c

Suggest: source file should be 644?

The following can also be ignored. Since rpm needs rpmmacros to use
%configure or the like it can't do that when building itself.
Or they otherwise appear to be false positives to me.

E: rpm configure-without-libdir-spec
E: rpm configure-without-libdir-spec
E: rpm hardcoded-library-path in /usr/lib/rpmrc
E: rpm standard-dir-owned-by-package /var/lib/rpm
W: rpm-libs no-documentation
W: rpm-python no-documentation
W: rpm dangerous-command-in-%pre rpm
W: rpm dangerous-command-in-%post chown
W: rpm dangerous-command-in-%postun userdel
E: rpm-build statically-linked-binary /usr/lib/rpm/debugedit
E: rpm-python script-without-shebang
/usr/lib64/python2.5/site-packages/rpm/__init__.py

15. Are all the static libs in -devel needed? If so perhaps a -static subpackage?

16. The 'Requires: python >= %{with_python_version}' isn't needed.
rpm adds a 'python(abi) = 2.5' to requires without it.

17. There are currently 147 outstanding open bugs against rpm.
You might consider going and doing some triage on them. If you like
I would be willing to assist in doing so. Some of them do seem related
to packaging concerns. Since there is a upstream at rpm.org, many
could possibly be pushed up there (especially patches/enhancement requests).
Perhaps we could even round up a QA team session and get them addressed.

18. Is there any chance of trying the sqlite backend? I know it's supposed
to be slower than DB, but if it is more reliable it will be well worth it
in my opinion. 

I would also be willing to assist in generating a patch to the spec
addressing many of the above if you would like me to.

Once you have addressed these items (either by making the suggested changes, or
by explaining why they don't make sense), please reassign this review back 
to me, and change the 'fedora-review' flag back to ? for me to take action. 


-- 
Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug, or are watching the QA contact.

_______________________________________________
Fedora-package-review mailing list
Fedora-package-review@xxxxxxxxxx
http://www.redhat.com/mailman/listinfo/fedora-package-review

[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]