From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43722) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gQOLa-00080T-LP for guix-patches@gnu.org; Fri, 23 Nov 2018 22:11:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gQOLW-00044I-Mz for guix-patches@gnu.org; Fri, 23 Nov 2018 22:11:06 -0500 Received: from debbugs.gnu.org ([208.118.235.43]:40448) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1gQOLW-00042A-He for guix-patches@gnu.org; Fri, 23 Nov 2018 22:11:02 -0500 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1gQOLW-0000TX-9J for guix-patches@gnu.org; Fri, 23 Nov 2018 22:11:02 -0500 Subject: [bug#33471] [PATCH] gnu: elogind: Update to 239.2. Resent-Message-ID: MIME-Version: 1.0 In-Reply-To: <871s7bockp.fsf@fastmail.com> References: <871s7bockp.fsf@fastmail.com> From: Stefan =?UTF-8?Q?Stefanovi=C4=87?= Date: Sat, 24 Nov 2018 04:10:36 +0100 Message-ID: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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: Marius Bakke Cc: Andy Wingo , 33471@debbugs.gnu.org On 11/23/18, Marius Bakke wrote: > Stefan Stefanovi=C4=87 writes: > >> Hello, >> Guix. > > Hello Stefan! > >> Elogind version 239.2 was released upstream, >> as I announced here is the Guix elogind package update. >> >> Sorry for the delay, >> it took me a while to reconfigure my system >> and test this release. It works, for me, as expected. > > Thank you very much for this work (again!). > > Overall the patch LGTM, some nitpicks: > >> From e220c336852ce6e5ae2c746fd70aacff18763cd9 Mon Sep 17 00:00:00 2001 >> From: =3D?UTF-8?q?Stefan=3D20Stefanovi=3DC4=3D87?=3D >> Date: Fri, 23 Nov 2018 04:47:02 +0100 >> Subject: [PATCH] gnu: elogind: Update to 239.2. >> >> * gnu/packages/freedesktop.scm (elogind): Update to 239.2. >> [version]: Use git-version. >> [source](method): Use git-fetch. >> [source](file-name): Use git-file-name. >> [source](patches): Remove elogind-glibc-2.27.patch. >> [source](snippet): >> Use substitute on meson.build file >> to clean RUNPATH and adjust pkttyagent path. >> Use substitute on man/meson.build file >> to build man pages without internet access. >> Use substitute on src/login/elogind.c file >> to change elogind.pid file path to tmpfs mount point. >> [arguments]<#:configure-flags>: Adjust build paths. >> [arguments]<#:make-flags>: Remove argument. >> [arguments]<#:phases>: >> [patch-locale-header]: Remove phase. >> [bootstrap]: Delete phase. >> [fix-service-file]: Remove phase. >> [add-libcap-to-search-path]: Remove phase. >> [remove-uaccess-tag]: Remove phase. >> [build-system]: Switch to meson-build-system. >> [native-inputs]: >> Remove: autoconf, automake, libtool, intltool. >> Add: docbook-xml-4.2, python-lxml. >> [inputs]: >> Remove: linux-libre-headers. >> Add: audit, glibc, libseccomp, libselinux, pcre2, >> python, util-linux. > > I'm surprised by the amount of new dependencies since I tried packaging > 236. Are all these actually required? Or do they enable optional > functionality? For the latter case I'd prefer a separate patch that > adds it. Some dependencies are optional, for example python, libselinux... I need to check the others I can not remember. > >> [synopsis]: Small adjustment. > > Can you align these with the left margin? The lines can be longer too. I will, but small example, taken as a partial modification of commit messag= e can be very helpful. > >> >> * gnu/packages/patches/elogind-glibc-2.27.patch: Delete file. >> * gnu/local.mk (dist_patch_DATA): Remove patch file. >> --- >> gnu/local.mk | 1 - >> gnu/packages/freedesktop.scm | 238 +++++++++--------- >> gnu/packages/patches/elogind-glibc-2.27.patch | 22 -- >> 3 files changed, 123 insertions(+), 138 deletions(-) >> delete mode 100644 gnu/packages/patches/elogind-glibc-2.27.patch >> >> diff --git a/gnu/local.mk b/gnu/local.mk >> index c56278e93..1b1511e54 100644 >> --- a/gnu/local.mk >> +++ b/gnu/local.mk >> @@ -659,7 +659,6 @@ dist_patch_DATA =3D \ >> %D%/packages/patches/dropbear-CVE-2018-15599.patch \ >> %D%/packages/patches/dvd+rw-tools-add-include.patch \ >> %D%/packages/patches/elfutils-tests-ptrace.patch \ >> - %D%/packages/patches/elogind-glibc-2.27.patch \ >> %D%/packages/patches/einstein-build.patch \ >> %D%/packages/patches/emacs-exec-path.patch \ >> %D%/packages/patches/emacs-fix-scheme-indent-function.patch \ >> diff --git a/gnu/packages/freedesktop.scm b/gnu/packages/freedesktop.scm >> index 5cc2699ad..14980b2ed 100644 >> --- a/gnu/packages/freedesktop.scm >> +++ b/gnu/packages/freedesktop.scm >> @@ -43,6 +43,7 @@ >> #:use-module (gnu packages acl) >> #:use-module (gnu packages admin) >> #:use-module (gnu packages autotools) >> + #:use-module (gnu packages base) >> #:use-module (gnu packages bash) >> #:use-module (gnu packages boost) >> #:use-module (gnu packages check) >> @@ -65,6 +66,7 @@ >> #:use-module (gnu packages libusb) >> #:use-module (gnu packages linux) >> #:use-module (gnu packages m4) >> + #:use-module (gnu packages pcre) >> #:use-module (gnu packages perl) >> #:use-module (gnu packages perl-check) >> #:use-module (gnu packages polkit) >> @@ -72,6 +74,7 @@ >> #:use-module (gnu packages perl) >> #:use-module (gnu packages perl-check) >> #:use-module (gnu packages python) >> + #:use-module (gnu packages selinux) >> #:use-module (gnu packages valgrind) >> #:use-module (gnu packages w3m) >> #:use-module (gnu packages web) >> @@ -227,124 +230,129 @@ the freedesktop.org XDG Base Directory >> specification.") >> (license license:expat))) >> >> (define-public elogind >> - (package >> - (name "elogind") >> - (version "232.4") >> - (source (origin >> - (method url-fetch) >> - (uri (string-append "https://github.com/elogind/elogind/" >> - "archive/v" version ".tar.gz")) >> - (file-name (string-append name "-" version ".tar.gz")) >> - (sha256 >> - (base32 >> - "1qcxian48z2dj5gfmp7brrngdydqf2jm00f4rjr5sy1myh8fy931")= ) >> - (patches (search-patches "elogind-glibc-2.27.patch")) >> - (modules '((guix build utils))) >> - (snippet >> - '(begin >> - (use-modules (guix build utils)) >> - (substitute* "Makefile.am" >> - ;; Avoid validation against DTD because the DTDs fo= r >> - ;; both doctype 4.2 and 4.5 are needed. >> - (("XSLTPROC_FLAGS =3D ") "XSLTPROC_FLAGS =3D >> --novalid")) >> - #t)))) >> - (build-system gnu-build-system) >> - (arguments >> - `(#:tests? #f ;FIXME: "make check" in the "po" directory fails. >> - #:configure-flags >> - (list (string-append "--with-udevrulesdir=3D" >> - (assoc-ref %outputs "out") >> - "/lib/udev/rules.d") >> - >> - ;; Let elogind be its own cgroup controller, rather than >> relying >> - ;; on systemd or OpenRC. By default, 'configure' makes an >> - ;; incorrect guess. >> - "--with-cgroup-controller=3Delogind" >> - >> - (string-append "--with-rootprefix=3D" >> - (assoc-ref %outputs "out")) >> - (string-append "--with-rootlibexecdir=3D" >> - (assoc-ref %outputs "out") >> - "/libexec/elogind") >> - ;; These are needed to ensure that lto linking works. >> - "RANLIB=3Dgcc-ranlib" >> - "AR=3Dgcc-ar" >> - "NM=3Dgcc-nm") >> - #:make-flags >> '("PKTTYAGENT=3D/run/current-system/profile/bin/pkttyagent") >> - #:phases >> - (modify-phases %standard-phases >> - (add-after 'unpack 'patch-locale-header >> - (lambda _ >> - ;; Fix compilation with glibc >=3D 2.26, which removed >> xlocale.h. >> - ;; This can be removed for elogind 234. >> - (substitute* "src/basic/parse-util.c" >> - (("xlocale\\.h") "locale.h")) >> - #t)) >> - (replace 'bootstrap >> - (lambda _ >> - (invoke "intltoolize" "--force" "--automake") >> - (invoke "autoreconf" "-vif"))) >> - (add-before 'build 'fix-service-file >> - (lambda* (#:key outputs #:allow-other-keys) >> - ;; Fix the file name of the 'elogind' binary in the D-Bus >> - ;; '.service' file. >> - (substitute* "src/login/org.freedesktop.login1.service" >> - (("^Exec=3D.*") >> - (string-append "Exec=3D" (assoc-ref %outputs "out") >> - "/libexec/elogind/elogind\n"))) >> - #t)) >> - (add-after 'install 'add-libcap-to-search-path >> - (lambda* (#:key inputs outputs #:allow-other-keys) >> - ;; Add a missing '-L' for libcap in libelogind.la. See >> - ;; >> . >> - (let ((libcap (assoc-ref inputs "libcap")) >> - (out (assoc-ref outputs "out"))) >> - (substitute* (string-append out "/lib/libelogind.la") >> - (("-lcap") >> - (string-append "-L" libcap "/lib -lcap"))) >> - #t))) >> - (add-after 'unpack 'remove-uaccess-tag >> - (lambda _ >> - ;; systemd supports a "uaccess" built-in tag, but eudev >> currently >> - ;; doesn't. This leads to eudev warnings that we'd rather >> not >> - ;; see, so remove the reference to "uaccess." >> - (substitute* "src/login/73-seat-late.rules.in" >> - (("^TAG=3D=3D\"uaccess\".*" line) >> - (string-append "# " line "\n"))) >> - #t))))) >> - (native-inputs >> - `(("autoconf" ,autoconf) >> - ("automake" ,automake) >> - ("libtool" ,libtool) >> - ("intltool" ,intltool) >> - ("gettext" ,gettext-minimal) >> - ("python" ,python) >> - ("docbook-xsl" ,docbook-xsl) >> - ("docbook-xml" ,docbook-xml) >> - ("xsltproc" ,libxslt) >> - ("m4" ,m4) >> - ("libxml2" ,libxml2) ;for XML_CATALOG_FILES >> - ("pkg-config" ,pkg-config) >> - >> - ;; Use gperf 3.0 to work around >> - ;; . >> - ("gperf" ,gperf-3.0))) >> - (inputs >> - `(("linux-pam" ,linux-pam) >> - ("linux-libre-headers" ,linux-libre-headers) >> - ("libcap" ,libcap) >> - ("shepherd" ,shepherd) ;for 'halt' and 'reboot', >> invoked >> - ;when pressing the power >> button >> - ("dbus" ,dbus) >> - ("eudev" ,eudev) >> - ("acl" ,acl))) ;to add individual users to ACLs on /de= v >> nodes >> - (home-page "https://github.com/elogind/elogind") >> - (synopsis "User, seat, and session management service") >> - (description "Elogind is the systemd project's \"logind\" service, >> + (let* ((commit "30c4221a9785a9e03ba37ece364ed8ff36d46480") >> + (revision "1") >> + (version (git-version "239.2" revision commit))) >> + (package >> + (name "elogind") >> + (version version) >> + (source (origin >> + (method git-fetch) >> + (uri (git-reference >> + (url "https://github.com/elogind/elogind") >> + (commit commit))) >> + (file-name (git-file-name name version)) >> + (sha256 >> + (base32 >> + >> "17khwbzqmkfd3hcscs51kzdpvq9p2llm08vbpsdhy9yxgwfzlfa6")) > > Can you split this into two patches: one that switches the current > elogind package to use "git-fetch", and afterwards this update? > > Please also remove the let binding so that we can keep the original > indentation. Those bindings are usually only necessary for packages > that don't have a proper versioning scheme, unlike elogind. > >> + (modules '((guix build utils))) >> + (snippet >> + '(begin >> + (use-modules (guix build utils)) >> + (substitute* "meson.build" >> + ;; Clean RUNPATH. >> + (("install_rpath :") "#install_rpath :") >> + ;; TODO: Remove $ORIGIN from RUNPATH >> + ;; of libexec executables. > > I think meson-build-system on 'core-updates' does this, if it does not > already (doesn't ld-wrapper ignore it?). Having $ORIGIN there should > be harmless though, no? I was not sure about this so I wrote this comment, will remove the TODO com= ment. The above substitute should remain, because without it the RUNPATH is filled with some strange artifacts, paths with 'XXXX...' strings. > >> + ;; Fix pkttyagent path: >> + (("join_paths\\(bindir, 'pkttyagent'\\)") >> + >> "'\"/run/current-system/profile/bin/pkttyagent\"'")) > > Please do this substitution in a phase, since it's very Guix-specific. > >> + (substitute* "man/meson.build" >> + ;; Necessary because >> + ;; there is no internet access >> + ;; inside the build environment. >> + (("xsltproc_flags =3D \\[") >> + (string-append "xsltproc_flags =3D [" >> + " '--novalid',"))) > > Please retain the original comment for this substitution. I will. > > Actually, I see in my WIP branch that this is rendered unnecessary by > adding "docbook-xsl@4.2" as a native-input. Did you try removing it? > >> + (substitute* "src/login/elogind.c" >> + ;; Change PID file path >> + ;; to fmpfs mount point. >> + (("\"/run/elogind.pid\"") >> + "\"/run/systemd/elogind.pid\"")) > > Is it possible to adjust elogind-service instead of making this > substitution? This is also better to do in a phase. Elogind, assumed that 'elogind.pid' file is in tmpfs RAM filesystem, it has more robust check now but this check is not tested. '/run' is not mounted as tmpfs, unlike '/run/systemd'. Is it feasible to make '/run' tmpfs mount point? If we decide not to place 'elogind.pid' file in tmpfs, we should at least change it to '/var/run/...' just to be consistent. In any case I will move this in phase. > >> + #t)))) >> + (build-system meson-build-system) >> + (outputs '("out")) > > '("out") is the default, so this line can be removed. I will, I tried to be more explicit. This is my old habit, sorry. > >> + (arguments >> + `(#:tests? #t >> + ;; Some tests fail only in chroot build environment: >> + ;; - https://github.com/elogind/elogind/issues/45 >> + ;; Some tests assume existence of standard directories: >> + ;; *** test_copy_bytes FAILS because there is no >> + ;; /usr/lib/os-release or /etc/os-release file >> + ;; *** test_chase_symlinks FAILS because there is no >> + ;; /usr directory > > Since we don't currently run the tests, I think we can skip this comment > and instead work on a followup patch that enables the tests. I will remove this comment. I was planing on working on those failing tests after this release. > >> + ;; >> + #:configure-flags >> + (let* ((out (assoc-ref %outputs "out")) >> + (sysconf (string-append out "/etc")) >> + (libexec (string-append out "/libexec/elogind")) >> + (dbuspolicy (string-append out "/etc/dbus-1/system.d")) >> + (shepherd (assoc-ref %build-inputs "shepherd")) >> + (halt-path (string-append shepherd "/sbin/halt")) >> + (kexec-path "") ;; NOTE: We need to package kexec-tools= , >> + ;; or support kexec with shepherd= . >> + (poweroff-path (string-append shepherd >> "/sbin/shutdown")) >> + (reboot-path (string-append shepherd "/sbin/reboot"))) >> + `(,(string-append >> + "-Drootprefix=3D" out) > > Please use (list ...) here instead of quote/unquote. Will do. I am already contradicting my stated desire to be more explicit. := ) > >> + ,(string-append >> + "-Dsysconfdir=3D" sysconf) >> + ,(string-append >> + "-Drootlibexecdir=3D" libexec) >> + ,(string-append >> + "-Ddbuspolicydir=3D" dbuspolicy) >> + ,(string-append >> + "-Dc_link_args=3D-Wl,-rpath=3D" libexec) >> + ,(string-append >> + "-Dcpp_link_args=3D-Wl,-rpath=3D" libexec) >> + ,(string-append >> + "-Dhalt-path=3D" halt-path) >> + ,(string-append >> + "-Dkexec-path=3D" kexec-path) >> + ,(string-append >> + "-Dpoweroff-path=3D" poweroff-path) >> + ,(string-append >> + "-Dreboot-path=3D" reboot-path) >> + "-Dcgroup-controller=3Delogind" >> + ;; Disable some tests. >> + "-Dtests=3Dfalse" >> + "-Dslow-tests=3Dfalse")) >> + #:phases >> + (modify-phases %standard-phases >> + (delete 'bootstrap)))) > > Can you add a comment about why this is necessary? I will. I need to check, the standard meson phases. Maybe we do not need it, I wrote this a few weeks ago. :( > >> + (native-inputs >> + `(("docbook-xsl" ,docbook-xsl) >> + ("docbook-xml-4.2" ,docbook-xml-4.2) >> + ("docbook-xml-4.5" ,docbook-xml) >> + ("libxml2" ,libxml2) >> + ("m4" ,m4) >> + ("pkg-config" ,pkg-config) >> + ("python" ,python) >> + ("python-lxml" ,python-lxml) >> + ("gettext" ,gettext-minimal) >> + ("gperf" ,gperf) >> + ("xsltproc" ,libxslt))) >> + (inputs >> + `(("acl" ,acl) >> + ("audit" ,audit) >> + ("dbus" ,dbus) >> + ("eudev" ,eudev) >> + ("glibc" ,glibc) > > glibc is already an input, so this can be dropped. Ok, will be removed. > > The rest of the patch LGTM, though I admit that I did not test it yet. > > Please also make sure "make check-system TESTS=3Delogind" passes. I foun= d > I had to s|/dev/tty1|tty1| in gnu/tests/desktop.scm in my branch. I will try that. > > Can you send updates patches? Thanks in advance! I will start working on them right away. Thank you, for your suggestions. Stefan.