Bug 220860

Summary: Review Request: galternatives - Alternatives Configurator
Product: [Fedora] Fedora Reporter: Deji Akingunola <dakingun>
Component: Package ReviewAssignee: Mamoru TASAKA <mtasaka>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: mtasaka, panemade
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2007-01-03 03:10:04 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 163779    

Description Deji Akingunola 2006-12-28 04:12:54 UTC
Spec URL: ftp://czar.eas.yorku.ca/pub/galternatives/galternatives.spec
SRPM URL: ftp://czar.eas.yorku.ca/pub/galternatives/galternatives-0.13.4-1.src.rpm
Description: Graphical setup tool for the alternatives system. A GUI to help the system
administrator to choose what program should provide a given service.

Builds fine in (rawhide) mock.

Comment 1 Mamoru TASAKA 2006-12-29 02:38:37 UTC
Please check the packages you created by rpmlint

Very Very quick note:
--------------------------------------
E: galternatives no-binary
E: galternatives non-executable-script
/usr/lib/python2.5/site-packages/galternatives/main.py 0644
E: galternatives non-executable-script
/usr/lib/python2.5/site-packages/galternatives/gadebug.py 0644
E: galternatives non-executable-script
/usr/lib/python2.5/site-packages/galternatives/common.py 0644
E: galternatives non-executable-script
/usr/lib/python2.5/site-packages/galternatives/__init__.py 0644
E: galternatives non-executable-script
/usr/lib/python2.5/site-packages/galternatives/alternative.py 0644
E: galternatives-debuginfo empty-debuginfo-package
----------------------------------------
* This seems to be a noarch rpm
* Scripts with shebang should have executable permission
  (or, if 0644 permissons are correct, shebangs should be removed)
* .pyo files are not marked as ghosts due to SELinux issues.
* Please check Requires
  - This package should require pam explicitly
  - And "include" line of pam configuration file means that
    requirement of pam is version-specific (pam >= 0.80)

Comment 2 Deji Akingunola 2006-12-29 05:40:23 UTC
(In reply to comment #1)
> Please check the packages you created by rpmlint
> 


> * This seems to be a noarch rpm
Yes, you're right, changed it to be so.

> * Scripts with shebang should have executable permission
>   (or, if 0644 permissons are correct, shebangs should be removed)
This is _strictly_ _not_ necessary (I've seen other reviews ignoring these kind
of warning on python packages); anyways to make everyone happy, I've sed out the
shebangs.

> * .pyo files are not marked as ghosts due to SELinux issues.
Right, fixed.

> * Please check Requires
>   - This package should require pam explicitly
It does already rightly requires usermode, which in turn explicitly requires
pam. Besides other packages in Extras that uses consolehelper only requires
usermode.
New file with changes uploaded, thanks for the review.
Spec URL: ftp://czar.eas.yorku.ca/pub/galternatives/galternatives.spec
SRPM URL: ftp://czar.eas.yorku.ca/pub/galternatives/galternatives-0.13.4-2.src.rpm





Comment 3 Mamoru TASAKA 2006-12-29 07:18:18 UTC
... not a packaging issue, however...

I get the following backtrace.
-----------------------------------------------------
[root@softbank218114170036 ~]# LANG=C alternatives --display print
print - status is auto.
 link currently points to /usr/bin/lpr.cups
/usr/bin/lpr.cups - priority 40
 slave print-cancel: /usr/bin/cancel.cups
 slave print-lp: /usr/bin/lp.cups
 slave print-lpq: /usr/bin/lpq.cups
 slave print-lprm: /usr/bin/lprm.cups
 slave print-lpstat: /usr/bin/lpstat.cups
 slave print-lpc: /usr/sbin/lpc.cups
 slave print-cancelman: /usr/share/man/man1/cancel-cups.1.gz
 slave print-lpman: /usr/share/man/man1/lp-cups.1.gz
 slave print-lpqman: /usr/share/man/man1/lpq-cups.1.gz
 slave print-lprman: /usr/share/man/man1/lpr-cups.1.gz
 slave print-lprmman: /usr/share/man/man1/lprm-cups.1.gz
 slave print-lpstatman: /usr/share/man/man1/lpstat-cups.1.gz
 slave print-lpcman: /usr/share/man/man8/lpc-cups.8.gz
Current `best' version is /usr/bin/lpr.cups.
[root@softbank218114170036 ~]# galternatives 
==========================================
(then select the item of print)
==========================================
Traceback (most recent call last):
  File "/usr/lib/python2.5/site-packages/galternatives/main.py", line 364, in
alternative_selected_cb
    self.update_options_tree ()
  File "/usr/lib/python2.5/site-packages/galternatives/main.py", line 400, in
update_options_tree
    self.PRIORITY, int(option['priority']),
ValueError: invalid literal for int() with base 10: '40 cups'
----------------------------------------------------

  Would you know why?

Comment 4 Mamoru TASAKA 2006-12-29 08:03:56 UTC
Well, also I
* launch galternatives
* select one item on "alternatimes" column (in the left)
* select one options
* push the button "Properties" (then a new window titled "Details" pops up)
* Break the windows which poped up newly
* Then push again the button "Properties"

--- Then the new window poped up is totally gray..

Comment 5 Mamoru TASAKA 2006-12-29 08:14:21 UTC
(In reply to comment #3)
> I get the following backtrace.
> -----------------------------------------------------
> [root@softbank218114170036 ~]# LANG=C alternatives --display print
> print - status is auto.
>  link currently points to /usr/bin/lpr.cups
> /usr/bin/lpr.cups - priority 40
>  slave print-cancel: /usr/bin/cancel.cups
>  slave print-lp: /usr/bin/lp.cups
>  slave print-lpq: /usr/bin/lpq.cups
>  slave print-lprm: /usr/bin/lprm.cups
>  slave print-lpstat: /usr/bin/lpstat.cups
>  slave print-lpc: /usr/sbin/lpc.cups
>  slave print-cancelman: /usr/share/man/man1/cancel-cups.1.gz
>  slave print-lpman: /usr/share/man/man1/lp-cups.1.gz
>  slave print-lpqman: /usr/share/man/man1/lpq-cups.1.gz
>  slave print-lprman: /usr/share/man/man1/lpr-cups.1.gz
>  slave print-lprmman: /usr/share/man/man1/lprm-cups.1.gz
>  slave print-lpstatman: /usr/share/man/man1/lpstat-cups.1.gz
>  slave print-lpcman: /usr/share/man/man8/lpc-cups.8.gz
> Current `best' version is /usr/bin/lpr.cups.
> [root@softbank218114170036 ~]# galternatives 
> ==========================================
> (then select the item of print)
> ==========================================
> Traceback (most recent call last):
>   File "/usr/lib/python2.5/site-packages/galternatives/main.py", line 364, in
> alternative_selected_cb
>     self.update_options_tree ()
>   File "/usr/lib/python2.5/site-packages/galternatives/main.py", line 400, in
> update_options_tree
>     self.PRIORITY, int(option['priority']),
> ValueError: invalid literal for int() with base 10: '40 cups'
> ----------------------------------------------------
> 
>   Would you know why?

Ah... I found because /var/lib/alternatives/print says:
------------------------------------------------------
auto
/usr/bin/lpr
print-cancel
<snip>

/usr/bin/lpr.cups
40 cups <- THIS LINE
/usr/bin/cancel.cups
<snip>
--------------------------------------------------

And....
--------------------------------------------------
[tasaka1@localhost ~]$ rpm -q --scripts cups
postinstall scriptlet (using /bin/sh):
<snip>
/usr/sbin/alternatives --install /usr/bin/lpr print /usr/bin/lpr.cups 40 \
         --slave /usr/bin/lp print-lp /usr/bin/lp.cups \
<snip>
         --initscript cups
--------------------------------------------------
and --initscripts option is "a Red Hat Linux specific option"
according to "man alternatives"

Then a patch is needed 
* to treat initscripts options correctly
* or to ignore initscripts options for now. Then a documentation
  (like README.fedora) which explains that this package ignores
  initscripts option currently.

Comment 6 Deji Akingunola 2006-12-29 08:32:41 UTC
(In reply to comment #4)
> Well, also I
> * launch galternatives
> * select one item on "alternatimes" column (in the left)
> * select one options
> * push the button "Properties" (then a new window titled "Details" pops up)
> * Break the windows which poped up newly
> * Then push again the button "Properties"
> 
> --- Then the new window poped up is totally gray..
This is a longstanding upstream bug, its not resolved yet.

I was just about point out the --initscript option issue, but you already
figured it out, I see what I can do about it tomorrow (well, later today ;).


Comment 7 Deji Akingunola 2006-12-30 01:24:09 UTC
> --------------------------------------------------
> and --initscripts option is "a Red Hat Linux specific option"
> according to "man alternatives"
> 
> Then a patch is needed 
> * to treat initscripts options correctly
> * or to ignore initscripts options for now. Then a documentation
>   (like README.fedora) which explains that this package ignores
>   initscripts option currently.

I've added a patch to treat the initscripts appropriately, and thus fixing the
traceback. Updated spec and srpm below;
Spec URL: ftp://czar.eas.yorku.ca/pub/galternatives/galternatives.spec
SRPM URL: ftp://czar.eas.yorku.ca/pub/galternatives/galternatives-0.13.4-3.src.rpm




Comment 8 Mamoru TASAKA 2006-12-30 07:36:04 UTC
Your patch works well, thanks.

Then, first full review for galternatives

* General notes for python related packages

  - Unlike shared libraries' dependencies, python related
  dependencies are not checked automatically by rpmbuild
  and these have to be looked into manually.

  Normally, this can be done by checking what modules this
  package has to import. 

  For this package
------------------------------------------------------------
[tasaka1@localhost ~]$ grep import /usr/sbin/galternatives `rpm -ql
galternatives | grep py$` 
/usr/sbin/galternatives:import os, sys
/usr/sbin/galternatives:import galternatives
/usr/sbin/galternatives:from galternatives import gtk
/usr/sbin/galternatives:import gettext
/usr/sbin/galternatives:from galternatives.common import PACKAGE
/usr/lib/python2.5/site-packages/galternatives/__init__.py:from main import *
/usr/lib/python2.5/site-packages/galternatives/alternative.py:from common import
PACKAGE
/usr/lib/python2.5/site-packages/galternatives/alternative.py:import os, gettext
/usr/lib/python2.5/site-packages/galternatives/alternative.py:from gadebug
import print_debug
/usr/lib/python2.5/site-packages/galternatives/main.py:import pygtk
/usr/lib/python2.5/site-packages/galternatives/main.py:   import gtk, gobject
/usr/lib/python2.5/site-packages/galternatives/main.py:   from gtk import glade
/usr/lib/python2.5/site-packages/galternatives/main.py:from common import PACKAGE
/usr/lib/python2.5/site-packages/galternatives/main.py:import sys, os, gettext
/usr/lib/python2.5/site-packages/galternatives/main.py:from alternative import
Alternative
------------------------------------------------------------
   This means that this package needs "Requires: pygtk2"

Then
A. http://fedoraproject.org/wiki/Packaging/Guidelines
* Licensing
  - Please include the license document. For this package, debian/copyright
    seems the best
  - Also, adding debian/changelog seems useful and should be included in
    the package.

B. http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
   = This is okay, except for issues on A

C. Other notes:
  - For upstream URL:
    Maybe http://packages.qa.debian.org/g/galternatives.html is more useful?

  - I think this package to be useful, however, how do you think of
    http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=365365 ?

Comment 9 Deji Akingunola 2006-12-30 10:10:57 UTC
(In reply to comment #8)

>    This means that this package needs "Requires: pygtk2"
>
I think I ave this in before, don't know why I removed it, re-added.
 
> Then
> A. http://fedoraproject.org/wiki/Packaging/Guidelines
> * Licensing
>   - Please include the license document. For this package, debian/copyright
>     seems the best
>   - Also, adding debian/changelog seems useful and should be included in
>     the package.
> 
Done.

> C. Other notes:
>   - For upstream URL:
>     Maybe http://packages.qa.debian.org/g/galternatives.html is more useful?
>
I'm not sure, I've changed to it though (at least the latest source package is
found there).
 
>   - I think this package to be useful, however, how do you think of
>     http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=365365 ?
Yes, I've seen it. However, I also believe the package to be useful and as long
as there is no security issue(s) with it, and no real alternative for it, then
we can as well have it and maintain it for fedora.
File with new changes below. Thanks for the thorough review.
Spec URL: ftp://czar.eas.yorku.ca/pub/galternatives/galternatives.spec
SRPM URL: ftp://czar.eas.yorku.ca/pub/galternatives/galternatives-0.13.4-4.src.rpm



Comment 10 Mamoru TASAKA 2006-12-30 16:24:27 UTC
Now
* This package meets ReviewGuidelines/Guidelines
* Works functionally

Okay
-------------------------------------------------
   This package (galternatives) is APPROVED by me