[bug#27083] screen-lockers: i3lock-color and i3lock-fancy

[ng0 <ng0@n0.is>]

[-- Attachment #1: Type: text/plain, Size: 11282 bytes --]

Hi Chris,

thanks for your review. I'll have no time to address it before next week,
just a couple of comments.

Hi meskarune, I've CC'd you because of a specific question/idea after the
4th quoted paragraph.

Chris Marusich transcribed 9.7K bytes:
> Hi ng0,
> 
> I'm glad to hear i3lock works for you, too!
> 
> I was able to find time to draft feedback for your latest i3lock-color
> patch, but not for i3lock-fancy (although it looks very simple, and I
> will check it in the next few days).  In the interest of not moving too
> slowly, I'm sending what I have to you now.
> 
> ng0 <ng0@infotropique.org> writes:
> 
> > I've changed my mind. As there's also 'i3lock' (without the -color suffix)
> > but we don't package it (yet), it would take away the freedom to
> > decide which i3lock you want to use.
> > The author of i3lock-fancy wantsus to use i3lock-color, but there's
> > also a check and it falls back to the normal i3lock.
> >
> > Therefore I think it must be up to people using i3lock-fancy to
> > install i3lock-color AND i3lock-fancy in their system profile
> > (or just i3lock-color in systemprofile + suid it, and i3lock-fancy
> > in the user profile).
> 
> I agree with your assessment.  For now, I think that's a fine plan,
> although I dislike the fact that by adding the i3lock-fancy package by
> itself, we will make it possible for someone to naively install just
> that package to their profile, only to find that it doesn't work because
> i3lock itself is not installed in the system as a screen locker program.
> However, I can't think of a better solution at this time, and we're
> already doing something similar for the other screen locker packages.
> For example, if you install xscreensaver into your profile without also
> installing it into the system as a screen locker service, xscreensaver
> won't work because it won't be setuid-root.  So I think it's OK.

We'll continue to have these problems with applications, maybe we should
consider it a bug and find a common description. So far it's limited to
screensavers, but I can't think of a reason why this problem would stay
limited to applications of the screensaver class.

For this specific application (i3lock-fancy), I could ask meskarune if she'd like to add
an error message if none is place already. At the top of the script
check for the existence of the i3lock binary in the users path (simply
'i3lock', not (only) total paths). If not found, throw an error and abort
with a message that "you need to install i3lock-color".

> >> > > > > > -;;; Copyright (c) 2016, 2017 ng0 <contact.ng0@cryptolab.net>
> >> > > > > > +;;; Copyright (c) 2016, 2017 ng0 <ng0@no-reply.pragmatique.xyz>
> >> > > > > 
> >> > > > > FYI, the contact.ng0@cryptolab.net email address is still mentioned in
> >> > > > > multiple other files, too.
> >> > > > 
> >> > > > Yes. I'm only changing addresses once I touch the file. So old
> >> > > > addresses might be in there for a long time. Otoh, I will
> >> > > > prepare a patch and change all my header addresses.
> 
> Understood.
> 
> >> > > > > > +    (description
> >> > > > > > +     "I3lock-color is a screen locker.  It is a re-patched version of
> >> > > > > > +i3lock, which is a simple screen locker like slock.  Features include:
> >> > > > > > +
> >> > > > > > +@enumerate
> >> > > > > > +@item forking process, the locked screen is preserved when you suspend from RAM
> >> > > > > > +@item specify background color or PNG image to be displayed in the lock screen
> >> > > > > > +@item many additional color options
> >> > > > > > +@end enumerate\n")
> >> > > > > 
> >> > > > > I'm not sure if the trailing newline is necessary.  Unless you beat me
> >> > > > > to it, I'll check on this detail the next time I return to this thread.
> >> > > > 
> >> > > > I've seen this multiple times before and have just followed this style.
> 
> I looked into this, and it seems that newlines are elided from the
> output.  Check "guix package --show=maxflow" and "guix edit maxflow" to
> see a particularly striking example.

Oh, thanks.

> > diff --git a/gnu/local.mk b/gnu/local.mk
> > index 54d1ac91c..2ec5844dc 100644
> > --- a/gnu/local.mk
> > +++ b/gnu/local.mk
> > @@ -744,6 +744,7 @@ dist_patch_DATA =						\
> >    %D%/packages/patches/hubbub-sort-entities.patch		\
> >    %D%/packages/patches/hurd-fix-eth-multiplexer-dependency.patch        \
> >    %D%/packages/patches/hydra-disable-darcs-test.patch		\
> > +  %D%/packages/patches/i3lock-color-fix-makefile.patch          \
> >    %D%/packages/patches/icecat-avoid-bundled-libraries.patch	\
> >    %D%/packages/patches/icecat-bug-1348660-pt5.patch		\
> >    %D%/packages/patches/icecat-bug-1415133.patch			\
> 
> Now that the Makefile is patched using substitute*, we can remove this
> added line from gnu/local.mk.

Oops, right.

> > diff --git a/gnu/packages/wm.scm b/gnu/packages/wm.scm
> > index 62a5b5460..e4db72a6f 100644
> > --- a/gnu/packages/wm.scm
> > +++ b/gnu/packages/wm.scm
> > @@ -68,6 +68,7 @@
> >    #:use-module (gnu packages gperf)
> >    #:use-module (gnu packages imagemagick)
> >    #:use-module (gnu packages lua)
> > +  #:use-module (gnu packages linux)
> >    #:use-module (gnu packages suckless)
> >    #:use-module (guix download)
> >    #:use-module (guix git-download))
> > @@ -335,6 +336,73 @@ and locate windows on all your workspaces, using an interactive dmenu
> >  prompt.")
> >        (license (license:non-copyleft "http://www.wtfpl.net/txt/copying/")))))
> >  
> > +(define-public i3lock-color
> > +  (package
> > +    (name "i3lock-color")
> > +    (version "2.9.1-c")
> 
> According to "guix lint", version 2.10.1-c is now available.  Should we
> use it?

Yes. I just don't track every single release made all the time ;)
So thanks for notifying me of this.

> > +    (source
> > +     (origin
> > +       (method url-fetch)
> > +       (uri (string-append "https://github.com/chrjguill/i3lock-color/"
> > +                           "archive/" version ".tar.gz"))
> > +       (file-name (string-append name "-" version ".tar.gz"))
> > +       (sha256
> > +        (base32
> > +         "18ql0kb24qlqsijs6j99algf2lprl78mzsz53b1hshmc8jjd4m27"))))
> > +    (build-system gnu-build-system)
> > +    (arguments
> > +     `(#:tests? #f ; No tests included.
> > +       #:make-flags (list "CC=gcc")
> 
> Why is this #:make-flags argument necessary?  I'm not suggesting it
> isn't; I just don't know whether it is or not, and I'm curious.

I don't remember. I'll check it out.

> > +       #:phases
> > +       (modify-phases %standard-phases
> > +         (replace 'configure
> > +           (lambda* (#:key outputs inputs #:allow-other-keys)
> > +             (let* ((out (assoc-ref outputs "out"))
> > +                    (etc (string-append out "/etc"))
> > +                    (man (string-append out "/share/man"))
> > +                    (xkb (assoc-ref inputs "libxkbcommon"))
> > +                    (xkbheader (string-append xkb
> > +                                              "/include/xkbcommon/"
> > +                                              "xkbcommon-compose.h")))
> > +               (substitute* "Makefile"
> > +                 (("PKG_CONFIG=pkg-config")
> > +                  (string-append "PKG_CONFIG="
> > +                                 (which "pkg-config")))
> > +                 (("/usr/include/xkbcommon/xkbcommon-compose.h")
> > +                  xkbheader)
> > +                 (("PREFIX=/usr")
> > +                  (string-append "PREFIX=" out))
> > +                 (("SYSCONFDIR=/etc")
> > +                  (string-append "SYSCONFDIR=" etc))
> > +                 (("MANDIR=/usr/share/man")
> > +                  (string-append "MANDIR=" man)))
> > +               #t))))))
> 
> I like that we were able to remove the patch file by using substitute*!
> Hopefully once you upstream your changes, we can simplify this further.

Well I don't have to upstream exactly what we have but by writing the
Makefile in a more flexible way (and by removing the one total path).

> > +    (inputs
> > +     `(("xcb-util-image" ,xcb-util-image)
> > +       ("xcb-util-keysyms" ,xcb-util-keysyms)
> > +       ("xcb-util" ,xcb-util)
> > +       ("libxcb" ,libxcb)
> > +       ("linux-pam" ,linux-pam)
> > +       ("libev" ,libev)
> > +       ("libx11" ,libx11)
> > +       ("cairo" ,cairo)))
> > +    (native-inputs
> > +     `(("libxkbcommon" ,libxkbcommon)
> > +       ("pkg-config" ,pkg-config)
> > +       ("which" ,which)))
> 
> i3lock-color retains the following references when built:
> 
> --8<---------------cut here---------------start------------->8---
> $ guix gc --references /gnu/store/kfymvwgm4g0avs3b97micq8477sa0if6-i3lock-color-2.9.1-c | sort -t - -k 2 -k 1,1
> /gnu/store/nwxv9s2q8pi0m6gn6fyidpj8442dwp6f-cairo-1.14.10
> /gnu/store/6wyjls0q2c9gjskkplsr1ad09p3d8gzg-gcc-5.4.0-lib
> /gnu/store/3h31zsqxjjg52da5gp3qmhkh4x8klhah-glibc-2.25
> /gnu/store/8zyry9663xgcrgg7fkrdrw40511d1gyz-libev-4.24
> /gnu/store/3cxhfkh0n1naan9db0z302mwqpxqkry6-libxcb-1.12
> /gnu/store/vyip2r21g65q90jy79nm5gsz6yl1s7gp-libxkbcommon-0.7.1
> /gnu/store/jf7sby087sakbsirdxrfi5cizf3ya4md-linux-pam-1.3.0
> /gnu/store/f4y2v0ynyr1rhzjxv1m6xlglgg0grxll-xcb-util-0.4.0
> /gnu/store/x67589ylz75jadmhab8z6z03f1i5l5rv-xcb-util-image-0.4.0
> --8<---------------cut here---------------end--------------->8---
> 
> Specifically, libxkbcommon is referenced.  Therefore, it might be a
> runtime dependency of the program.  Are you sure it should be a
> native-input?  If it really is required at runtime, it should be an
> input, not a native-input.
> 
> In addition, xcb-util-keysyms and libx11 are both unreferenced.  Does
> i3lock-color build correctly without them?  If so, can we just remove
> them?  If not, should they be native-inputs?
> 
> > +    (synopsis "Screenlocker with color configuration support")
> 
> Could you change this phrase to "screen locker" instead of
> "screenlocker"?  That is what you use in the description, and it's also
> what we use in the synopses/descriptions of the other screen lockers
> we've already packaged (and the manual).  The consistency might make
> this package more discoverable.
> 
> > i3lock-color should be removed before commiting this patch,
> > and we should add a note about this to the description. Like:
> > "You will need to install i3lock or one of its variants (like
> > i3lock-color) to make use of i3lock-fancy."
> 
> I don't understand: why do we need to remove i3lock-color?

i3lock-fancy is Bash script. I'm patching in the references it
should keep. The 2 calls of i3lock in there must be just 'i3lock',
this will call the binary with the suid, not for example the one
in the store path of i3lock-color (which never worked).
More correct would be this remark:
i3lock-color should be removed from the inputs before commiting
this patch, and we should add a note about this to the description.

> -- 
> Chris



-- 
GnuPG: A88C8ADD129828D7EAC02E52E22F9BBFEE348588
GnuPG: https://c.n0.is/ng0_pubkeys/tree/keys
  WWW: https://n0.is

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]