[bug#60068] [PATCH 1/2] guix-install.sh: Add GUIX_ALLOW_OVERWRITE environment variable.

[bug#60068] [PATCH 1/2] guix-install.sh: Add GUIX_ALLOW_OVERWRITE environment variable.

[Maxim Cournoyer]

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.
(REQUIRE): Add missing "useradd" command.
(sys_create_store) [GUIX_ALLOW_OVERWRITE]: Skip pre-existing installation
checks and output a warning.  Extract the tarball directly to /.
---
 etc/guix-install.sh | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/etc/guix-install.sh b/etc/guix-install.sh
index fb9006b3e2..06730f7e3f 100755
--- a/etc/guix-install.sh
+++ b/etc/guix-install.sh
@@ -29,6 +29,22 @@
 
 # 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
+#
+# GUIX_BINARY_FILE_NAME
+#
+# Can be used to override the automatic download mechanism and point
+# to a local Guix binary archive filename like
+# "/tmp/guix-binary-1.4.0rc2.armhf-linux.tar.xz"
+#
+# GUIX_ALLOW_OVERWRITE
+#
+# Instead of aborting to avoid overwriting a previous installations,
+# allow copying over /var/guix or /gnu.  This can be useful when the
+# installation required the user to extract Guix packs under /gnu to
+# satisfy its dependencies.
+
 if [ "x$BASH_VERSION" = "x" ]
 then
     exec bash "$0" "$@"
@@ -53,6 +69,7 @@ REQUIRE=(
     "chmod"
     "uname"
     "groupadd"
+    "useradd"
     "tail"
     "tr"
     "xz"
@@ -337,16 +354,15 @@ sys_create_store()
 
     _debug "--- [ ${FUNCNAME[0]} ] ---"
 
-    if [[ -e "/var/guix" || -e "/gnu" ]]; then
+    if [[ -z $GUIX_ALLOW_OVERWRITE && (-e /var/guix || -e /gnu) ]]; then
         die "A previous Guix installation was found.  Refusing to overwrite."
+    else
+        _msg "${WAR}Overwriting existing installation!"
     fi
 
     cd "$tmp_path"
-    tar --extract --file "$pkg" && _msg "${PAS}unpacked archive"
-
     _msg "${INF}Installing /var/guix and /gnu..."
-    mv "${tmp_path}/var/guix" /var/
-    mv "${tmp_path}/gnu" /
+    tar --extract --file "$pkg" -C /
 
     _msg "${INF}Linking the root user's profile"
     mkdir -p ~root/.config/guix

base-commit: 1b6e251ed1bae7aa2f544e8ccb6b4aaf872e77e6
-- 
2.38.1

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

[Maxim Cournoyer]

* 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

[bug#60068] [PATCH 1/2] guix-install.sh: Add GUIX_ALLOW_OVERWRITE environment variable.

[Tobias Geerinckx-Rice via Guix-patches via]

[-- Attachment #1: Type: text/plain, Size: 1430 bytes --]

Hi Maxim,

Nice!  More steps towards world domination.

Maxim Cournoyer 写道：
> +# Environment variables

These sound like they should be command-line arguments.

[…]

Actually, I'm not totally sold on GUIX_ALLOW_OVERWRITE.  It's not 
solving much a problem.

Instead, the error message could be specific about what it 
considers a ‘previous Guix installation’ — which would be a good 
idea regardless — and tell the admin exactly what needs to be 
removed to continue.

>      "groupadd"
> +    "useradd"

Good catch, but separate patch.  (♪)

> -    if [[ -e "/var/guix" || -e "/gnu" ]]; then
> +    if [[ -z $GUIX_ALLOW_OVERWRITE && (-e /var/guix || -e /gnu) 
> ]]; then
>          die "A previous Guix installation was found.  Refusing 
>          to overwrite."
> +    else
> +        _msg "${WAR}Overwriting existing installation!"
>      fi
>  
>      cd "$tmp_path"
> -    tar --extract --file "$pkg" && _msg "${PAS}unpacked 
> archive"
> -
>      _msg "${INF}Installing /var/guix and /gnu..."
> -    mv "${tmp_path}/var/guix" /var/
> -    mv "${tmp_path}/gnu" /
> +    tar --extract --file "$pkg" -C /

I'm still in favour of using something like ‘mktemp -d 
/gnu.XXXXXX’ here if there's no security flaw I missed.  WDYT?

If the overwrite functionality is kept, we should remove the old 
directories before re-populating them.

Kind regards,

T G-R

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 247 bytes --]

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

[Tobias Geerinckx-Rice via Guix-patches via]

[-- Attachment #1: Type: text/plain, Size: 335 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 247 bytes --]

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

[Maxim Cournoyer]

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:

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

[bug#60068] [PATCH 1/2] guix-install.sh: Add GUIX_ALLOW_OVERWRITE environment variable.

[Maxim Cournoyer]

Hi Tobias,

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

> Hi Maxim,
>
> Nice!  More steps towards world domination.

Eh :-)

> Maxim Cournoyer 写道：
>> +# Environment variables
>
> These sound like they should be command-line arguments.

I agree, but that'd require a loop, or GNU getopt, and I'm not motivated
enough in the moment to re-design it :-).  When we get there, we could
add an --uninstall option too.

> […]
>
> Actually, I'm not totally sold on GUIX_ALLOW_OVERWRITE.  It's not
> solving much a problem.

The problem it solves for me was that I needed to use 'guix pack'd
dependencies such as gpg, glibc for getent, and shadow's
groupadd/useradd, etc. to satisfy the install script dependencies on my
weird target OS (minimal busybox embedded OS); which are needed to be
unpacked under /gnu, thus conflicting with the requirement that /gnu
doesn't exist.

I tried a relocatable pack, but it didn't work, at least for gpg (file
not found error).

> Instead, the error message could be specific about what it considers a
> ‘previous Guix installation’ — which would be a good idea regardless —
> and tell the admin exactly what needs to be removed to continue.
>
>>      "groupadd"
>> +    "useradd"
>
> Good catch, but separate patch.  (♪)

OK!

>> -    if [[ -e "/var/guix" || -e "/gnu" ]]; then
>> +    if [[ -z $GUIX_ALLOW_OVERWRITE && (-e /var/guix || -e /gnu) ]];
>> then
>>          die "A previous Guix installation was found.  Refusing
>> to overwrite."
>> +    else
>> +        _msg "${WAR}Overwriting existing installation!"
>>      fi
>>       cd "$tmp_path"
>> -    tar --extract --file "$pkg" && _msg "${PAS}unpacked archive"
>> -
>>      _msg "${INF}Installing /var/guix and /gnu..."
>> -    mv "${tmp_path}/var/guix" /var/
>> -    mv "${tmp_path}/gnu" /
>> +    tar --extract --file "$pkg" -C /
>
> I'm still in favour of using something like ‘mktemp -d /gnu.XXXXXX’
> here if there's no security flaw I missed.  WDYT?
>
> If the overwrite functionality is kept, we should remove the old
> directories before re-populating them.

Hopefully the reason the above makes more sense is also covered by my
use case explanation above.

Is the use case/change motivation a bit clearer now?

-- 
Thanks,
Maxim

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

[Maxim Cournoyer]

Hi,

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

> 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

[bug#60068] [PATCH v2 1/3] guix-install.sh: Add missing "useradd" command.

[Maxim Cournoyer]

* etc/guix-install.sh: (REQUIRE): Add missing "useradd" command.
---
 etc/guix-install.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/etc/guix-install.sh b/etc/guix-install.sh
index f008593d84..b8ea9e54c3 100755
--- a/etc/guix-install.sh
+++ b/etc/guix-install.sh
@@ -53,6 +53,7 @@ REQUIRE=(
     "chmod"
     "uname"
     "groupadd"
+    "useradd"
     "tail"
     "tr"
     "xz"

base-commit: fa23fb86f7741570d194bba1f227016d9aa25881
-- 
2.38.1

[bug#60068] [PATCH v2 2/3] guix-install.sh: Add GUIX_ALLOW_OVERWRITE environment variable.

[Maxim Cournoyer]

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 /.
---
 etc/guix-install.sh | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/etc/guix-install.sh b/etc/guix-install.sh
index b8ea9e54c3..62d85e765a 100755
--- a/etc/guix-install.sh
+++ b/etc/guix-install.sh
@@ -29,6 +29,22 @@
 
 # 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
+#
+# GUIX_BINARY_FILE_NAME
+#
+# Can be used to override the automatic download mechanism and point
+# to a local Guix binary archive filename like
+# "/tmp/guix-binary-1.4.0rc2.armhf-linux.tar.xz"
+#
+# GUIX_ALLOW_OVERWRITE
+#
+# Instead of aborting to avoid overwriting a previous installations,
+# allow copying over /var/guix or /gnu.  This can be useful when the
+# installation required the user to extract Guix packs under /gnu to
+# satisfy its dependencies.
+
 if [ "x$BASH_VERSION" = "x" ]
 then
     exec bash "$0" "$@"
@@ -338,16 +354,15 @@ sys_create_store()
 
     _debug "--- [ ${FUNCNAME[0]} ] ---"
 
-    if [[ -e "/var/guix" || -e "/gnu" ]]; then
+    if [[ -z $GUIX_ALLOW_OVERWRITE && (-e /var/guix || -e /gnu) ]]; then
         die "A previous Guix installation was found.  Refusing to overwrite."
+    else
+        _msg "${WAR}Overwriting existing installation!"
     fi
 
     cd "$tmp_path"
-    tar --extract --file "$pkg" && _msg "${PAS}unpacked archive"
-
     _msg "${INF}Installing /var/guix and /gnu..."
-    mv "${tmp_path}/var/guix" /var/
-    mv "${tmp_path}/gnu" /
+    tar --extract --file "$pkg" -C /
 
     _msg "${INF}Linking the root user's profile"
     mkdir -p ~root/.config/guix
-- 
2.38.1

[bug#60068] [PATCH v2 3/3] guix-install.sh: Directly exit in case of errors in chk_require.

[Maxim Cournoyer]

* 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 62d85e765a..ea10f35250 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

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

[Ludovic Courtès]

Hi,

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

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

[Ludovic Courtès]

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

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

Thanks,
Ludo’.

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

[Ludovic Courtès]

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

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

Thanks,
Ludo’.

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

[Maxim Cournoyer]

Hi Ludovic,

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

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

> 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

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

[Maxim Cournoyer]

Hi Ludo,

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

> Hi,
>
> Maxim Cournoyer <maxim.cournoyer@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.

Done, for the two patches mentioned above, leaving the
GUIX_ALLOW_OVERWRITE one open for further discussions.

-- 
Thanks,
Maxim

[bug#60068] [PATCH 1/2] guix-install.sh: Add GUIX_ALLOW_OVERWRITE environment variable.

[Maxim Cournoyer]

Hi Tobias and Ludovic,

Had you seen my reply below?  It seems you had perhaps misunderstood the
use case at hand?

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

> Hi Tobias,
>
> Tobias Geerinckx-Rice <me@tobias.gr> writes:
>
>> Hi Maxim,
>>
>> Nice!  More steps towards world domination.
>
> Eh :-)
>
>> Maxim Cournoyer 写道：
>>> +# Environment variables
>>
>> These sound like they should be command-line arguments.
>
> I agree, but that'd require a loop, or GNU getopt, and I'm not motivated
> enough in the moment to re-design it :-).  When we get there, we could
> add an --uninstall option too.
>
>> […]
>>
>> Actually, I'm not totally sold on GUIX_ALLOW_OVERWRITE.  It's not
>> solving much a problem.
>
> The problem it solves for me was that I needed to use 'guix pack'd
> dependencies such as gpg, glibc for getent, and shadow's
> groupadd/useradd, etc. to satisfy the install script dependencies on my
> weird target OS (minimal busybox embedded OS); which are needed to be
> unpacked under /gnu, thus conflicting with the requirement that /gnu
> doesn't exist.
>
> I tried a relocatable pack, but it didn't work, at least for gpg (file
> not found error).

-- 
Thanks,
Maxim