Stefan Stefanovi? <stefanx2ovic@gmail.com> writes:
Toggle quote (3 lines)
> Hello,
> Guix.
Toggle quote (7 lines)
> 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:
Toggle quote (34 lines)
> From e220c336852ce6e5ae2c746fd70aacff18763cd9 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Stefan=20Stefanovi=C4=87?= <stefanx2ovic@gmail.com>
> 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.
Toggle quote (2 lines)
> [synopsis]: Small adjustment.
Can you align these with the left margin? The lines can be longer too.
Toggle quote (184 lines)
>
> * 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 = \
> %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 for
> - ;; both doctype 4.2 and 4.5 are needed.
> - (("XSLTPROC_FLAGS = ") "XSLTPROC_FLAGS = --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="
> - (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=elogind"
> -
> - (string-append "--with-rootprefix="
> - (assoc-ref %outputs "out"))
> - (string-append "--with-rootlibexecdir="
> - (assoc-ref %outputs "out")
> - "/libexec/elogind")
> - ;; These are needed to ensure that lto linking works.
> - "RANLIB=gcc-ranlib"
> - "AR=gcc-ar"
> - "NM=gcc-nm")
> - #:make-flags '("PKTTYAGENT=/run/current-system/profile/bin/pkttyagent")
> - #:phases
> - (modify-phases %standard-phases
> - (add-after 'unpack 'patch-locale-header
> - (lambda _
> - ;; Fix compilation with glibc >= 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=.*")
> - (string-append "Exec=" (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
> - ;; <https://lists.gnu.org/archive/html/guix-devel/2017-09/msg00084.html>.
> - (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==\"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
> - ;; <https://github.com/wingo/elogind/issues/8>.
> - ("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 /dev 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.
Toggle quote (10 lines)
> + (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?
Toggle quote (4 lines)
> + ;; 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.
Toggle quote (8 lines)
> + (substitute* "man/meson.build"
> + ;; Necessary because
> + ;; there is no internet access
> + ;; inside the build environment.
> + (("xsltproc_flags = \\[")
> + (string-append "xsltproc_flags = ["
> + " '--novalid',")))
Please retain the original comment for this substitution.
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?
Toggle quote (6 lines)
> + (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.
Toggle quote (4 lines)
> + #t))))
> + (build-system meson-build-system)
> + (outputs '("out"))
'("out") is the default, so this line can be removed.
Toggle quote (10 lines)
> + (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.
Toggle quote (15 lines)
> + ;;
> + #: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=" out)
Please use (list ...) here instead of quote/unquote.
Toggle quote (26 lines)
> + ,(string-append
> + "-Dsysconfdir=" sysconf)
> + ,(string-append
> + "-Drootlibexecdir=" libexec)
> + ,(string-append
> + "-Ddbuspolicydir=" dbuspolicy)
> + ,(string-append
> + "-Dc_link_args=-Wl,-rpath=" libexec)
> + ,(string-append
> + "-Dcpp_link_args=-Wl,-rpath=" libexec)
> + ,(string-append
> + "-Dhalt-path=" halt-path)
> + ,(string-append
> + "-Dkexec-path=" kexec-path)
> + ,(string-append
> + "-Dpoweroff-path=" poweroff-path)
> + ,(string-append
> + "-Dreboot-path=" reboot-path)
> + "-Dcgroup-controller=elogind"
> + ;; Disable some tests.
> + "-Dtests=false"
> + "-Dslow-tests=false"))
> + #:phases
> + (modify-phases %standard-phases
> + (delete 'bootstrap))))
Can you add a comment about why this is necessary?
Toggle quote (19 lines)
> + (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.
The rest of the patch LGTM, though I admit that I did not test it yet.
Please also make sure "make check-system TESTS=elogind" passes. I found
I had to s|/dev/tty1|tty1| in gnu/tests/desktop.scm in my branch.
Can you send updates patches? Thanks in advance!
Toggle quote (49 lines)
> + ("libcap" ,libcap)
> + ("libseccomp" ,libseccomp)
> + ("libselinux" ,libselinux)
> + ("linux-pam" ,linux-pam)
> + ("pcre2" ,pcre2)
> + ("python" ,python) ;; libpython optional input
> + ("shepherd" ,shepherd)
> + ("util-linux" ,util-linux)))
> + (home-page "https://github.com/elogind/elogind")
> + (synopsis "Elogind provides user, seat, and session management service")
> + (description "Elogind is the systemd project's \"logind\" service,
> extracted out as a separate project. Elogind integrates with PAM to provide
> the org.freedesktop.login1 interface over the system bus, allowing other parts
> of a the system to know what users are logged in, and where.")
> - (license license:lgpl2.1+)))
> + (license license:lgpl2.1+))))
>
> (define-public packagekit
> (package
> diff --git a/gnu/packages/patches/elogind-glibc-2.27.patch b/gnu/packages/patches/elogind-glibc-2.27.patch
> deleted file mode 100644
> index 4ade587b5..000000000
> --- a/gnu/packages/patches/elogind-glibc-2.27.patch
> +++ /dev/null
> @@ -1,22 +0,0 @@
> -Look for memfd_create in sys/mman.h instead of linux/memfd.h.
> -Needed to build with glibc-2.27.
> -
> ---- a/configure.ac 1969-12-31 19:00:00.000000000 -0500
> -+++ b/configure.ac 2018-03-27 23:54:15.414589005 -0400
> -@@ -360,7 +360,7 @@
> - # ------------------------------------------------------------------------------
> -
> - AC_CHECK_HEADERS([sys/capability.h], [], [AC_MSG_ERROR([*** POSIX caps headers not found])])
> --AC_CHECK_HEADERS([linux/memfd.h], [], [])
> -+AC_CHECK_HEADERS([sys/mman.h], [], [])
> -
> - AC_CHECK_HEADERS([printf.h], [have_printf_h=yes], [have_printf_h=no])
> - AS_IF([test x$have_printf_h = xyes], [
> -@@ -395,6 +395,7 @@
> - [], [], [[
> - #include <sys/types.h>
> - #include <unistd.h>
> -+#include <sys/mman.h>
> - #include <sys/mount.h>
> - #include <fcntl.h>
> - #include <sched.h>
> --
> 2.19.2
