Bug 1225229 - Review Request: light-locker - A simple session-locker for lightdm
Summary: Review Request: light-locker - A simple session-locker for lightdm
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Rex Dieter
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Keywords:
Depends On: light-locker-settings
Blocks:
TreeView+ depends on / blocked
 
Reported: 2015-05-26 21:39 UTC by Raphael Groner
Modified: 2016-04-27 06:52 UTC (History)
3 users (show)

(edit)
Clone Of:
(edit)
Last Closed: 2015-08-13 16:56:24 UTC
rdieter: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)


External Trackers
Tracker ID Priority Status Summary Last Updated
Debian BTS 779749 None None None Never

Description Raphael Groner 2015-05-26 21:39:57 UTC
Spec URL: https://raphgro.fedorapeople.org/review/locker/light-locker.spec
SRPM URL: https://raphgro.fedorapeople.org/review/locker/light-locker-1.6.0-1.fc22.src.rpm
Description: A simple session-locker for lightdm 
Fedora Account System Username: raphgro

http://koji.fedoraproject.org/koji/taskinfo?taskID=9854416

Comment 1 Rex Dieter 2015-06-25 13:33:39 UTC
naming: ok

sources: ok
7f2425887d7bc8e86b53846f400d4ace  light-locker-1.6.0.tar.bz2

1. licensing: NOT ok

using licensecheck, it's clear all sources are GPLv2, so MUST use:
License: GPLv2+

2. scriptlets NOT ok, MUST add gsettings schema scriplets, see
https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#GSettings_Schema

3. SHOULD drop
desktop-file-validate %{buildroot}%{_sysconfdir}/xdg/autostart/*.desktop
no need to validate autostart, only stuff under /usr/share/applications/

4. MUST do verbose build (to show compiler flags), either use
%configure --disable-silent-rules
and/or
make ... V=1


As a practical matter, it worries my a little that it seems to conditionally autostart based on DE used (regardless if lightdm is active or not?).  this feels wrong to me, but I don't have any suggestions on how to improve that offhand.

Comment 2 Raphael Groner 2015-06-26 13:18:00 UTC
Spec URL: https://raphgro.fedorapeople.org/review/locker/light-locker.spec
SRPM URL: https://raphgro.fedorapeople.org/review/locker/light-locker-1.6.0-2.fc22.src.rpm

rawhide build: http://koji.fedoraproject.org/koji/taskinfo?taskID=10218012
epel7   build: http://koji.fedoraproject.org/koji/taskinfo?taskID=10218073

* Fri Jun 26 2015 Raphael Groner <> - 1.6.0-2
- fix license, enhance build options, add gsettings schema scriplets

(In reply to Rex Dieter from comment #1)
…
> 3. SHOULD drop
> desktop-file-validate %{buildroot}%{_sysconfdir}/xdg/autostart/*.desktop
> no need to validate autostart, only stuff under /usr/share/applications/

Well, I do that intentionally. There's an issue with NotShowIn and a patch provided, see also the upstream issue.

…
> As a practical matter, it worries my a little that it seems to conditionally
> autostart based on DE used (regardless if lightdm is active or not?).  this
> feels wrong to me, but I don't have any suggestions on how to improve that
> offhand.

Not sure what you mean. The DE should provide an on/off switch. What DE could be problematic in that matter?

Comment 3 Rex Dieter 2015-06-26 14:44:09 UTC
Thanks, APPROVED

Comment 4 Raphael Groner 2015-06-26 15:55:08 UTC
New Package SCM Request
=======================
Package Name: light-locker
Short Description: 
Upstream URL: https://github.com/the-cavalry/light-locker
Owners: raphgro
Branches: f22 f21 epel7
InitialCC:

Comment 5 Raphael Groner 2015-06-26 15:56:20 UTC
New Package SCM Request
=======================
Package Name: light-locker
Short Description: A simple session-locker for lightdm
Upstream URL: https://github.com/the-cavalry/light-locker
Owners: raphgro
Branches: f22 f21 epel7
InitialCC:

Comment 6 Gwyn Ciesla 2015-06-26 18:09:21 UTC
Git done (by process-git-requests).

Comment 7 Fedora Update System 2015-06-26 18:45:24 UTC
light-locker-1.6.0-2.fc22 has been submitted as an update for Fedora 22.
https://admin.fedoraproject.org/updates/light-locker-1.6.0-2.fc22

Comment 8 Fedora Update System 2015-06-26 18:57:55 UTC
light-locker-1.6.0-2.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/light-locker-1.6.0-2.fc21

Comment 9 Fedora Update System 2015-06-26 19:06:45 UTC
light-locker-1.6.0-2.el7 has been submitted as an update for Fedora EPEL 7.
https://admin.fedoraproject.org/updates/light-locker-1.6.0-2.el7

Comment 10 Fedora Update System 2015-06-27 12:39:55 UTC
light-locker-1.6.0-2.fc22 has been pushed to the Fedora 22 testing repository.

Comment 11 Christoph Wickert 2015-07-01 13:31:37 UTC
Raphael, your fixes for the desktop file don't seem correct:

1. First you fix NotShowIn and then you use sed to remove the key, so your patch becomes useless.

2. You will also need this patch: https://github.com/the-cavalry/light-locker/commit/1c23309c2f6a


Please also configure with

--enable-lock-on-suspend=on

instead of

--enable-lock-on-suspend=yes

because the Red Hat security response team wants machines to be locked after waking up from suspend (see bug 614608 and bug 632978). "on" means we do it by default whereas "yes" means just having the option to do it, but that requires changing the command line in the desktop file that starts light-locker.

Comment 12 Raphael Groner 2015-07-01 15:18:49 UTC
(In reply to Christoph Wickert from comment #11)
> Raphael, your fixes for the desktop file don't seem correct:
> 
> 1. First you fix NotShowIn and then you use sed to remove the key, so your
> patch becomes useless.

Yeah, this is currently b0rken, as mentioned in the comments.

> 2. You will also need this patch:
> https://github.com/the-cavalry/light-locker/commit/1c23309c2f6a

Thanks.

> Please also configure with
> 
> --enable-lock-on-suspend=on
> 
> instead of
> 
> --enable-lock-on-suspend=yes
> 
> because the Red Hat security response team wants machines to be locked after
> waking up from suspend (see bug 614608 and bug 632978). "on" means we do it
> by default whereas "yes" means just having the option to do it, but that
> requires changing the command line in the desktop file that starts
> light-locker.

Sure. Thanks a lot for these hints.

Next update will include both fixes.

Comment 13 Fedora Update System 2015-07-05 17:16:41 UTC
light-locker-1.6.0-3.fc22 has been submitted as an update for Fedora 22.
https://admin.fedoraproject.org/updates/light-locker-1.6.0-3.fc22

Comment 14 Fedora Update System 2015-07-05 18:21:57 UTC
light-locker-1.6.0-4.fc22 has been submitted as an update for Fedora 22.
https://admin.fedoraproject.org/updates/light-locker-1.6.0-4.fc22

Comment 15 Fedora Update System 2015-07-05 18:31:27 UTC
light-locker-1.6.0-4.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/light-locker-1.6.0-4.fc21

Comment 16 Fedora Update System 2015-07-05 18:38:27 UTC
light-locker-1.6.0-4.el7 has been submitted as an update for Fedora EPEL 7.
https://admin.fedoraproject.org/updates/light-locker-1.6.0-4.el7

Comment 17 Fedora Update System 2015-07-08 17:10:41 UTC
light-locker-1.6.0-4.el7 has been pushed to the Fedora EPEL 7 testing repository.

Comment 18 Raphael Groner 2015-07-18 07:56:50 UTC
bodhi - 2015-07-17 22:25:16 (karma: 0)
This update has reached 7 days in testing and can be pushed to stable now if the maintainer wishes

Well, it might be better to push with light-locker-settings in combination to improve the user experience. But still waiting for a reviewer in bug #1225231.

Comment 19 Raphael Groner 2015-07-23 06:34:04 UTC
Transition to systemd.

Is that Debian Bug (#779749) somehow relevant, also for Fedora?

Upstream issue: https://github.com/the-cavalry/light-locker/issues/64

Comment 20 Fedora Update System 2015-08-13 16:56:24 UTC
light-locker-1.6.0-4.fc21 has been pushed to the Fedora 21 stable repository.

Comment 21 Fedora Update System 2015-08-13 16:58:18 UTC
light-locker-1.6.0-4.fc22 has been pushed to the Fedora 22 stable repository.

Comment 22 Fedora Update System 2015-08-13 20:20:46 UTC
light-locker-1.6.0-4.el7 has been pushed to the Fedora EPEL 7 stable repository.

Comment 23 Raphael Groner 2016-04-27 06:52:55 UTC
Enable search for light-locker bugs.


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