* bug#67241: guix-install.sh: Run ’info guix’ needs ’info’ @ 2023-11-17 10:38 Simon Tournier 2024-11-12 6:28 ` Maxim Cournoyer 0 siblings, 1 reply; 6+ messages in thread From: Simon Tournier @ 2023-11-17 10:38 UTC (permalink / raw) To: 67241 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#67241: guix-install.sh: Run ’info guix’ needs ’info’ 2023-11-17 10:38 bug#67241: guix-install.sh: Run ’info guix’ needs ’info’ Simon Tournier @ 2024-11-12 6:28 ` Maxim Cournoyer 2024-11-16 7:54 ` bug#67241: [PATCH] guix-install.sh: Add message about Info reader Simon Tournier 0 siblings, 1 reply; 6+ messages in thread From: Maxim Cournoyer @ 2024-11-12 6:28 UTC (permalink / raw) To: Simon Tournier; +Cc: 67241 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#67241: [PATCH] guix-install.sh: Add message about Info reader. 2024-11-12 6:28 ` Maxim Cournoyer @ 2024-11-16 7:54 ` Simon Tournier 2024-12-16 2:53 ` Maxim Cournoyer 0 siblings, 1 reply; 6+ messages in thread From: Simon Tournier @ 2024-11-16 7:54 UTC (permalink / raw) To: Maxim Cournoyer; +Cc: Simon Tournier, 67241 * 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* bug#67241: [PATCH] guix-install.sh: Add message about Info reader. 2024-11-16 7:54 ` bug#67241: [PATCH] guix-install.sh: Add message about Info reader Simon Tournier @ 2024-12-16 2:53 ` Maxim Cournoyer 2024-12-16 17:58 ` Simon Tournier 0 siblings, 1 reply; 6+ messages in thread From: Maxim Cournoyer @ 2024-12-16 2:53 UTC (permalink / raw) To: Simon Tournier; +Cc: 67241 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#67241: [PATCH] guix-install.sh: Add message about Info reader. 2024-12-16 2:53 ` Maxim Cournoyer @ 2024-12-16 17:58 ` Simon Tournier 2024-12-19 7:58 ` Maxim Cournoyer 0 siblings, 1 reply; 6+ messages in thread From: Simon Tournier @ 2024-12-16 17:58 UTC (permalink / raw) To: Maxim Cournoyer; +Cc: 67241 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#67241: [PATCH] guix-install.sh: Add message about Info reader. 2024-12-16 17:58 ` Simon Tournier @ 2024-12-19 7:58 ` Maxim Cournoyer 0 siblings, 0 replies; 6+ messages in thread From: Maxim Cournoyer @ 2024-12-19 7:58 UTC (permalink / raw) To: Simon Tournier; +Cc: 67241 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-12-19 8:01 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-11-17 10:38 bug#67241: guix-install.sh: Run ’info guix’ needs ’info’ Simon Tournier 2024-11-12 6:28 ` Maxim Cournoyer 2024-11-16 7:54 ` bug#67241: [PATCH] guix-install.sh: Add message about Info reader Simon Tournier 2024-12-16 2:53 ` Maxim Cournoyer 2024-12-16 17:58 ` Simon Tournier 2024-12-19 7:58 ` Maxim Cournoyer
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/guix.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).