Spec URL: http://people.redhat.com/mclasen/ConsoleKit/ConsoleKit.spec SRPM URL: http://people.redhat.com/mclasen/ConsoleKit/ConsoleKit-0.1.0-1.src.rpm Description: ConsoleKit is a system daemon for tracking seats and sessions. It provides asynchronous notification via the system message bus. ConsoleKit is a central part of the effort to make fast user switching work well for FC7.
I should have mentioned that still needs to go in core, since gdm will depend on it.
the description is a little terse. the words "seats" and "sessions" have a specific undefined meaning for instance.
in the last package i reviewed you said you didn't think having specific version for the build requires was a good idea.
If the tarball name is console-kit, why are we calling the package ConsoleKit ? why are we specifically passing -n to %setup?
you need a Requires(preun): service i think
do we really want to be shipping a binary that upstream explicitly made noinst? and giving it our own name nontheless?
The init scripts have bogus priority numbers. why is it getting started so late?
Re comment 2, from the README: What is a seat? 8 =============== 9 10 A seat is a collection of sessions and a set of hardware (usually at 11 least a keyboard and mouse). Only one session may be active on a 12 seat at a time. 13 14 What is a session? 15 ================== 16 17 A session is a collection of all processes that originate from a single 18 common ancestor and retain knowledge of a secret. As an implementation 19 detail, this secret may be stored in the process environment by the 20 login manager under the name XDG_SESSION_COOKIE. That reminds me that I should include the README ChangeLog, etc in the rpm Re comment 3: True. This spec was stolen from davidz... Re comment 4: I don't know. This is the way William Jon McCann shipped it. The project itself is clearly called ConsoleKit (see e.g. the README), but the tarball is named console-kit. David decided to go with the project name for the rpm, which is explicitly allowed by the package naming guidelines. Re comment 5: /sbin/service is part of initscripts, which is on the exceptions list.
can you put a complete url in the source line?
Re comment 6: good point, and I guess the answer is no. It is certainly not essential functionality, since it is just a wrapper of sorts around the dbus interface. Re comment 7: I don't know. Why would you start it earlier ? Re comment 9: sure, let me produce an updated spec
ConsoleKit seems sort of low level. If we start it at the end, it means no other services will be able to use it... We made this same mistake with hal and networkmanager for what its worth. We shouldn't repeat it again.
Well, its currently started at the same time as hal and NetworkManager... Do we really want to deviate from upstream for that, without a concrete problem that would be solved by starting it earlier ? Updated: Spec URL: http://people.redhat.com/mclasen/ConsoleKit/ConsoleKit.spec SRPM URL: http://people.redhat.com/mclasen/ConsoleKit/ConsoleKit-0.1.0-2.src.rpm
it guess is shouldn't hold up the review, but it's something we need to fix for all three services.
We should fix the description to say what consolekit does in primitive terms. But with that change and assuming this passes rpmlint and succeeds in mock, it seems like it's good to go.
* Commands under paths only used by previledged users (like /sbin/service) must be written with full path in scriptlets.
We should probably require dbus explicitly, for the /etc/dbus-1/system.d directory
New versions including the last 3 comments: Spec URL: http://people.redhat.com/mclasen/ConsoleKit/ConsoleKit.spec SRPM URL: http://people.redhat.com/mclasen/ConsoleKit/ConsoleKit-0.1.0-3.src.rpm
rpmlint has some interesting things to say: W: ConsoleKit conffile-without-noreplace-flag /etc/rc.d/init.d/ConsoleKit Are these really config files? W: ConsoleKit non-conffile-in-etc /etc/dbus-1/system.d/console-kit.conf This WOULD be a config, and should get (noreplace) maybe? E: ConsoleKit executable-marked-as-config-file /etc/rc.d/init.d/ConsoleKit Another dupe of above. W: ConsoleKit service-default-enabled /etc/rc.d/init.d/ConsoleKit I think this is fine yes? We _always_ want ConsoleKit running? W: ConsoleKit incoherent-subsys /etc/rc.d/init.d/ConsoleKit $servicename W: ConsoleKit incoherent-subsys /etc/rc.d/init.d/ConsoleKit $servicename W: ConsoleKit incoherent-subsys /etc/rc.d/init.d/ConsoleKit $servicename This is because "ConsoleKit" != the binary name. I think thats OK in this case right? W: ConsoleKit no-reload-entry /etc/rc.d/init.d/ConsoleKit Does 'reload' vs restart make any sense in this daemon? W: ConsoleKit incoherent-init-script-name ConsoleKit Dupe from above.
Note, despite its location /etc/dbus-1/system.d doesn't really house config files so much as policy files. i.e., it would be very rare for a sysadmin to want to change anything in that directory.
The 'incoherent' messages are real errors. The lock file name in subsys *must* match the script file name.
New versions including the last 3 comments: Spec URL: http://people.redhat.com/mclasen/ConsoleKit/ConsoleKit.spec SRPM URL: http://people.redhat.com/mclasen/ConsoleKit/ConsoleKit-0.1.0-4.src.rpm
Added to brew. Close when built for rawhide.
built