Bug 222404 - Review Request: ConsoleKit - System daemon for tracking seats and sessions
Summary: Review Request: ConsoleKit - System daemon for tracking seats and sessions
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
Target Milestone: ---
Assignee: Ray Strode [halfline]
QA Contact: Fedora Extras Quality Assurance
Depends On:
TreeView+ depends on / blocked
Reported: 2007-01-12 04:00 UTC by Matthias Clasen
Modified: 2007-11-30 22:11 UTC (History)
2 users (show)

Clone Of:
Last Closed: 2007-01-12 20:38:12 UTC

Attachments (Terms of Use)

Description Matthias Clasen 2007-01-12 04:00:30 UTC
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
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.

Comment 1 Matthias Clasen 2007-01-12 04:19:14 UTC
I should have mentioned that still needs to go in core, since gdm will depend on it.

Comment 2 Ray Strode [halfline] 2007-01-12 04:23:40 UTC
the description is a little terse.  the words "seats" and "sessions" have a
specific undefined meaning for instance.

Comment 3 Ray Strode [halfline] 2007-01-12 04:25:05 UTC
in the last package i reviewed you said you didn't think having specific version
for the build requires was a good idea.

Comment 4 Ray Strode [halfline] 2007-01-12 04:26:20 UTC
If the tarball name is console-kit, why are we calling the package ConsoleKit ?
 why are we specifically passing -n to %setup?

Comment 5 Ray Strode [halfline] 2007-01-12 04:29:12 UTC
you need a Requires(preun): service i think

Comment 6 Ray Strode [halfline] 2007-01-12 04:37:27 UTC
do we really want to be shipping a binary that upstream explicitly made noinst?
 and giving it our own name nontheless?

Comment 7 Ray Strode [halfline] 2007-01-12 04:39:38 UTC
The init scripts have bogus priority numbers.  why is it getting started so late?

Comment 8 Matthias Clasen 2007-01-12 04:41:45 UTC
Re comment 2, from the README:

 What is a seat?

   8 ===============


  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.


  14 What is a session?

  15 ==================


  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

Comment 9 Ray Strode [halfline] 2007-01-12 04:47:54 UTC
can you put a complete url in the source line?

Comment 10 Matthias Clasen 2007-01-12 04:49:19 UTC
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

Re comment 7: I don't know. Why would you start it earlier ?

Re comment 9: sure, let me produce an updated spec

Comment 11 Ray Strode [halfline] 2007-01-12 04:51:12 UTC
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.

Comment 12 Matthias Clasen 2007-01-12 05:13:27 UTC
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 ?


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

Comment 13 Ray Strode [halfline] 2007-01-12 05:15:14 UTC
it guess is shouldn't hold up the review, but it's something we need to fix for
all three services.

Comment 14 Ray Strode [halfline] 2007-01-12 05:18:38 UTC
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.

Comment 15 Mamoru TASAKA 2007-01-12 06:00:19 UTC
* Commands under paths only used by previledged users (like
  /sbin/service) must be written with full path in scriptlets.

Comment 16 Matthias Clasen 2007-01-12 06:11:24 UTC
We should probably require dbus explicitly, for the /etc/dbus-1/system.d directory

Comment 17 Matthias Clasen 2007-01-12 13:31:31 UTC
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

Comment 18 Jesse Keating 2007-01-12 16:34:17 UTC
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

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.

Comment 19 Ray Strode [halfline] 2007-01-12 16:45:29 UTC
Note, despite its location


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.

Comment 20 Bill Nottingham 2007-01-12 16:53:54 UTC
The 'incoherent' messages are real errors. The lock file name in subsys *must*
match the script file name.

Comment 21 Matthias Clasen 2007-01-12 17:23:00 UTC
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

Comment 22 Jesse Keating 2007-01-12 19:29:22 UTC
Added to brew.  Close when built for rawhide.

Comment 23 Matthias Clasen 2007-01-12 20:38:12 UTC

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