Bug 226666

This is my first review so maybe somebody should take a extra look at my review.

Must :
OK - spec filename is %{name}.spec
OK - source match upstream md5sum
     CVS:          6e6953f92531aa0f9074199f2925d22a  yum-3.1.1.tar.gz
     Upstream :	   6e6953f92531aa0f9074199f2925d22a  yum-3.1.1.tar.gz
OK - Package naming 
OK - Spec in American English and legible
OK - License : GPL
OK - BuildRequires correct
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - License file (COPYING) is included in %doc
   - Package compiles and builds on at least one arch.
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.
OK - Package owns all the directories it creates.
FAIL - Buildroot should be
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

Comments:
 * Source0 should end with %{name}-%{version}, not yum-%{version}	
 * Requires: python (Upstream spec has Requires: python >= 2.4)
 * Requires: rpm >= 0:4.1.1 ( Upstream spec has Requires:  rpm >= 0:4.4.2)

rpmlinst output:

[tim@naboo devel]$ rpmlint yum-3.1.1-1.src.rpm 
W: yum prereq-use /sbin/chkconfig
W: yum prereq-use /sbin/service
E: yum hardcoded-library-path in $RPM_BUILD_ROOT/usr/lib/yum-plugins
E: yum hardcoded-library-path in $RPM_BUILD_ROOT/usr/lib/yum-plugins/installonlyn.py
E: yum hardcoded-library-path in /usr/lib/yum-plugins
E: yum hardcoded-library-path in /usr/lib/yum-plugins/*

I am sure what the prereq warning means.

the hardcoded library error should be ignored i think, because the plugins has
to go into /usr/lib/yum-plugins/ always, even on 64 bit systems.

[tim@naboo devel]$ rpmlint ~/rpmbuild/RPMS/noarch/yum-3.1.1-1.noarch.rpm 
E: yum only-non-binary-in-usr-lib
W: yum conffile-without-noreplace-flag /etc/logrotate.d/yum
E: yum non-executable-script /usr/lib/python2.5/site-packages/yum/packages.py 0644
E: yum non-executable-script /usr/lib/python2.5/site-packages/yum/depsolve.py 0644
E: yum non-executable-script /usr/lib/python2.5/site-packages/yum/Errors.py 0644
E: yum non-executable-script /usr/lib/python2.5/site-packages/yum/sqlitesack.py 0644
E: yum non-executable-script /usr/lib/python2.5/site-packages/rpmUtils/arch.py 0644
E: yum non-executable-script /usr/share/yum-cli/progress_meter.py 0644
E: yum non-executable-script
/usr/lib/python2.5/site-packages/yum/storagefactory.py 0644
E: yum non-executable-script /usr/share/yum-cli/yumupd.py 0644
E: yum non-executable-script /usr/lib/python2.5/site-packages/yum/__init__.py 0644
E: yum non-executable-script /usr/share/yum-cli/yummain.py 0644
E: yum non-executable-script /usr/share/yum-cli/utils.py 0644
E: yum non-executable-script /usr/share/yum-cli/output.py 0644
E: yum non-executable-script /usr/lib/python2.5/site-packages/yum/config.py 0644
E: yum non-executable-script /usr/share/yum-cli/callback.py 0644
E: yum non-executable-script
/usr/lib/python2.5/site-packages/rpmUtils/transaction.py 0644
E: yum non-executable-script /usr/share/yum-cli/i18n.py 0644
E: yum non-executable-script
/usr/lib/python2.5/site-packages/rpmUtils/__init__.py 0644
E: yum non-executable-script
/usr/lib/python2.5/site-packages/rpmUtils/updates.py 0644
E: yum non-executable-script
/usr/lib/python2.5/site-packages/rpmUtils/oldUtils.py 0644
E: yum non-executable-script /usr/lib/python2.5/site-packages/yum/packageSack.py
0644
E: yum non-executable-script
/usr/lib/python2.5/site-packages/rpmUtils/miscutils.py 0644
E: yum non-executable-script /usr/lib/python2.5/site-packages/yum/sqlitecache.py
0644
E: yum non-executable-script /usr/lib/python2.5/site-packages/yum/rpmsack.py 0644
E: yum non-executable-script /usr/share/yum-cli/yumcommands.py 0644
E: yum non-executable-script
/usr/lib/python2.5/site-packages/yum/repoMDObject.py 0644
E: yum non-executable-script /usr/lib/python2.5/site-packages/yum/sqlutils.py 0644
E: yum non-executable-script /usr/lib/python2.5/site-packages/yum/update_md.py 0644
E: yum non-executable-script /usr/lib/python2.5/site-packages/yum/repos.py 0644
E: yum non-executable-script /usr/share/yum-cli/cli.py 0644
E: yum non-executable-script /usr/lib/python2.5/site-packages/yum/failover.py 0644

only-non-binary-in-usr-lib is ok, because the yum-plugins has to go into
/usr/lib/yum-plugins.

yum non-executable-script is ok too, the .py files is not called directly, so
they dont need to be executables, could be fixed in the upstream Makefile's to
use 755 insted of 644 for the .py files.

conffile-without-noreplace-flag /etc/logrotate.d/yum has to be checked, should
it be overwriten every time.

Summery:
 * Buildroot should be fixed.
 * The rpm and version should match the ones in upstream spec.
 * The 'prereq' warning should be checked out.
 * the 'conffile-without-noreplace-flag' warning should be investigated.

(In reply to comment #5)
> Summery:
>  * Buildroot should be fixed.
>  * The rpm and version should match the ones in upstream spec.
>  * The 'prereq' warning should be checked out.
>  * the 'conffile-without-noreplace-flag' warning should be investigated.

I fixed up the buildroot, rpm and python versions, and the noreplace config
file. I also used the name macro in source0.

prereq should be ok, since we do want to have yum-updatesd running by default.

The new version is yum-3.1.1-2

(In reply to comment #4)
> only-non-binary-in-usr-lib is ok, because the yum-plugins has to go into
> /usr/lib/yum-plugins.

Wouldn't /usr/share/yum-plugins be a more appropriate location for them?

(In reply to comment #7)

> Wouldn't /usr/share/yum-plugins be a more appropriate location for them?

Probably. I'll bring it up on yum-devel.

The updated spec look fine to me, so i will approve it

APPROVED

Issues:

* Why is it a BuildRequires: python and not Buildrequires: python-devel?

* it seems to me that the python-devel BuildRequires should be versionned
then Requires: python >= 2.4 would be autodetected 

* Prereq should be replaced with Requires(preun) and so on...

* Is the conflict really needed? pirut >= 1.1.4 should requires a
recent enough yum version.

suggestions:

* use %defattr(-, root, root, -) instead of %defattr(-, root, root)

* prefix plugin.conf source file by a disambiguating prefix like
yum-plugin.conf

* remove the -f to be notified when the file doesn't exist/change name...
rm -f $RPM_BUILD_ROOT/%{_sysconfdir}/yum/yum.conf

* there is an occurence of /etc hardcoded in the specfile, it could be
  %_sysconfdir. However the upstream Makefiles and programs have many
  paths hardcoded so this is not really an issue. (There is also a /var
  and some /usr, but I guess they are also hardcoded in the package).

* It seems to me that BuildArch is more used that BuildArchitectures

* There is no need of / after $RPM_BUILD_ROOT

(In reply to comment #10)
> Issues:
> 
> * Why is it a BuildRequires: python and not Buildrequires: python-devel?
> 
> * it seems to me that the python-devel BuildRequires should be versionned
> then Requires: python >= 2.4 would be autodetected

Yum uses a Makefile + python to byte compile the code rather than setup.py, so
it wouldn't actually BR: python-devel

> * Prereq should be replaced with Requires(preun) and so on...

These are fixed up already.

> * Is the conflict really needed? pirut >= 1.1.4 should requires a
> recent enough yum version.
 
pirut < 1.1.4 would blow up with new yum, so the conflict is needed. Now,
whether or not yum in F7 and rawhide should care about older pirut is a
different issue...


> suggestions:
> 
> * use %defattr(-, root, root, -) instead of %defattr(-, root, root)
> 
> * prefix plugin.conf source file by a disambiguating prefix like
> yum-plugin.conf
> 
> * remove the -f to be notified when the file doesn't exist/change name...
> rm -f $RPM_BUILD_ROOT/%{_sysconfdir}/yum/yum.conf
> 
> * there is an occurence of /etc hardcoded in the specfile, it could be
>   %_sysconfdir. However the upstream Makefiles and programs have many
>   paths hardcoded so this is not really an issue. (There is also a /var
>   and some /usr, but I guess they are also hardcoded in the package).
> 
> * It seems to me that BuildArch is more used that BuildArchitectures
> 
> * There is no need of / after $RPM_BUILD_ROOT

defattr, buildarch, and macro suggestions incorporated in devel.

Everyone ok with closing this one?

Ok, with me

You should keep timestamps of installed files, for example
you could use

install -m 0644 -p %{SOURCE2} $RPM_BUILD_ROOT/%{_sysconfdir}/yum/yum-updatesd.conf

For the file are installed by make install nothing can be done,
maybe it should be suggested to upstream to use $(INSTALL) instead
of install, and set
INSTALL = install

In any case, this is not blocking.