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

[Chris Marusich]

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 <address@hidden> 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.

>> > > > > > -;;; Copyright (c) 2016, 2017 ng0 <address@hidden>
>> > > > > > +;;; Copyright (c) 2016, 2017 ng0 <address@hidden>
>> > > > > 
>> > > > > FYI, the address@hidden 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:
>> > > > > > +
>> > > > > > address@hidden
>> > > > > > address@hidden forking process, the locked screen is preserved 
>> > > > > > when you suspend from RAM
>> > > > > > address@hidden specify background color or PNG image to be 
>> > > > > > displayed in the lock screen
>> > > > > > address@hidden many additional color options
>> > > > > > address@hidden 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.

> 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.

> 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?

> +    (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.

> +       #: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.

> +    (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?

-- 
Chris

signature.asc
Description: PGP signature