bug#67241: guix-install.sh: Run ’info guix’ needs ’info’

bug#67241: guix-install.sh: Run ’info guix’ needs ’info’

[Simon Tournier]

Hi,

The guix-install.sh script ends with the message:

    Run 'info guix' to read the manual.

And this works only if the foreign distribution has already installed an
’info’ reader.  That could not be the case.

I suggest to test if “type -P info“ returns something, then display the
message, else recommend to install an Info reader often named ’info’ or
’info-reader’ and display the message.

WDYT?

Cheers,
simon

bug#67241: guix-install.sh: Run ’info guix’ needs ’info’

[Maxim Cournoyer]

Hi Simon,

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

> Hi,
>
> The guix-install.sh script ends with the message:
>
>     Run 'info guix' to read the manual.
>
> And this works only if the foreign distribution has already installed an
> ’info’ reader.  That could not be the case.
>
> I suggest to test if “type -P info“ returns something, then display the
> message, else recommend to install an Info reader often named ’info’ or
> ’info-reader’ and display the message.

Sounds like a good idea to me.

-- 
Thanks,
Maxim

bug#67241: [PATCH] guix-install.sh: Add message about Info reader.

[Simon Tournier]

* etc/guix-install.sh (_info): New procedure.
(_chk_sys_nscd, main_install): Use it.

Change-Id: I2cad8bc2554cd4ea88f30c8a104b7c62f2aa2e0e
---
 etc/guix-install.sh | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/etc/guix-install.sh b/etc/guix-install.sh
index f07b2741bb..08e25de238 100755
--- a/etc/guix-install.sh
+++ b/etc/guix-install.sh
@@ -5,7 +5,7 @@
 # Copyright © 2018 Efraim Flashner <efraim@flashner.co.il>
 # Copyright © 2019–2020, 2022 Tobias Geerinckx-Rice <me@tobias.gr>
 # Copyright © 2020 Morgan Smith <Morgan.J.Smith@outlook.com>
-# Copyright © 2020 Simon Tournier <zimon.toutoune@gmail.com>
+# Copyright © 2020, 2024 Simon Tournier <zimon.toutoune@gmail.com>
 # Copyright © 2020 Daniel Brooks <db48x@db48x.net>
 # Copyright © 2021 Jakub Kądziołka <kuba@kadziolka.net>
 # Copyright © 2021 Chris Marusich <cmmarusich@gmail.com>
@@ -129,6 +129,16 @@ die()
     exit 1
 }
 
+_info()
+{
+    if [ "$(type -P info)" ]; then
+        _msg "$1"
+    else
+        _msg "${WAR}Please install Info reader; see package 'info-reader'"
+        _msg "$1"
+    fi
+}
+
 # Return true if user answered yes, false otherwise.  The prompt is
 # yes-biased, that is, when the user simply enter newline, it is equivalent to
 # answering "yes".
@@ -290,11 +300,11 @@ chk_sys_nscd()
     if [ "$(type -P pidof)" ]; then
         if [ ! "$(pidof nscd)" ]; then
             _msg "${WAR}We recommend installing and/or starting your distribution 'nscd' service"
-            _msg "${WAR}Please read 'info guix \"Application Setup\"' about \"Name Service Switch\""
+            _info "${WAR}Please read 'info guix \"Application Setup\"' about \"Name Service Switch\""
         fi
     else
         _msg "${INF}We cannot determine if your distribution 'nscd' service is running"
-        _msg "${INF}Please read 'info guix \"Application Setup\"' about \"Name Service Switch\""
+        _info "${INF}Please read 'info guix \"Application Setup\"' about \"Name Service Switch\""
     fi
 }
 
@@ -856,7 +866,7 @@ main_install()
     rm -r "${tmp_path}"
 
     _msg "${PAS}Guix has successfully been installed!"
-    _msg "${INF}Run 'info guix' to read the manual."
+    _info "${INF}Run 'info guix' to read the manual."
 
     # Required to source /etc/profile in desktop environments.
     _msg "${INF}Please log out and back in to complete the installation."

base-commit: 3e8d3d80f41e016cdfe80e488a78c2351c94fef8
-- 
2.45.2

bug#67241: [PATCH] guix-install.sh: Add message about Info reader.

[Maxim Cournoyer]

Hi Simon,

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

> * etc/guix-install.sh (_info): New procedure.
> (_chk_sys_nscd, main_install): Use it.

>
> Change-Id: I2cad8bc2554cd4ea88f30c8a104b7c62f2aa2e0e
> ---
>  etc/guix-install.sh | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/etc/guix-install.sh b/etc/guix-install.sh
> index f07b2741bb..08e25de238 100755
> --- a/etc/guix-install.sh
> +++ b/etc/guix-install.sh
> @@ -5,7 +5,7 @@
>  # Copyright © 2018 Efraim Flashner <efraim@flashner.co.il>
>  # Copyright © 2019–2020, 2022 Tobias Geerinckx-Rice <me@tobias.gr>
>  # Copyright © 2020 Morgan Smith <Morgan.J.Smith@outlook.com>
> -# Copyright © 2020 Simon Tournier <zimon.toutoune@gmail.com>
> +# Copyright © 2020, 2024 Simon Tournier <zimon.toutoune@gmail.com>
>  # Copyright © 2020 Daniel Brooks <db48x@db48x.net>
>  # Copyright © 2021 Jakub Kądziołka <kuba@kadziolka.net>
>  # Copyright © 2021 Chris Marusich <cmmarusich@gmail.com>
> @@ -129,6 +129,16 @@ die()
>      exit 1
>  }
>  
> +_info()
> +{
> +    if [ "$(type -P info)" ]; then
> +        _msg "$1"
> +    else
> +        _msg "${WAR}Please install Info reader; see package 'info-reader'"
> +        _msg "$1"
> +    fi
> +}

It seems odd to me to "overload" _msg into _info that deals with some side
effect; I'd rather see this conditional explicit at the message printing
site.

Also, your test is testing for the empty string when info is not found,
not the exist status, which is wrong.  I think you meant something like:

--8<---------------cut here---------------start------------->8---
if type -P info >/dev/null then [...]; fi
--8<---------------cut here---------------end--------------->8---

But this got me curious again... could we instead automate the
installation of info post-installation?  If yes, we should also automate
the installation of glibc-locales, using prompts that the user can
accept or decline like for the other configuration choices.

That'd be more useful than asking the user to manually install things
itself.

-- 
Thanks,
Maxim

bug#67241: [PATCH] guix-install.sh: Add message about Info reader.

[Simon Tournier]

Hi Maxim,

On Mon, 16 Dec 2024 at 11:53, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:

>> +_info()
>> +{
>> +    if [ "$(type -P info)" ]; then
>> +        _msg "$1"
>> +    else
>> +        _msg "${WAR}Please install Info reader; see package 'info-reader'"
>> +        _msg "$1"
>> +    fi
>> +}
>
> It seems odd to me to "overload" _msg into _info that deals with some side
> effect; I'd rather see this conditional explicit at the message printing
> site.

It was to avoid the duplication of the exact same conditional with the
exact same message.

I do not have an opinion…

> Also, your test is testing for the empty string when info is not found,
> not the exist status, which is wrong.

Please note that the script already uses:

     if [ "$(type -P pidof)" ]; then
         if [ ! "$(pidof nscd)" ]; then

And I have only respected the same. :-)

> not the exist status, which is wrong.  I think you meant something like:
>
> --8<---------------cut here---------------start------------->8---
> if type -P info >/dev/null then [...]; fi
> --8<---------------cut here---------------end--------------->8---

Well, I am not a Bash expert but I guess that’s the same result in
practise, no?

Both $() and "" used in tandem makes the test sound, from my
understanding.

> But this got me curious again... could we instead automate the
> installation of info post-installation?

It appears to me unrelated to this change at hand. :-)

>                                          If yes, we should also automate
> the installation of glibc-locales, using prompts that the user can
> accept or decline like for the other configuration choices.

Yeah why not, but let open another issue for tracking that, because
that’s not necessary straightforward since it’s on the top of different
distros.

Cheers,
simon

bug#67241: [PATCH] guix-install.sh: Add message about Info reader.

[Maxim Cournoyer]

Hi Simon,

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

> Hi Maxim,
>
> On Mon, 16 Dec 2024 at 11:53, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:
>
>>> +_info()
>>> +{
>>> +    if [ "$(type -P info)" ]; then
>>> +        _msg "$1"
>>> +    else
>>> +        _msg "${WAR}Please install Info reader; see package 'info-reader'"
>>> +        _msg "$1"
>>> +    fi
>>> +}
>>
>> It seems odd to me to "overload" _msg into _info that deals with some side
>> effect; I'd rather see this conditional explicit at the message printing
>> site.
>
> It was to avoid the duplication of the exact same conditional with the
> exact same message.
>
> I do not have an opinion…

Hm.  I agree duplication is not nice.  Probably a naming issue ;-)

>> Also, your test is testing for the empty string when info is not found,
>> not the exist status, which is wrong.
>
> Please note that the script already uses:
>
>      if [ "$(type -P pidof)" ]; then
>          if [ ! "$(pidof nscd)" ]; then
>
> And I have only respected the same. :-)

According to git blame these lines were also authored by you 4 years
ago, ha!

>> not the exist status, which is wrong.  I think you meant something like:
>>
>> --8<---------------cut here---------------start------------->8---
>> if type -P info >/dev/null then [...]; fi
>> --8<---------------cut here---------------end--------------->8---
>
> Well, I am not a Bash expert but I guess that’s the same result in
> practise, no?

It checks the exit status instead of the captured string output.  While
it's not that bad in that case, in general I find checking for the exit
status a much more reliable and clean option.

> Both $() and "" used in tandem makes the test sound, from my
> understanding.

Hm.  Is [ "something" ] true and [ "" ] false?  Apparently it is, but
I'd argue that's not very clear, especially when there are explicit test
operations to check for an non-empty or empty string (test -n and test
-z).

>> But this got me curious again... could we instead automate the
>> installation of info post-installation?
>
> It appears to me unrelated to this change at hand. :-)

It's related in that if the user opted to install 'info-reader' (on by
default), we wouldn't have to warn anything about, but yes, we can do so
later if you prefer, as I expect it's not that trivial.

I don't have strong feelings about the change as-is anymore, but I may
refactor the type -P checks to use the alternative style outlined above,
if you don't mind.

-- 
Thanks,
Maxim