Bug 237170 - Review Request: repoman - Tool for configuring yum(8) settings and repositories
Review Request: repoman - Tool for configuring yum(8) settings and repositories
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Kevin Fenzi
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-04-19 16:06 EDT by David Cantrell
Modified: 2007-11-30 17:12 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-05-10 09:52:08 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kevin: fedora‑review+
wtogami: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description David Cantrell 2007-04-19 16:06:46 EDT
Spec URL: http://www.boston.burdell.org/repoman/RPMS/source/repoman.spec
SRPM URL: http://www.boston.burdell.org/repoman/RPMS/source/repoman-0.7-1.fc7.src.rpm
Description: repoman is a graphical application for configuring yum(8) settings,
enabling and disabling repositories, and configuring yum(8) plug-ins.
Comment 1 Kevin Fenzi 2007-04-23 00:08:43 EDT
Hey David. Here's a review. 

It looks like you also need a sponsor? 

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
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
aba4be5ea7da8cb0f751e1400d509acf  repoman-0.7.tar.gz
aba4be5ea7da8cb0f751e1400d509acf  repoman-0.7.tar.gz.1
OK - BuildRequires correct
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Package has correct buildroot
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.
OK - Package has rm -rf RPM_BUILD_ROOT at top of %install
See below - Package is a GUI app and has a .desktop file
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.
OK - 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 dist tag
OK - Should package latest version
    
Issues:

1. The source url doesn't seem quite right
http://www.boston.burdell.org/repoman/src/repoman-0.7.tar.gz
works. (ie, it needs a /src/ in there)

2. rpmlint says:

a) W: repoman no-dependency-on usermode

Should "Requires: usermode" since you have a link to consolehelper.

b) W: repoman incoherent-version-in-changelog 0.7 0.7-1.fc7

Should have the Release on the versions in the changelog...
ie, 0.7-1

c)
W: repoman conffile-without-noreplace-flag /etc/pam.d/repoman
W: repoman conffile-without-noreplace-flag /etc/security/console.apps/repoman

Are users ever likely to modify those files? Should they be noreplace?

2. You shouldn't need to require desktop-file-utils anymore, also you
might use the standardized scriptlet for updating the mime-type key. See:
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#head-de6770dd9867fcd085a73a4700f6bcd0d10294ef

3. You should use desktop-file-install to install the .desktop file:
http://fedoraproject.org/wiki/Packaging/Guidelines#head-254ddf07aae20a23ced8cecc219d8f73926e9755

4. Is there a reason for the (8) after yum in the summary and description?
I find it distracting, and many people won't know what it means.

Finally two items that are by no means blockers, but I thought I would mention:

- Perhaps you could talk with the yum-presto maintainer and see if it would
be possible/easy to add support for deltarpm repos when they appear?

- I see that this application doesn't have an icon. Perhaps you could ask for
someone on the art group to whip one up?
http://fedoraproject.org/wiki/Artwork/DesignService
Comment 2 David Cantrell 2007-04-23 14:19:11 EDT
(In reply to comment #1)
> It looks like you also need a sponsor?

Yes.

> 1. The source url doesn't seem quite right
> http://www.boston.burdell.org/repoman/src/repoman-0.7.tar.gz
> works. (ie, it needs a /src/ in there)

Fixed.

> 2. rpmlint says:
> 
> a) W: repoman no-dependency-on usermode
> 
> Should "Requires: usermode" since you have a link to consolehelper.

Fixed.
 
> b) W: repoman incoherent-version-in-changelog 0.7 0.7-1.fc7
> 
> Should have the Release on the versions in the changelog...
> ie, 0.7-1

Fixed.

> c)
> W: repoman conffile-without-noreplace-flag /etc/pam.d/repoman
> W: repoman conffile-without-noreplace-flag /etc/security/console.apps/repoman
> 
> Are users ever likely to modify those files? Should they be noreplace?

Most likely users will never have to modify those files.  But, they are config
files and we wouldn't go to the trouble of making them config files if we didn't
want to give the users the option of changing them.  I've added the noreplace
attribute.

> 2. You shouldn't need to require desktop-file-utils anymore, also you
> might use the standardized scriptlet for updating the mime-type key. See:
>
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#head-de6770dd9867fcd085a73a4700f6bcd0d10294ef

Fixed.

> 3. You should use desktop-file-install to install the .desktop file:
>
http://fedoraproject.org/wiki/Packaging/Guidelines#head-254ddf07aae20a23ced8cecc219d8f73926e9755

Fixed, I think.  Not sure if I'm using this correctly.

> 4. Is there a reason for the (8) after yum in the summary and description?
> I find it distracting, and many people won't know what it means.

Only to indicate it's a command with a man page.  Removed the (8).

> Finally two items that are by no means blockers, but I thought I would mention:
> 
> - Perhaps you could talk with the yum-presto maintainer and see if it would
> be possible/easy to add support for deltarpm repos when they appear?

Definitely something to look in to.  Added it to the TODO list.

> - I see that this application doesn't have an icon. Perhaps you could ask for
> someone on the art group to whip one up?
> http://fedoraproject.org/wiki/Artwork/DesignService

Also added to the TODO list.

I have put all of these changes together in repoman-0.8.  Here is the new srpm
and spec file:
http://www.boston.burdell.org/repoman/RPMS/source/repoman-0.8-1.fc7.src.rpm
http://www.boston.burdell.org/repoman/RPMS/source/repoman.spec
Comment 3 Kevin Fenzi 2007-05-01 23:12:41 EDT
Sorry for the delay, I was out on vacation... 

1. ok. Looks good. 
2.a. ok. Looks good. 
2.b. ok. Looks good. 
2.c. ok. Looks good. 
2 (oops. I seem to have failed numbering here). ok. looks good. 
3. That looks right to me on the desktop file install. 
4. good. 

You seem to have removed the 'rm -rf $RPM_BUILD_ROOT' from the top of %install.
That needs to be there... 

Humm... the 0.8 version doesn't seem to build here in mock: 

+ desktop-file-install --vendor=fedora --delete-original
--dir=/var/tmp/repoman-0.8-1.fc7-root-mockbuild/usr/share/applications/fedora
/var/tmp/repoman-0.8-1.fc7-root-mockbuild/usr/share/applications/fedora/repoman.desktop
Error on file
"/var/tmp/repoman-0.8-1.fc7-root-mockbuild/usr/share/applications/fedora/repoman.desktop":
Failed to open file
'/var/tmp/repoman-0.8-1.fc7-root-mockbuild/usr/share/applications/fedora/repoman.desktop':
No such file or directory
error: Bad exit status from /var/tmp/rpm-tmp.16798 (%install)
Comment 4 Kevin Fenzi 2007-05-02 13:58:24 EDT
Some more enhancement suggestions: 

- Would it make sense for repoman to allow you to enable/disable plugins?

- Could you look at adding support for the priorities plugin?
http://wiki.centos.org/PackageManagement/Yum/Priorities

Just RFE's, nothing blocking this review. 
Comment 5 Chris Lumens 2007-05-04 12:03:36 EDT
David - I believe I've fixed the issues in comment #3.  Want to check it out and
do a new release?
Comment 6 David Cantrell 2007-05-07 10:31:30 EDT
New release that to fix up remaining review issues:

http://www.boston.burdell.org/repoman/RPMS/source/repoman-0.9-1.fc7.src.rpm
http://www.boston.burdell.org/repoman/RPMS/source/repoman.spec

The RFEs posted here are all great ideas.  I'd rather not post there here since
this bug is just for the review though.
Comment 7 Kevin Fenzi 2007-05-08 01:40:58 EDT
The package from comment #6 seems to have all blockers I can see fixed. 

So, this package is APPROVED. 

Don't forget to close this review request once it's been imported and built. 

It looks like you added the RFE's from here to the TODO file, which is a fine
place for them. ;) 
Comment 8 Chris Lumens 2007-05-08 10:04:21 EDT
New Package CVS Request
=======================
Package Name: repoman
Short Description: Tool for configuring yum settings and repositories
Owners: dcantrell@redhat.com,clumens@redhat.com
Branches: 
InitialCC: 

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