[PATCH 2/2] guix-install.sh: Directly exit in case of errors in chk_require.

  • Open
  • quality assurance status badge
Details
3 participants
  • Ludovic Courtès
  • Maxim Cournoyer
  • Tobias Geerinckx-Rice
Owner
unassigned
Submitted by
Maxim Cournoyer
Severity
normal
Merged with
M
M
Maxim Cournoyer wrote on 14 Dec 2022 16:56
(address . guix-patches@gnu.org)(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
20221214155603.29381-2-maxim.cournoyer@gmail.com
* etc/guix-install.sh (chk_require): Directly exit in case of errors in
chk_require, instead of relying on 'set -e'.
---
etc/guix-install.sh | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)

Toggle diff (19 lines)
diff --git a/etc/guix-install.sh b/etc/guix-install.sh
index 06730f7e3f..0ca12f8b66 100755
--- a/etc/guix-install.sh
+++ b/etc/guix-install.sh
@@ -137,10 +137,8 @@ chk_require()
command -v "$c" &>/dev/null || warn+=("$c")
done
- [ "${#warn}" -ne 0 ] &&
- { _err "${ERR}Missing commands: ${warn[*]}.";
- return 1; }
-
+ [ "${#warn}" -ne 0 ] && die "Missing commands: ${warn[*]}."
+
_msg "${PAS}verification of required commands completed"
}
--
2.38.1
T
T
Tobias Geerinckx-Rice wrote on 14 Dec 2022 17:37
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
871qp2c54w.fsf@nckx
Maxim Cournoyer 写道:
Toggle quote (7 lines)
> - [ "${#warn}" -ne 0 ] &&
> - { _err "${ERR}Missing commands: ${warn[*]}.";
> - return 1; }
> -
> + [ "${#warn}" -ne 0 ] && die "Missing commands: ${warn[*]}."
> +

I did not run this, but will it not itself trigger -e when the
test is false?

Kind regards,

T G-R
-----BEGIN PGP SIGNATURE-----

iIMEARYKACsWIQT12iAyS4c9C3o4dnINsP+IT1VteQUCY5n7/w0cbWVAdG9iaWFz
LmdyAAoJEA2w/4hPVW153VEBAMZc1COX05j1JvkESrO5nlRi50eA3a+azXu7O3HW
KAJoAQDMJihIBA5fp84NNcckxHto8PMi+dC9sXNcDFCU23NzAg==
=1EgH
-----END PGP SIGNATURE-----

M
M
Maxim Cournoyer wrote on 14 Dec 2022 19:17
(name . Tobias Geerinckx-Rice)(address . me@tobias.gr)
875yed4zp5.fsf@gmail.com
Hi Tobias,

Tobias Geerinckx-Rice <me@tobias.gr> writes:

Toggle quote (11 lines)
> Maxim Cournoyer 写道:
>> - [ "${#warn}" -ne 0 ] &&
>> - { _err "${ERR}Missing commands: ${warn[*]}.";
>> - return 1; }
>> - + [ "${#warn}" -ne 0 ] && die "Missing commands:
>> ${warn[*]}."
>> +
>
> I did not run this, but will it not itself trigger -e when the test
> is false?

This apparently falls in the special casing by Bash of what is
considered a failure when using 'set -e'; here's a test:

Toggle snippet (8 lines)
$ cat test.sh
#!/usr/bin/env bash

set -e

[ false ] && echo "hey, we made it!"

Toggle snippet (4 lines)
$ ./test.sh
hey, we made it!

I hope this answers your question.

--
Thanks,
Maxim
M
M
Maxim Cournoyer wrote on 14 Dec 2022 19:33
Re: bug#60069: [PATCH 2/2] guix-install.sh: Directly exit in case of errors in chk_require.
(name . Tobias Geerinckx-Rice)(address . me@tobias.gr)(address . 60069@debbugs.gnu.org)
87sfhh3keh.fsf_-_@gmail.com
Hi,

Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

Toggle quote (28 lines)
> Hi Tobias,
>
> Tobias Geerinckx-Rice <me@tobias.gr> writes:
>
>> Maxim Cournoyer 写道:
>>> - [ "${#warn}" -ne 0 ] &&
>>> - { _err "${ERR}Missing commands: ${warn[*]}.";
>>> - return 1; }
>>> - + [ "${#warn}" -ne 0 ] && die "Missing commands:
>>> ${warn[*]}."
>>> +
>>
>> I did not run this, but will it not itself trigger -e when the test
>> is false?
>
> This apparently falls in the special casing by Bash of what is
> considered a failure when using 'set -e'; here's a test:
>
> $ cat test.sh
> #!/usr/bin/env bash
>
> set -e
>
> [ false ] && echo "hey, we made it!"
>
> $ ./test.sh
> hey, we made it!

The above example was bogus and unnecessary; looking at it more closely,
the test would return true when the 'warn' array contains 1 or more
items (missing commands), which would cause the die command to be
invoked and the script to exit. The first test handling isn't modified,
so it'll chain though the second part the same as it does now.

I hope that's a better explanation.

--
Thanks,
Maxim
M
M
Maxim Cournoyer wrote on 14 Dec 2022 21:47
control message for bug #60068
(address . control@debbugs.gnu.org)
87cz8l3e7l.fsf@gmail.com
forcemerge 60068 60069
quit
L
L
Ludovic Courtès wrote on 15 Dec 2022 15:41
Re: bug#60069: [PATCH 2/2] guix-install.sh: Directly exit in case of errors in chk_require.
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
871qp0pw4y.fsf_-_@gnu.org
Hi,

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

Toggle quote (2 lines)
> * etc/guix-install.sh: (REQUIRE): Add missing "useradd" command.

[...]

Toggle quote (3 lines)
> * etc/guix-install.sh (chk_require): Directly exit in case of errors in
> chk_require, instead of relying on 'set -e'.

These two patches LGTM; you can add them to ‘master’ and that way people
will benefit from it when installing 1.4.0.

Thanks!

Ludo’.
L
L
Ludovic Courtès wrote on 15 Dec 2022 15:43
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
87wn6sohh7.fsf_-_@gnu.org
Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

Toggle quote (9 lines)
> The need for this use case appeared when attempting to install Guix on a truly
> minimal image made with Buildroot, which lacked enough GNU components that I
> had to extract a guix pack to /gnu before attempting installation, which would
> then refuse to proceed because of the existing /gnu.
>
> * etc/guix-install.sh: Document environment variables.
> (sys_create_store) [GUIX_ALLOW_OVERWRITE]: Skip pre-existing installation
> checks and output a warning. Extract the tarball directly to /.

Like Tobias, I’m reluctant to adding environment variables; I’m also
skeptical about the use case (I think it’s fine to let users remove
their previous installation if that’s what they want).

I also think we’d rather minimize changes to the script since we’re a
couple of days before the release.

Thanks,
Ludo’.
L
L
Ludovic Courtès wrote on 15 Dec 2022 15:44
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
87v8mcoheh.fsf_-_@gnu.org
Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

Toggle quote (9 lines)
> The need for this use case appeared when attempting to install Guix on a truly
> minimal image made with Buildroot, which lacked enough GNU components that I
> had to extract a guix pack to /gnu before attempting installation, which would
> then refuse to proceed because of the existing /gnu.
>
> * etc/guix-install.sh: Document environment variables.
> (sys_create_store) [GUIX_ALLOW_OVERWRITE]: Skip pre-existing installation
> checks and output a warning. Extract the tarball directly to /.

Like Tobias, I’m reluctant to adding environment variables; I’m also
skeptical about the use case (I think it’s fine to let users remove
their previous installation if that’s what they want).

I also think we’d rather minimize changes to the script since we’re a
couple of days before the release.

Thanks,
Ludo’.
M
M
Maxim Cournoyer wrote on 16 Dec 2022 06:07
(name . Ludovic Courtès)(address . ludo@gnu.org)
87wn6saqcz.fsf@gmail.com
Hi Ludovic,

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

Toggle quote (15 lines)
> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>> The need for this use case appeared when attempting to install Guix on a truly
>> minimal image made with Buildroot, which lacked enough GNU components that I
>> had to extract a guix pack to /gnu before attempting installation, which would
>> then refuse to proceed because of the existing /gnu.
>>
>> * etc/guix-install.sh: Document environment variables.
>> (sys_create_store) [GUIX_ALLOW_OVERWRITE]: Skip pre-existing installation
>> checks and output a warning. Extract the tarball directly to /.
>
> Like Tobias, I’m reluctant to adding environment variables; I’m also
> skeptical about the use case (I think it’s fine to let users remove
> their previous installation if that’s what they want).

Removing my previous installation wouldn't have helped (it would have
cleared the guix packs I needed to be able to run the installer).
Without this change, I wouldn't have been able to install guix using
guix-install.sh. It's niche, but I bet it'd help folks trying to
install Guix on Alpine and similar minimal OSes.

Toggle quote (3 lines)
> I also think we’d rather minimize changes to the script since we’re a
> couple of days before the release.

The change seems fairly small to me and would be easy to revert if it
causes a problem. But I'll let you do the call, given you're the one
pushing the release forward (thank you!).

--
Thanks,
Maxim
?