Bug 226666 - Merge Review: yum
Merge Review: yum
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jeremy Katz
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 16:36 EST by Nobody's working on this, feel free to take it
Modified: 2013-01-10 05:16 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-07-11 07:48:33 EDT
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)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 16:36:33 EST
Fedora Merge Review: yum

http://cvs.fedora.redhat.com/viewcvs/devel/yum/
Initial Owner: katzj@redhat.com
Comment 1 Tim Lauridsen 2007-02-13 08:20:08 EST
This is my first review so maybe somebody should take a extra look at my review.
Comment 2 Tim Lauridsen 2007-02-13 08:23:25 EST
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 09:06:28 EST
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 09:15:22 EST
[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 09:21:32 EST
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 10:41:36 EST
(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 16:27:10 EST
(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 16:51:07 EST
(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 03:11:05 EST
The updated spec look fine to me, so i will approve it

APPROVED
Comment 10 Patrice Dumas 2007-02-14 04:19:12 EST
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-10 21:56:46 EDT
(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 02:41:21 EDT
Ok, with me

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