Bug 492479

I've reviewed your package and it looks quite good. There is one TODO item and I have a small usage question (see below).

* rpmlint: OK
rpmlint SPECS/razertool.spec RPMS/i386/razertool-* SRPMS/razertool-0.0.7-1.fc10.src.rpm
3 packages and 1 specfiles checked; 0 errors, 0 warnings.

* naming: OK

* spec file name: OK

* License: OK
- GPLv2+: acceptable
- matches the actual license
- license file packaged

* spec file in English and legible: OK

* Source0: OK
- newest upstream version used
- source file matches upstream - md5sum:
dad8236b2fface39f054a7ca6d1a448c  razertool_0.0.7.orig.tar.gz
- spectool -g works

* compilation: OK
- mock (F10)
- koji: F11, F10 and F9
- %{optflags} are used correctly
- smpflags used

* locale handling: OK

* debuginfo: OK
- debuginfo files are not empty and can be used in gdb

* handling of shared/static libraries, header files, pkgconfig files: OK (n/a)

* directory ownership: OK
- no directories created
- only directories used which are included in the base filesystem

* no files listed twice: OK

* file permissions and ownership: OK

* %clean section: OK

* macro usage: OK

* code vs. content: OK (only code)

* subpackage for large documentation: OK (n/a)

* %doc: OK

* rm -rf in %install: OK

* filenames UTF8: OK

* desktop file: OK
- it may be debatable in which menu the razortool appears (Preferences vs. Administration), but since it changes the configuration values globally (in the mouse), Administration seems the best option...

* buildrequires: OK

* scriptlets: OK

* final requires: OK

* functional test: TODO
- programs run without segfault ;-)
- since I don't have such a device, I can't do a full functional test
- razertool-gtk doesn't find its icon:
** (razertool-gtk:12930): WARNING **: Couldn't find pixmap file: razertool-icon.png

This is caused by moving the pixmaps in the spec file (the program will search the icon in the old place).

I would suggest the following:
- don't move %{_datadir}/%{name}/pixmaps, but package %{_datadir}/%{name} completly
- use the full path to the icon in the *.desktop file (That's OK according to http://fedoraproject.org/wiki/Packaging/Guidelines#Icon_tag_in_Desktop_Files)

Additionally I have one (optional) question: I've seen that libusb tries to open the files in /dev/bus/usb/*/*. On my system the user has no write permissions to any of these files (not even the one of the mouse). Is there a need to run this program as root or do you have to explicitly change the permissions of these files?

Spec URL:
http://fedora.mkdir.name/packages/razertool-0.0.7/razertool.spec

SRPM URL:
http://fedora.mkdir.name/packages/razertool-0.0.7/razertool-0.0.7-2.fc10.src.rpm

I fixed the pixmaps issue.
Concerning that question regarding access rights...
You are right, due to the permissions the tool has to be either run as root or a rule has to be added to udev.
As I'm no fan of the adding-a-rule scenario, I'm wondering how to realize a password request here. Any ideas?

With pam:

/etc/security/console.apps/razertool:
. config-util
PROGRAM=/usr/sbin/razertool
SESSION=true

In order to install razertool to /usr/sbin instead of /usr/bin, configure with
%configure --bindir=%{_sbindir}

/etc/pam.d/razertool:
#%PAM-1.0
auth            include         config-util
account         include         config-util
session         include         config-util

If you choose the pam approach, don't forget to require usermode-gtk.

I would however prefer not to run this tool as root. Are these per-user or global settings? Running as root is ok if it's golbal, but as downside a weakness in the code could be exploited with root privileges.
On the other hand I don't like the idea of making _all_ the usb device files writable. Does the program really need write access or is it just probing the devices? Or is there a special device for the razer mice? If so, a file in /etc/security/console.perms.d/ might suffice.

Well, I'm pretty unsure.
The risk when using pam is clear to me... and should be avoided.

<snip>
ACTION=="add", BUS=="usb", SYSFS{idVendor}=="1532", SYSFS{idProduct}=="0101", MODE="0660", GROUP="plugdev"
ACTION=="add", BUS=="usb", SYSFS{idVendor}=="1532", SYSFS{idProduct}=="0009", MODE="0660", GROUP="plugdev"
</snip>

This is the udev example rulefile.
As you can see, this rulefile restricts the permission changes to Razer-only devices. There is no special device for Razer mice.

The changes are rather hardware-side, as they are directly transmitted to the Razer (mouse) hardware.

In this case udev should IMO be preferred.

Then I shall go for this.
One last question remains though:
in the example udev case, the permissions are assigned to a group.
As on Fedora, the user is -- by default -- in no group, should permissions to write be set for everyone here (equaling 0666)?

(In reply to comment #2)
> Spec URL:
> http://fedora.mkdir.name/packages/razertool-0.0.7/razertool.spec
> 
> SRPM URL:
> http://fedora.mkdir.name/packages/razertool-0.0.7/razertool-0.0.7-2.fc10.src.rpm
> 
> I fixed the pixmaps issue.

Thanks! Yes, that fixed the problem.

A minor issue has leaked in with the latest fix:

Since you've used
%{_datadir}/%{name}/*
to package the files in /usr/share/razertool/*, only the contents of the directory but not the directory itself is packaged.
This lead to an directory which isn't owned by any package:

rpm -qf /usr/share/razertool
file /usr/share/razertool is not owned by any package

The fix is quite easy, just use
%{_datadir}/%{name}/
or
%{_datadir}/%{name}
to package the complete directory.

Just for completness: please also removed the unused line:
#rm -rf %{buildroot}%{_datadir}/%{name}/
from the spec file. ;-)

No correction of Christian's comment, just a more elaborate explanation for our new contributor:

(In reply to comment #7)
> %{_datadir}/%{name}/
> or
> %{_datadir}/%{name}

I would prefer %{_datadir}/%{name}/. It doesn't make a difference from the technical point of view, but another human that reads the spec will realize that it's a dir and not a file.

Spec URL:
http://fedora.mkdir.name/packages/razertool-0.0.7/razertool.spec

SRPM URL:
http://fedora.mkdir.name/packages/razertool-0.0.7/razertool-0.0.7-3.fc10.src.rpm

* Thu Apr 16 2009 Andreas Osowski <th0br0> - 0.0.7-3
- Code cleanup
- Permission fix
- Added the rules file for udev

Christoph Wickert is sponsoring me, FE-NEEDSPONSOR removed.

(In reply to comment #9)

I've looked at the newest package and it looks quite good besides the following enhancement of the permission management for the mouse's device file.

> - Added the rules file for udev  

Instead of the udev rule which sets the permissions of the device file to 0666 I would suggest the following:

Add a hal rule to /usr/share/hal/fdi/policy/10osvendor:

- let hal set for the specific vendor/device ID of the mice additional permissions for the local user (it is already done for mice for the /dev/input/event* device) for the "linux.device_file" (/dev/bus/usb/*/*).
- make sure the number is lower than 20 (which defines the acl actions)

I've attached a sample file which changes the permissions of linux's device file associated with a specific vendor and device id (which must be filled out).

Created attachment 340650 [details]
sample hal policy file

Hello,
first of all sorry for the bi~g delay.

I added the hal policy and removed the udev rule.

Spec URL:
http://fedora.mkdir.name/packages/razertool-0.0.7/razertool.spec

SRPM URL:
http://fedora.mkdir.name/packages/razertool-0.0.7/razertool-0.0.7-4.fc10.src.rpm

Hi Andreas,

Thanks for the new pacakge - we're very close to approval now. ;-) Unfortunately one minor issue appeared by adding the hal rule:

Since the package contains a file in /usr/share/hal/fdi/policy/10osvendor/ and this directory belongs to hal, razertool must require hal:

http://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership

Best regards,
Christian

Oh, that's true. I totally forgot about that requirement :)

Spec URL:
http://fedora.mkdir.name/packages/razertool-0.0.7/razertool.spec

SRPM URL:
http://fedora.mkdir.name/packages/razertool-0.0.7/razertool-0.0.7-5.fc10.src.rpm

Thanks for the update!

All open issues were addressed in the last package:

-> APPROVED

thanks a lot for your review


New Package CVS Request
=======================
Package Name: razertool
Short Description: Tool for controlling Razer Copperhead mice
Owners: th0br0
Branches: F-10 F-11
InitialCC:

cvs done.

razertool-0.0.7-5.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/razertool-0.0.7-5.fc10

razertool-0.0.7-5.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/razertool-0.0.7-5.fc11

razertool-0.0.7-5.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

razertool-0.0.7-5.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.