guix-install.sh: Run ’info guix’ needs ’info’

  • Open
  • quality assurance status badge
Details
2 participants
  • Maxim Cournoyer
  • Simon Tournier
Owner
unassigned
Submitted by
Simon Tournier
Severity
normal
S
S
Simon Tournier wrote on 17 Nov 2023 11:38
guix-install.sh: Run ’info guix’ needs ’info’
(address . bug-guix@gnu.org)
87a5rcekbk.fsf@gmail.com
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
M
M
Maxim Cournoyer wrote on 12 Nov 07:28 +0100
Re: bug#67241: guix-install.sh: Run ’info guix ’ needs ’info’
(name . Simon Tournier)(address . zimon.toutoune@gmail.com)(address . 67241@debbugs.gnu.org)
87h68d6k4e.fsf@gmail.com
Hi Simon,

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

Toggle quote (13 lines)
> 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
S
S
Simon Tournier wrote on 16 Nov 08:54 +0100
[PATCH] guix-install.sh: Add message about Info reader.
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
e8022e114a050910d879193b9ec317826f768278.1731743586.git.zimon.toutoune@gmail.com
* 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(-)

Toggle diff (57 lines)
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
M
M
Maxim Cournoyer wrote 7 days ago
(name . Simon Tournier)(address . zimon.toutoune@gmail.com)(address . 67241@debbugs.gnu.org)
87r068uyn5.fsf@gmail.com
Hi Simon,

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

Toggle quote (36 lines)
> * 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:

Toggle snippet (3 lines)
if type -P info >/dev/null then [...]; fi

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
S
S
Simon Tournier wrote 6 days ago
Re: bug#67241: [PATCH] guix-install.sh: Add message about Info reader.
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)(address . 67241@debbugs.gnu.org)
87ldwf7bml.fsf@gmail.com
Hi Maxim,

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

Toggle quote (14 lines)
>> +_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…

Toggle quote (3 lines)
> 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. :-)

Toggle quote (6 lines)
> 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.

Toggle quote (3 lines)
> 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. :-)

Toggle quote (4 lines)
> 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
M
M
Maxim Cournoyer wrote 3 days ago
(name . Simon Tournier)(address . zimon.toutoune@gmail.com)(address . 67241@debbugs.gnu.org)
87frmkkssn.fsf@gmail.com
Hi Simon,

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

Toggle quote (23 lines)
> 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 ;-)

Toggle quote (10 lines)
>> 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!

Toggle quote (9 lines)
>> 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.

Toggle quote (3 lines)
> 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).

Toggle quote (5 lines)
>> 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
?
Your comment

Commenting via the web interface is currently disabled.

To comment on this conversation send an email to 67241@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 67241
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch