| Message ID | 4253a50c53caa41f468163c6f9f8730e86678f38.1716942966.git.richard@freakingpenguin.com |
|---|---|
| State | New |
| Headers | show |
| Series | [bug#70148,v2] guix-install.sh: Add unique requirement for sysv init system | expand |
Hi Richard, Richard Sent <richard@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’.
Hi Ludo! 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? 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
Hello, Richard Sent <richard@freakingpenguin.com> skribis: > 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? [...] > 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’.
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