[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[bug#33471] [PATCH] gnu: elogind: Update to 239.2.
|
From: |
Stefan Stefanović |
|
Subject: |
[bug#33471] [PATCH] gnu: elogind: Update to 239.2. |
|
Date: |
Sat, 24 Nov 2018 04:10:36 +0100 |
On 11/23/18, Marius Bakke <address@hidden> wrote:
> Stefan Stefanović <address@hidden> 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: =?UTF-8?q?Stefan=20Stefanovi=C4=87?= <address@hidden>
>> 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 message
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 =
>> \
>> %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.
>
>> + (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 comment.
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 = \\[")
>> + (string-append "xsltproc_flags = ["
>> + " '--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 "address@hidden" 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=" 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=" 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?
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=elogind" passes. I found
> 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.