Bug 226666 - Merge Review: yum
Summary: Merge Review: yum
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review   
(Show other bugs)
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jeremy Katz
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Keywords:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-01-31 21:36 UTC by Nobody's working on this, feel free to take it
Modified: 2013-01-10 10:16 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-07-11 11:48:33 UTC
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tla: fedora-review+


Attachments (Terms of Use)

Description Nobody's working on this, feel free to take it 2007-01-31 21:36:33 UTC
Fedora Merge Review: yum

http://cvs.fedora.redhat.com/viewcvs/devel/yum/
Initial Owner: katzj@redhat.com

Comment 1 Tim Lauridsen 2007-02-13 13:20:08 UTC
This is my first review so maybe somebody should take a extra look at my review.


Comment 2 Tim Lauridsen 2007-02-13 13:23:25 UTC
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)


Comment 3 Tim Lauridsen 2007-02-13 14:06:28 UTC
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.






Comment 4 Tim Lauridsen 2007-02-13 14:15:22 UTC
[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.





Comment 5 Tim Lauridsen 2007-02-13 14:21:32 UTC
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.



Comment 6 James Bowes 2007-02-13 15:41:36 UTC
(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



Comment 7 Ville Skyttä 2007-02-13 21:27:10 UTC
(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?

Comment 8 James Bowes 2007-02-13 21:51:07 UTC
(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.



Comment 9 Tim Lauridsen 2007-02-14 08:11:05 UTC
The updated spec look fine to me, so i will approve it

APPROVED

Comment 10 Patrice Dumas 2007-02-14 09:19:12 UTC
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

Comment 11 James Bowes 2007-07-11 01:56:46 UTC
(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?

Comment 12 Tim Lauridsen 2007-07-11 06:41:21 UTC
Ok, with me



Comment 13 Patrice Dumas 2007-07-11 19:44:45 UTC
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.


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