GNU bug report logs - #60069
[PATCH 2/2] guix-install.sh: Directly exit in case of errors in chk_require.

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 60069 in the body.
You can then email your comments to 60069 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: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: guix-patches <at> gnu.org
Cc: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Subject: [PATCH 2/2] guix-install.sh: Directly exit in case of errors in
 chk_require.
Date: Wed, 14 Dec 2022 10:56:03 -0500

* 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(-)

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

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

From: Tobias Geerinckx-Rice <me <at> tobias.gr>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: 60069 <at> debbugs.gnu.org, guix-patches <at> gnu.org
Subject: Re: [bug#60069] [PATCH 2/2] guix-install.sh: Directly exit in case
 of errors in chk_require.
Date: Wed, 14 Dec 2022 17:37:19 +0100

[Message part 1 (text/plain, inline)]

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?

Kind regards,

T G-R

[signature.asc (application/pgp-signature, inline)]

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

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Tobias Geerinckx-Rice <me <at> tobias.gr>
Cc: 60069 <at> debbugs.gnu.org, guix-patches <at> gnu.org
Subject: Re: [bug#60069] [PATCH 2/2] guix-install.sh: Directly exit in case
 of errors in chk_require.
Date: Wed, 14 Dec 2022 13:17:42 -0500

Hi Tobias,

Tobias Geerinckx-Rice <me <at> 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:

--8<---------------cut here---------------start------------->8---
$ cat test.sh
#!/usr/bin/env bash

set -e

[ false ] && echo "hey, we made it!"
--8<---------------cut here---------------end--------------->8---

--8<---------------cut here---------------start------------->8---
$ ./test.sh
hey, we made it!
--8<---------------cut here---------------end--------------->8---

I hope this answers your question.

-- 
Thanks,
Maxim

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

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Tobias Geerinckx-Rice <me <at> tobias.gr>
Cc: 60069 <at> debbugs.gnu.org
Subject: Re: bug#60069: [PATCH 2/2] guix-install.sh: Directly exit in case
 of errors in chk_require.
Date: Wed, 14 Dec 2022 13:33:26 -0500

Hi,

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

> Hi Tobias,
>
> Tobias Geerinckx-Rice <me <at> 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

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: me <at> tobias.gr, 60069 <at> debbugs.gnu.org, 60068 <at> debbugs.gnu.org
Subject: Re: bug#60069: [PATCH 2/2] guix-install.sh: Directly exit in case
 of errors in chk_require.
Date: Thu, 15 Dec 2022 15:41:17 +0100

Hi,

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

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

[...]

> * 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’.

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: me <at> tobias.gr, 60069 <at> debbugs.gnu.org, 60068 <at> debbugs.gnu.org
Subject: Re: bug#60069: [PATCH 2/2] guix-install.sh: Directly exit in case
 of errors in chk_require.
Date: Thu, 15 Dec 2022 15:43:16 +0100

Maxim Cournoyer <maxim.cournoyer <at> 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).

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

Thanks,
Ludo’.

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: me <at> tobias.gr, 60069 <at> debbugs.gnu.org, 60068 <at> debbugs.gnu.org
Subject: Re: bug#60069: [PATCH 2/2] guix-install.sh: Directly exit in case
 of errors in chk_require.
Date: Thu, 15 Dec 2022 15:44:54 +0100

Maxim Cournoyer <maxim.cournoyer <at> 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).

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

Thanks,
Ludo’.

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

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: me <at> tobias.gr, 60069 <at> debbugs.gnu.org, 60068 <at> debbugs.gnu.org
Subject: Re: bug#60069: [PATCH 2/2] guix-install.sh: Directly exit in case
 of errors in chk_require.
Date: Fri, 16 Dec 2022 00:07:24 -0500

Hi Ludovic,

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

> Maxim Cournoyer <maxim.cournoyer <at> 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.

> 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

Previous Next

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