This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 222404 - Review Request: ConsoleKit - System daemon for tracking seats and sessions
Review Request: ConsoleKit - System daemon for tracking seats and sessions
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Ray Strode [halfline]
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FC-ACCEPT
  Show dependency treegraph
 
Reported: 2007-01-11 23:00 EST by Matthias Clasen
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-12 15:38:12 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 Matthias Clasen 2007-01-11 23:00:30 EST
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.
Comment 1 Matthias Clasen 2007-01-11 23:19:14 EST
I should have mentioned that still needs to go in core, since gdm will depend on it.
Comment 2 Ray Strode [halfline] 2007-01-11 23:23:40 EST
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-11 23:25:05 EST
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-11 23:26:20 EST
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-11 23:29:12 EST
you need a Requires(preun): service i think
Comment 6 Ray Strode [halfline] 2007-01-11 23:37:27 EST
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-11 23:39:38 EST
The init scripts have bogus priority numbers.  why is it getting started so late?
Comment 8 Matthias Clasen 2007-01-11 23:41:45 EST
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.
Comment 9 Ray Strode [halfline] 2007-01-11 23:47:54 EST
can you put a complete url in the source line?
Comment 10 Matthias Clasen 2007-01-11 23:49:19 EST
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
Comment 11 Ray Strode [halfline] 2007-01-11 23:51:12 EST
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 00:13:27 EST
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
Comment 13 Ray Strode [halfline] 2007-01-12 00:15:14 EST
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 00:18:38 EST
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 01:00:19 EST
* 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 01:11:24 EST
We should probably require dbus explicitly, for the /etc/dbus-1/system.d directory
Comment 17 Matthias Clasen 2007-01-12 08:31:31 EST
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 11:34:17 EST
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.
Comment 19 Ray Strode [halfline] 2007-01-12 11:45:29 EST
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.

Comment 20 Bill Nottingham 2007-01-12 11:53:54 EST
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 12:23:00 EST
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 14:29:22 EST
Added to brew.  Close when built for rawhide.
Comment 23 Matthias Clasen 2007-01-12 15:38:12 EST
built

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