tzselect.ksh: Use /bin/sh default shell interpreter

  checkbashism reports no issues with tzselect.ksh, therefore using
/bin/sh instead of /bin/bash should be safe and portable across systems
which don't ship bash ( embedded systems )

Signed-off-by: Khem Raj <raj.khem@gmail.com>
Cc: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Cc: Paul Eggert <eggert@cs.ucla.edu>
---
 timezone/tzselect.ksh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

On 12/15/21 21:52, Khem Raj wrote:
> checkbashism reports no issues with tzselect.ksh, therefore using
> /bin/sh instead of /bin/bash should be safe and portable across systems
> which don't ship bash ( embedded systems )

Didn't we already go through this? You proposed a better patch here:

https://sourceware.org/pipermail/libc-alpha/2021-December/133938.html

and I okayed it here:

https://sourceware.org/pipermail/libc-alpha/2021-December/133940.html


In contrast, the patch you just now emailed would change tzselect.ksh to 
just use /bin/sh. This would be an inferior patch because:

(1) tzselect.ksh should be taken verbatim from upstream as Joseph 
mentioned 
<https://sourceware.org/pipermail/libc-alpha/2021-December/133559.html>.

(2) tzselect.ksh has a better UI with Bash (or with ksh) than it does 
with a vanilla POSIX shell. It dynamically probes shell features and 
uses the Bash/ksh 'select' only if supported. Evidently this sort of 
dynamic probe is something that checkbashism is not smart enough to 
grok, so you shouldn't trust checkbashism on this script.
  

On Wed, Dec 15, 2021 at 10:27 PM Paul Eggert <eggert@cs.ucla.edu> wrote:
>
> On 12/15/21 21:52, Khem Raj wrote:
> > checkbashism reports no issues with tzselect.ksh, therefore using
> > /bin/sh instead of /bin/bash should be safe and portable across systems
> > which don't ship bash ( embedded systems )
>
> Didn't we already go through this? You proposed a better patch here:
>
> https://sourceware.org/pipermail/libc-alpha/2021-December/133938.html
>
> and I okayed it here:
>
> https://sourceware.org/pipermail/libc-alpha/2021-December/133940.html
>
>
> In contrast, the patch you just now emailed would change tzselect.ksh to
> just use /bin/sh. This would be an inferior patch because:
>
> (1) tzselect.ksh should be taken verbatim from upstream as Joseph
> mentioned
> <https://sourceware.org/pipermail/libc-alpha/2021-December/133559.html>.
>
> (2) tzselect.ksh has a better UI with Bash (or with ksh) than it does
> with a vanilla POSIX shell. It dynamically probes shell features and
> uses the Bash/ksh 'select' only if supported. Evidently this sort of
> dynamic probe is something that checkbashism is not smart enough to
> grok, so you shouldn't trust checkbashism on this script.

I am ok with either of the fixes since both are good for the usecase I have.
Adhemerval suggested to send this version as well. We can apply the previous one
too.
  

On Dez 15 2021, Paul Eggert wrote:

> (2) tzselect.ksh has a better UI with Bash (or with ksh) than it does with
> a vanilla POSIX shell. It dynamically probes shell features and uses the
> Bash/ksh 'select' only if supported. Evidently this sort of dynamic probe
> is something that checkbashism is not smart enough to grok, so you
> shouldn't trust checkbashism on this script.

So isn't it correct to use /bin/sh, since that can be a shell that does
support these features?  If you force the script to be executed by bash,
those runtime checks are useless, and should be removed.
  

On 16/12/2021 03:27, Paul Eggert wrote:
> On 12/15/21 21:52, Khem Raj wrote:
>> checkbashism reports no issues with tzselect.ksh, therefore using
>> /bin/sh instead of /bin/bash should be safe and portable across systems
>> which don't ship bash ( embedded systems )
> 
> Didn't we already go through this? You proposed a better patch here:
> 
> https://sourceware.org/pipermail/libc-alpha/2021-December/133938.html
> 
> and I okayed it here:
> 
> https://sourceware.org/pipermail/libc-alpha/2021-December/133940.html

I don't like this undocumented option that changes the installed script
through a Makefile variable.  The worse part is when users start to use
we won't be able to properly remove unless we actually fix the tzselect
script.

> 
> 
> In contrast, the patch you just now emailed would change tzselect.ksh to just use /bin/sh. This would be an inferior patch because:
> 
> (1) tzselect.ksh should be taken verbatim from upstream as Joseph mentioned <https://sourceware.org/pipermail/libc-alpha/2021-December/133559.html>.
> 
> (2) tzselect.ksh has a better UI with Bash (or with ksh) than it does with a vanilla POSIX shell. It dynamically probes shell features and uses the Bash/ksh 'select' only if supported. Evidently this sort of dynamic probe is something that checkbashism is not smart enough to grok, so you shouldn't trust checkbashism on this script.

We are trying to avoid hard code bash on internal scripts so I think it
is a fair assumption that if user want to use tzselect properly as a
system tool it should use set bash or compatible shell as default.
  

On 12/16/21 00:58, Andreas Schwab wrote:
> So isn't it correct to use /bin/sh, since that can be a shell that does
> support these features?

Yes, if /bin/sh supports 'select' then no UI functionality is lost by 
using /bin/sh. I expect, though, that installers want /bin/sh because 
they have only a stripped-down shell like Busybox that lacks support for 
'select'. In contrast, on distributions like Ubuntu where /bin/sh is 
dash and /bin/bash is available, we don't want /bin/sh here because dash 
lacks 'select'.

> If you force the script to be executed by bash,
> those runtime checks are useless, and should be removed.

Although that would be a valid optimization, I doubt whether the 
performance advantages would be worth the maintenance hassle.
  

On 12/16/21 02:21, Adhemerval Zanella wrote:

>> https://sourceware.org/pipermail/libc-alpha/2021-December/133938.html
> ...
> I don't like this undocumented option that changes the installed script
> through a Makefile variable.

Ah, I didn't know about the objection that it's undocumented. In that 
case, how about improving the patch to document the option? Presumably 
this documentation would be a comment and a configuration option in 
Makeconfig, in the style of the zonedir configuration option that's 
already present and is used when creating tzselect.

> The worse part is when users start to use
> we won't be able to properly remove unless we actually fix the tzselect
> script.

By "users" do you mean "end users" or "people who build glibc"? If the 
former, then I'm not following. If the latter, these users are expert 
and if we change the build procedure later we can notify them in the 
usual way. This approach has worked for the other ways that tzselect.ksh 
is edited into tzselect (e.g., zonedir, REPORT_BUGS_TO) and I don't see 
why it wouldn't work for shell selection too.

> We are trying to avoid hard code bash on internal scripts so I think it
> is a fair assumption that if user want to use tzselect properly as a
> system tool it should use set bash or compatible shell as default.

tzselect is not an internal script; it's installed into /usr/bin.

And again I don't know what the word "users" refers to here. Also, I 
don't know what "use set bash or compatible shell as default" means. The 
abovementioned patch would modify how the tzselect script is created, so 
that end users should get a shell that works and that is what their 
glibc builder wants them to use.
  

On 16/12/2021 14:06, Paul Eggert wrote:
> On 12/16/21 02:21, Adhemerval Zanella wrote:
> 
>>> https://sourceware.org/pipermail/libc-alpha/2021-December/133938.html
>> ...
>> I don't like this undocumented option that changes the installed script
>> through a Makefile variable.
> 
> Ah, I didn't know about the objection that it's undocumented. In that case, how about improving the patch to document the option? Presumably this documentation would be a comment and a configuration option in Makeconfig, in the style of the zonedir configuration option that's already present and is used when creating tzselect.

I think it is better than throwing a make variable.

> 
>> The worse part is when users start to use
>> we won't be able to properly remove unless we actually fix the tzselect
>> script.
> 
> By "users" do you mean "end users" or "people who build glibc"? If the former, then I'm not following. If the latter, these users are expert and if we change the build procedure later we can notify them in the usual way. This approach has worked for the other ways that tzselect.ksh is edited into tzselect (e.g., zonedir, REPORT_BUGS_TO) and I don't see why it wouldn't work for shell selection too.

I mean 'people who build glibc', since most likely which shell is used would
be transparent to 'end users' (we might even providing is a shell script).

What I want is to avoid *another* configure / make install option that 
glibc builders will need to take care.  For instance, latest ubuntu still
builds with options that had been removed a long time ago 
(--enable-add-ons=libidn and --enable-stackguard-randomization)

> 
>> We are trying to avoid hard code bash on internal scripts so I think it
>> is a fair assumption that if user want to use tzselect properly as a
>> system tool it should use set bash or compatible shell as default.
> 
> tzselect is not an internal script; it's installed into /usr/bin.

Yes I am aware and even there my comment apply.

> 
> And again I don't know what the word "users" refers to here. Also, I don't know what "use set bash or compatible shell as default" means. The abovementioned patch would modify how the tzselect script is created, so that end users should get a shell that works and that is what their glibc builder wants them to use.

I mean that a distribution that want to improve tzselect support 
would either patch it locally to use a better shell or make the
default shell bash or a supported one. 

Also, running tzselect with dash, the only layout difference seems
that each option is placed in one line (different than bash that 
place multiple options in one line). It is hard a deal breaker for
an options that will be run sporadically and still I prefer to avoid
bashism if possible.
  

On 12/16/21 10:25, Adhemerval Zanella wrote:
> I mean that a distribution that want to improve tzselect support
> would either patch it locally to use a better shell or make the
> default shell bash or a supported one.

If I understand you correctly, you're proposing to leave tzselect.ksh 
alone, and to put a comment and a configuration option in Makeconfig, 
and have that option be used to edit tzselect, and that the glibc 
default (in Makeconfig) to be /bin/sh rather than /bin/bash.

This would all work, except that the glibc default (in Makeconfig) 
should be /bin/bash not /bin/sh, for three reasons. First, this keeps 
the default the same as before and there is an advantage to not messing 
with things. Second, this makes it more likely for tzselect to use 
'select' which is significantly better user interface (see below). 
Third, the GNU C library should default to the GNU implementation of the 
shell.

> running tzselect with dash, the only layout difference seems
> that each option is placed in one line (different than bash that 
> place multiple options in one line). It is hard a deal breaker

No, using Bash is a significant user-visible improvement worth keeping. 
For example, under a Ubuntu terminal if I run 'tzselect' with bash and 
select the Americas, I get the first attached image. If I run it with 
dash I get the second. The bash-based tzselect is clear and easily 
usable, the dash-based tzselect is not.
  

On 16/12/2021 15:45, Paul Eggert wrote:
> On 12/16/21 10:25, Adhemerval Zanella wrote:
>> I mean that a distribution that want to improve tzselect support
>> would either patch it locally to use a better shell or make the
>> default shell bash or a supported one.
> 
> If I understand you correctly, you're proposing to leave tzselect.ksh alone, and to put a comment and a configuration option in Makeconfig, and have that option be used to edit tzselect, and that the glibc default (in Makeconfig) to be /bin/sh rather than /bin/bash.
> 
> This would all work, except that the glibc default (in Makeconfig) should be /bin/bash not /bin/sh, for three reasons. First, this keeps the default the same as before and there is an advantage to not messing with things. Second, this makes it more likely for tzselect to use 'select' which is significantly better user interface (see below). Third, the GNU C library should default to the GNU implementation of the shell.

I personally don't have a strong preference here, what I don't really like
is adding another make install option that adds even complexity in the code.
My suggestion to use /bin/sh as default is just to simplify both glibc and 
distros that do not want to pack 'bash'.  If a distro thinks 'select' is 
really a deal breaker, it can set its default to bash or whatever.

Regarding if should default to a GNU implementation, I really do not want to
enter in this topic.

> 
>> running tzselect with dash, the only layout difference seems
>> that each option is placed in one line (different than bash that place multiple options in one line). It is hard a deal breaker
> 
> No, using Bash is a significant user-visible improvement worth keeping. For example, under a Ubuntu terminal if I run 'tzselect' with bash and select the Americas, I get the first attached image. If I run it with dash I get the second. The bash-based tzselect is clear and easily usable, the dash-based tzselect is not.
But I am not suggesting change the code, I am suggestion changing the way
we distribute.  If distro stick with bash nothing change.

At same time I really think glibc is not the best place for such tool, 
specially for installed script that also calls awk (so users will also 
need to have it installed) and just change the a user define environment
variable.  If I would chose, my preference would be to just remove or pack
a simple non interactive C programs, as we do for other tools.
  

On 12/16/21 11:01, Adhemerval Zanella wrote:
> I really think glibc is not the best place for such tool

It does make sense to split off glibc-assistance tools like tzselect, 
zdump, zic, gencat, localedef, iconv, and sotruss into packages that can 
be omitted in embedded applications that don't need these niceties. 
Various distros do this already, albeit in different ways (e.g., Fedora 
glibc-common has gencat, Ubuntu libc-bin doesn't because gencat is in a 
separate package libc-dev-bin).

Of course any changes to glibc in this are would have to be coordinated 
with downstream; not sure it's worth the trouble.