From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39323) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eKhc6-0005Xc-9t for guix-patches@gnu.org; Fri, 01 Dec 2017 04:28:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1eKhc2-0007dj-Rf for guix-patches@gnu.org; Fri, 01 Dec 2017 04:28:06 -0500 Received: from debbugs.gnu.org ([208.118.235.43]:58555) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1eKhc2-0007dF-Lq for guix-patches@gnu.org; Fri, 01 Dec 2017 04:28:02 -0500 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1eKhc2-0005aT-DN for guix-patches@gnu.org; Fri, 01 Dec 2017 04:28:02 -0500 Subject: [bug#27083] screen-lockers: i3lock-color and i3lock-fancy Resent-Message-ID: Date: Fri, 1 Dec 2017 09:27:31 +0000 From: ng0 Message-ID: <20171201092731.aea3sdto6dgsqisk@abyayala> References: <871skx9ql3.fsf@gmail.com> <20171117101835.whd5f7lincppxlgy@abyayala> <20171117201705.75ldk4yxnpd7m3qa@abyayala> <20171117205734.63tbjhxqbcw6sis7@abyayala> <20171117210211.vdauzbvhrjubwamt@abyayala> <20171117211901.eor5yjlvj3icgbsd@abyayala> <87374uc3tk.fsf@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="k2kcdiwqqyskm37e" Content-Disposition: inline In-Reply-To: <87374uc3tk.fsf@gmail.com> List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+kyle=kyleam.com@gnu.org Sender: "Guix-patches" To: Chris Marusich Cc: 27083@debbugs.gnu.org, admin@doloresportalatin.info --k2kcdiwqqyskm37e Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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, >=20 > I'm glad to hear i3lock works for you, too! >=20 > 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. >=20 > ng0 writes: >=20 > > I've changed my mind. As there's also 'i3lock' (without the -color suff= ix) > > 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). >=20 > 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 > >> > > > > > +;;; Copyright (c) 2016, 2017 ng0 > >> > > > >=20 > >> > > > > FYI, the contact.ng0@cryptolab.net email address is still ment= ioned in > >> > > > > multiple other files, too. > >> > > >=20 > >> > > > 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. >=20 > Understood. >=20 > >> > > > > > + (description > >> > > > > > + "I3lock-color is a screen locker. It is a re-patched = version of > >> > > > > > +i3lock, which is a simple screen locker like slock. Featur= es 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") > >> > > > >=20 > >> > > > > 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 thi= s thread. > >> > > >=20 > >> > > > I've seen this multiple times before and have just followed this= style. >=20 > I looked into this, and it seems that newlines are elided from the > output. Check "guix package --show=3Dmaxflow" 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 =3D \ > > %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 \ >=20 > 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 a= n interactive dmenu > > prompt.") > > (license (license:non-copyleft "http://www.wtfpl.net/txt/copying= /"))))) > > =20 > > +(define-public i3lock-color > > + (package > > + (name "i3lock-color") > > + (version "2.9.1-c") >=20 > 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=3Dgcc") >=20 > 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=3Dpkg-config") > > + (string-append "PKG_CONFIG=3D" > > + (which "pkg-config"))) > > + (("/usr/include/xkbcommon/xkbcommon-compose.h") > > + xkbheader) > > + (("PREFIX=3D/usr") > > + (string-append "PREFIX=3D" out)) > > + (("SYSCONFDIR=3D/etc") > > + (string-append "SYSCONFDIR=3D" etc)) > > + (("MANDIR=3D/usr/share/man") > > + (string-append "MANDIR=3D" man))) > > + #t)))))) >=20 > 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))) >=20 > i3lock-color retains the following references when built: >=20 > --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--- >=20 > 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. >=20 > 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? >=20 > > + (synopsis "Screenlocker with color configuration support") >=20 > 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. >=20 > > 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." >=20 > 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. > --=20 > Chris --=20 GnuPG: A88C8ADD129828D7EAC02E52E22F9BBFEE348588 GnuPG: https://c.n0.is/ng0_pubkeys/tree/keys WWW: https://n0.is --k2kcdiwqqyskm37e Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCgAdFiEEqIyK3RKYKNfqwC5S4i+bv+40hYgFAlohIIIACgkQ4i+bv+40 hYjnug/8C6sF2u8rXGMBg1zdOsE4Yz9EzyfTlYCnxANtAsXEdR9YabYoobwJgmFg LuhvAyN/Tvx00c9dBXsG+fTeRMj2vXXgswZvRhmCJ+FoZ07a1vsQkdTMtIq5sVCN QWpE3cDjkkXbLMblGVG00FVO/tyg8K73AL77zGcLhsl+wW3ixeWy2X8nIzOSUZsI 4rufse3PxVFI1A/4R2fxBmQUDHNsLcztwvV4CPrrQJqjO0feCuBhqKL9LH4rMhrj PIzqe91fU2+D7h3Nb/aM58jKU5Gc2o0J3aH1/nROb9Sslc0r7jGzSeEpJboREYOI DNSp2dRRlmA51ez1JK0AnKSAaEmNNP8Rq7CMlD9t2TpFLUnHake61bSL72VTi1FW fLwkt8KaIUJTveQCt9i4gS04dqmDaRpCH+EKm5BkQUwI8uAaXDOKsnPzSuJ5K2De Zf6u79ElMXDQvkwTEWUhNI74PN1coUvHtNtxGaB8jQXAh/SiztJ/ki2AjsISs4Qq d8alBPcqVuvpn9FHkdE7O0BzAik9+cxCy9nlPP9MWcUkyocccPYQxH6T+C7MxVT0 IZNo3yeUvOi5yLOG4zxIgcAAMcXhz8ezDDuToUOJh7o5edUGKDvYPzCXGs4OrCGL btMYBduL2cLhLecZLcJtii6hnVY8inRGT6i8Ix9KQFgBZ9ZGAMM= =AHqR -----END PGP SIGNATURE----- --k2kcdiwqqyskm37e--