Bug 220860 - Review Request: galternatives - Alternatives Configurator
Review Request: galternatives - Alternatives Configurator
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-12-27 23:12 EST by Deji Akingunola
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-01-02 22:10:04 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)

  None (edit)
Description Deji Akingunola 2006-12-27 23:12:54 EST
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-28 21:38:37 EST
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 00:40:23 EST
(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 02:18:18 EST
... 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 03:03:56 EST
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 03:14:21 EST
(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 03:32:41 EST
(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-29 20:24:09 EST
> --------------------------------------------------
> 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 02:36:04 EST
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 05:10:57 EST
(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 11:24:27 EST
Now
* This package meets ReviewGuidelines/Guidelines
* Works functionally

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

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