GNU bug report logs - #70148
[PATCH] guix-install.sh: Add daemonize to requirements.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 70148 in the body.
You can then email your comments to 70148 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Richard Sent <richard <at> freakingpenguin.com>
To: guix-patches <at> gnu.org
Cc: Richard Sent <richard <at> freakingpenguin.com>
Subject: [PATCH] guix-install.sh: Add daemonize to requirements.
Date: Tue,  2 Apr 2024 12:14:16 -0400

* etc/guix-install.sh (REQUIRE): Add daemonize to requirements list.
Needed to spawn the Guix Daemon in guix-daemon.in

Change-Id: I77c7f2bdd686bb023ecfa108a499c2eafbad1eb7
---

Hi Guix. I noticed that in Debian WSL the guix daemon sysvinit service
wouldn't start due to daemonize not being present. This patch should
catch that issue sooner.

Both openrc and sysvinit use guix-daemon.in so daemonize should be
required regardless of the init system.

 etc/guix-install.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/etc/guix-install.sh b/etc/guix-install.sh
index 982fb0a266..94ecb1d8f3 100755
--- a/etc/guix-install.sh
+++ b/etc/guix-install.sh
@@ -56,6 +56,7 @@ set -eo pipefail
 [ "$UID" -eq 0 ] || { echo "This script must be run as root."; exit 1; }
 
 REQUIRE=(
+    "daemonize"
     "dirname"
     "readlink"
     "wget"

base-commit: d67e4f0f9b10c7ddac8fb0ca68cbf1d6ad0a6e5d
-- 
2.41.0

Message #8 received at 70148 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Richard Sent <richard <at> freakingpenguin.com>
Cc: 70148 <at> debbugs.gnu.org
Subject: Re: [bug#70148] [PATCH] guix-install.sh: Add daemonize to
 requirements.
Date: Sat, 04 May 2024 18:49:31 +0200

Hi Richard,

Richard Sent <richard <at> freakingpenguin.com> skribis:

> * etc/guix-install.sh (REQUIRE): Add daemonize to requirements list.
> Needed to spawn the Guix Daemon in guix-daemon.in
>
> Change-Id: I77c7f2bdd686bb023ecfa108a499c2eafbad1eb7
> ---
>
> Hi Guix. I noticed that in Debian WSL the guix daemon sysvinit service
> wouldn't start due to daemonize not being present. This patch should
> catch that issue sooner.
>
> Both openrc and sysvinit use guix-daemon.in so daemonize should be
> required regardless of the init system.

‘daemonize’ seems to be used by etc/init.d/guix-daemon.in, but not by
etc/openrc/guix-daemon.in, right?

>  REQUIRE=(
> +    "daemonize"

My only concern is if the majority of users (which I assume use systemd)
would get an error for a missing package they don’t actually need.

Do you think that is a risk or is ‘daemonize’ usually installed “by
default” even on those systemd distros?

Thanks,
Ludo’.

Message #11 received at 70148 <at> debbugs.gnu.org (full text, mbox):

From: Richard Sent <richard <at> freakingpenguin.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 70148 <at> debbugs.gnu.org
Subject: Re: [bug#70148] [PATCH] guix-install.sh: Add daemonize to
 requirements.
Date: Sat, 04 May 2024 18:44:31 -0400

Hi Ludo!

>> Both openrc and sysvinit use guix-daemon.in so daemonize should be
>> required regardless of the init system.
>
> ‘daemonize’ seems to be used by etc/init.d/guix-daemon.in, but not by
> etc/openrc/guix-daemon.in, right?

Correct, I think I got confused by guix-install.sh copying
etc/openrc/guix-daemon to /etc/init.d/guix-daemon. (See the openrc
handler in the $INIT_SYS case statement in sys_enable_guix_daemon()).
Assuming I understand the script; I find shell scripts hard to parse.

> My only concern is if the majority of users (which I assume use systemd)
> would get an error for a missing package they don’t actually need.
>
> Do you think that is a risk or is ‘daemonize’ usually installed “by
> default” even on those systemd distros?

I suspect there's a risk. WSL Debian does not come with daemonize out of
the box so I imagine normal Debian does not either.

If OpenRC/systemd does not require daemonize, there's probably a better way to
handle this. Perhaps a INITD_REQUIRE variable that's only checked
conditionally depending on what init system is detected (aka INIT_SYS).

-- 
Take it easy,
Richard Sent
Making my computer weirder one commit at a time.

Message #14 received at 70148 <at> debbugs.gnu.org (full text, mbox):

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: Richard Sent <richard <at> freakingpenguin.com>, Ludovic Courtès <ludo <at> gnu.org>
Cc: 70148 <at> debbugs.gnu.org
Subject: Re: [bug#70148] [PATCH] guix-install.sh: Add daemonize to
 requirements.
Date: Wed, 22 May 2024 16:08:08 +0200

Hi,

On sam., 04 mai 2024 at 18:44, Richard Sent <richard <at> freakingpenguin.com> wrote:

> If OpenRC/systemd does not require daemonize, there's probably a better way to
> handle this. Perhaps a INITD_REQUIRE variable that's only checked
> conditionally depending on what init system is detected (aka INIT_SYS).

This appears to me the best solution.  It seems better that the
requirements stay as minimal as possible for most of users.

Well, maybe another variable as INITD_REQUIRE.  Or maybe a plain test
for this daemonize when the init system is detected; since for now there
is only one extra package, if I read correctly.

Cheers,
simon

Message #17 received at 70148 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Simon Tournier <zimon.toutoune <at> gmail.com>
Cc: Richard Sent <richard <at> freakingpenguin.com>, 70148 <at> debbugs.gnu.org
Subject: Re: [bug#70148] [PATCH] guix-install.sh: Add daemonize to
 requirements.
Date: Sat, 25 May 2024 15:59:23 +0200

Hi,

Simon Tournier <zimon.toutoune <at> gmail.com> skribis:

> On sam., 04 mai 2024 at 18:44, Richard Sent <richard <at> freakingpenguin.com> wrote:
>
>> If OpenRC/systemd does not require daemonize, there's probably a better way to
>> handle this. Perhaps a INITD_REQUIRE variable that's only checked
>> conditionally depending on what init system is detected (aka INIT_SYS).
>
> This appears to me the best solution.  It seems better that the
> requirements stay as minimal as possible for most of users.

+1.  Would be best to ensure the installer doesn’t get in the way of
those using systemd distros.

Could you look into it, Richard?

Ludo’.

Message #20 received at 70148 <at> debbugs.gnu.org (full text, mbox):

From: Richard Sent <richard <at> freakingpenguin.com>
To: 70148 <at> debbugs.gnu.org
Cc: ludo <at> gnu.org, Richard Sent <richard <at> freakingpenguin.com>,
 zimon.toutoune <at> gmail.com
Subject: [PATCH v2] guix-install.sh: Add unique requirement for sysv init
 system
Date: Tue, 28 May 2024 20:36:06 -0400

This improves the installer's ability to detect that all requirements are
present regardless of init system. It also avoids performing the requirement
check twice (printing excessively to the console) and provides a framework for
adding new init system specific requirements if it's needed in the future.

* etc/guix-install.sh (add_init_sys_require): Create.
(SYSV_INIT_REQUIRE): Create.
(main_install): Reorder installer steps so all requirements are checked in one
pass.

Change-Id: Ic541c1b90499d504642b7ab4ae595501b1a37b0d
---
Hi all,

Here's an updated patch that is more selective about only checking for
dependencies when it's required. It might be a touch overengineered,
but I felt this was a better solution compared to hardcoding a
daemonize requirement check in chk_init_sys or similar.

I wanted to avoid calling chk_require twice. Because chk_require
outputs to the console, calling it multiple times will either a) print
multiple "verification of blah blah blah" messages to the user or b)
cause the user to fix the generic requirements, then need to fix init
system specific requirements as a two step process.

If it's ever relevant, this patch will make it quite easy to add
additional checks for specific init systems.

 etc/guix-install.sh | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/etc/guix-install.sh b/etc/guix-install.sh
index 82accfd5d5..09a7ca3ae8 100755
--- a/etc/guix-install.sh
+++ b/etc/guix-install.sh
@@ -15,6 +15,7 @@
 # Copyright © 2020 David A. Redick <david.a.redick <at> gmail.com>
 # Copyright © 2024 Janneke Nieuwenhuizen <janneke <at> gnu.org>
 # Copyright © 2024 Tomas Volf <~@wolfsden.cz>
+# Copyright © 2024 Richard Sent <richard <at> freakingpenguin.com>
 #
 # This file is part of GNU Guix.
 #
@@ -81,6 +82,12 @@ REQUIRE=(
     "xz"
 )
 
+# Add variables using form FOO_INIT_REQUIRE when init system FOO dependencies
+# should be checked.
+SYSV_INIT_REQUIRE=(
+    "daemonize"
+)
+
 PAS=$'[ \033[32;1mPASS\033[0m ] '
 ERR=$'[ \033[31;1mFAIL\033[0m ] '
 WAR=$'[ \033[33;1mWARN\033[0m ] '
@@ -148,6 +155,18 @@ chk_require()
     _msg "${PAS}verification of required commands completed"
 }
 
+add_init_sys_require()
+{ # Add the elements of FOO_INIT_SYS to REQUIRE
+    local init_require="${INIT_SYS}_REQUIRE[@]"
+    if [[ ! -z "$init_require" ]]; then
+        # Have to add piecemeal because ${!foo[@]} performs direct array key
+        # expansion, not indirect plain array expansion.
+        for r in "${!init_require}"; do
+            REQUIRE+=("$r")
+        done
+    fi
+}
+
 chk_gpg_keyring()
 { # Check whether the Guix release signing public key is present.
     _debug "--- [ ${FUNCNAME[0]} ] ---"
@@ -793,9 +812,10 @@ main_install()
     _msg "Starting installation ($(date))"
 
     chk_term
+    chk_init_sys
+    add_init_sys_require
     chk_require "${REQUIRE[@]}"
     chk_gpg_keyring
-    chk_init_sys
     chk_sys_arch
     chk_sys_nscd
 

base-commit: 542b18709a46e361de8f25e3fece29860532743c
-- 
2.41.0

Message #23 received at 70148 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Richard Sent <richard <at> freakingpenguin.com>
Cc: 70148 <at> debbugs.gnu.org, zimon.toutoune <at> gmail.com
Subject: Re: [bug#70148] [PATCH v2] guix-install.sh: Add unique requirement
 for sysv init system
Date: Sun, 02 Jun 2024 11:45:27 +0200

Hi Richard,

Richard Sent <richard <at> freakingpenguin.com> skribis:

> This improves the installer's ability to detect that all requirements are
> present regardless of init system. It also avoids performing the requirement
> check twice (printing excessively to the console) and provides a framework for
> adding new init system specific requirements if it's needed in the future.
>
> * etc/guix-install.sh (add_init_sys_require): Create.
> (SYSV_INIT_REQUIRE): Create.
> (main_install): Reorder installer steps so all requirements are checked in one
> pass.
>
> Change-Id: Ic541c1b90499d504642b7ab4ae595501b1a37b0d
> ---
> Hi all,
>
> Here's an updated patch that is more selective about only checking for
> dependencies when it's required. It might be a touch overengineered,
> but I felt this was a better solution compared to hardcoding a
> daemonize requirement check in chk_init_sys or similar.

Neat!  I have one concern though:

> +add_init_sys_require()
> +{ # Add the elements of FOO_INIT_SYS to REQUIRE
> +    local init_require="${INIT_SYS}_REQUIRE[@]"
> +    if [[ ! -z "$init_require" ]]; then
> +        # Have to add piecemeal because ${!foo[@]} performs direct array key
> +        # expansion, not indirect plain array expansion.
> +        for r in "${!init_require}"; do
> +            REQUIRE+=("$r")
> +        done
> +    fi

‘local’, [[, and arrays are probably Bash-specific.  However this is a
#!/bin/sh script, and some systems such as Debian use Dash as /bin/sh.
So I’m afraid the script would break on such systems.

WDYT?  Do you think we can avoid those features?

So much for a script.  :-)

Thanks!

Ludo’.

Message #26 received at 70148 <at> debbugs.gnu.org (full text, mbox):

From: Richard Sent <richard <at> freakingpenguin.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 70148 <at> debbugs.gnu.org, zimon.toutoune <at> gmail.com
Subject: Re: [bug#70148] [PATCH v2] guix-install.sh: Add unique requirement
 for sysv init system
Date: Sun, 02 Jun 2024 10:33:56 -0400

Hi Ludo!

Ludovic Courtès <ludo <at> gnu.org> writes:

> ‘local’, [[, and arrays are probably Bash-specific.  However this is a
> #!/bin/sh script, and some systems such as Debian use Dash as /bin/sh.
> So I’m afraid the script would break on such systems.
>
> WDYT?  Do you think we can avoid those features?

Right, I did notice that. A strictly 100% POSIX-compliant shell doesn't
support local, arrays, or [[]] constructs. From my understanding Dash
supports 1 and /maybe/ 3, but not 2.

However, the script already contains provisions to enter bash even if launched
using a POSIX-complaint shell. See this at the beginning:

--8<---------------cut here---------------start------------->8---
if [ "x$BASH_VERSION" = "x" ]
then
    exec bash "$0" "$@"
fi
--8<---------------cut here---------------end--------------->8---

This construct is a little bit odd to me. At the time it was added ( by
you ;) ) because /usr/bin/env did not exist on Guix System [1], but
that's been added since [2]. Perhaps we should change the shebang to
#!/usr/bin/env bash.

(Also looks like the comment you wrote re. bash was spliced away from
the code.)

--8<---------------cut here---------------start------------->8---
# We require Bash but for portability we'd rather not use /bin/bash or
# /usr/bin/env in the shebang, hence this hack.

# Environment variables
.......
if [ "x$BASH_VERSION" = "x" ]
.......
--8<---------------cut here---------------end--------------->8---

The script is already using all 3 of these constructs too.

[1]: https://issues.guix.gnu.org/34279
[2]: https://issues.guix.gnu.org/35910

-- 
Take it easy,
Richard Sent
Making my computer weirder one commit at a time.

Message #31 received at 70148-done <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Richard Sent <richard <at> freakingpenguin.com>
Cc: zimon.toutoune <at> gmail.com, 70148-done <at> debbugs.gnu.org
Subject: Re: [bug#70148] [PATCH v2] guix-install.sh: Add unique requirement
 for sysv init system
Date: Tue, 04 Jun 2024 12:11:41 +0200

Hello,

Richard Sent <richard <at> freakingpenguin.com> skribis:

> Ludovic Courtès <ludo <at> gnu.org> writes:
>
>> ‘local’, [[, and arrays are probably Bash-specific.  However this is a
>> #!/bin/sh script, and some systems such as Debian use Dash as /bin/sh.
>> So I’m afraid the script would break on such systems.
>>
>> WDYT?  Do you think we can avoid those features?

[...]

> The script is already using all 3 of these constructs too.

Oh right, I had completely overlooked that.  In that case my point is
moot (it might be useful to not require Bash, but that’s a separate
topic).

Pushed as 40c6f708393885a2d28f847350e8f47beb11e745, thanks!

Ludo’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.