[PATCH] guix-install.sh: Add daemonize to requirements.

  • Done
  • quality assurance status badge
Details
3 participants
  • Ludovic Courtès
  • Richard Sent
  • Simon Tournier
Owner
unassigned
Submitted by
Richard Sent
Severity
normal
R
R
Richard Sent wrote on 2 Apr 18:14 +0200
(address . guix-patches@gnu.org)(name . Richard Sent)(address . richard@freakingpenguin.com)
4e4fa72cfff690365f06c94534bc3d03ac7a3418.1712074456.git.richard@freakingpenguin.com
* 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(+)

Toggle diff (16 lines)
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
L
L
Ludovic Courtès wrote on 4 May 18:49 +0200
(name . Richard Sent)(address . richard@freakingpenguin.com)(address . 70148@debbugs.gnu.org)
87wmo9sesk.fsf@gnu.org
Hi Richard,

Richard Sent <richard@freakingpenguin.com> skribis:

Toggle quote (13 lines)
> * 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?

Toggle quote (3 lines)
> 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’.
R
R
Richard Sent wrote on 5 May 00:44 +0200
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 70148@debbugs.gnu.org)
87bk5l42pc.fsf@freakingpenguin.com
Hi Ludo!

Toggle quote (6 lines)
>> 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.

Toggle quote (6 lines)
> 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.
S
S
Simon Tournier wrote on 22 May 16:08 +0200
(address . 70148@debbugs.gnu.org)
87ttiq2b3r.fsf@gmail.com
Hi,

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

Toggle quote (4 lines)
> 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
L
L
Ludovic Courtès wrote on 25 May 15:59 +0200
(name . Simon Tournier)(address . zimon.toutoune@gmail.com)
87ed9q0z7o.fsf@gnu.org
Hi,

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

Toggle quote (9 lines)
> On sam., 04 mai 2024 at 18:44, Richard Sent <richard@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’.
R
R
Richard Sent wrote on 29 May 02:36 +0200
[PATCH v2] guix-install.sh: Add unique requirement for sysv init system
(address . 70148@debbugs.gnu.org)
4253a50c53caa41f468163c6f9f8730e86678f38.1716942966.git.richard@freakingpenguin.com
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(-)

Toggle diff (60 lines)
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@gmail.com>
# Copyright © 2024 Janneke Nieuwenhuizen <janneke@gnu.org>
# Copyright © 2024 Tomas Volf <~@wolfsden.cz>
+# Copyright © 2024 Richard Sent <richard@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
L
L
Ludovic Courtès wrote on 2 Jun 11:45 +0200
(name . Richard Sent)(address . richard@freakingpenguin.com)
87a5k3n0eg.fsf@gnu.org
Hi Richard,

Richard Sent <richard@freakingpenguin.com> skribis:

Toggle quote (19 lines)
> 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:

Toggle quote (11 lines)
> +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’.
R
R
Richard Sent wrote on 2 Jun 16:33 +0200
(name . Ludovic Courtès)(address . ludo@gnu.org)
87ikyre7mz.fsf@freakingpenguin.com
Hi Ludo!

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

Toggle quote (6 lines)
> ‘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:

Toggle snippet (6 lines)
if [ "x$BASH_VERSION" = "x" ]
then
exec bash "$0" "$@"
fi

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.)

Toggle snippet (9 lines)
# 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" ]
.......

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


--
Take it easy,
Richard Sent
Making my computer weirder one commit at a time.
L
L
Ludovic Courtès wrote on 4 Jun 12:11 +0200
(name . Richard Sent)(address . richard@freakingpenguin.com)
87zfs16mqq.fsf@gnu.org
Hello,

Richard Sent <richard@freakingpenguin.com> skribis:

Toggle quote (8 lines)
> Ludovic Courtès <ludo@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?

[...]

Toggle quote (2 lines)
> 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’.
Closed
?
Your comment

This issue is archived.

To comment on this conversation send an email to 70148@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 70148
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch