[bug#74962] [PATCH v3 4/5] etc/guix-install.sh: Remove 'which' commands from requirements.

[Maxim Cournoyer <maxim.cournoyer@gmail.com>]

Hi Simon,

Simon Josefsson <simon@josefsson.org> writes:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>>
>>> * etc/guix-install.sh (REQUIRE): Remove "which".  Add "nologin".
>>> (sys_create_build_user): Use 'type' instead of 'which'.
>>>
>>> Fixes: <https://issues.guix.gnu.org/74952>
>>> Reported-by: Simon Josefsson <simon@josefsson.org>
>>> Change-Id: I0675716bab3fc22d3289ee7af2cb0ab33a1cee71
>>
>> LGTM.
>
> Using 'type -P' is not POSIX and neither /bin/dash nor /bin/gash
> supports it.  It seems like a GNU bash extension.  Is that okay?

I think it's OK, since we currently mandate Bash, but we could use
'command -v the-command > /dev/null' for the same result, which *is*
POSIX, so perhaps we should use that instead.

> The snippet ends up in the manual as recommendations for users to run on
> different operating systems.  We may want to assume GNU bash to favor
> it, but I'm not sure if that is really helping users.
>
> If 'type -P' is used, shouldn't that really be 'type -fP' to avoid shell
> function expansion?  It isn't all that clear from the man page if -f is
> still needed for -P or not:
>
> https://manpages.debian.org/bookworm/bash/bash.1.en.html#type

I think we'd have to use -f if we want to guard against shell functions
being found instead; but maybe then it's simpler and clearer to just use
'command -v' as I mentioned above.

> Even so 'type' uses hashed names, do they survive sub-shell $()
> execution?  If type is to be used, maybe this should be:
>
>   $(hash -r nologin && type -Pf nologin)
>
> My suggestion was to use 'command -v nologin' which behaviour is
> standard POSIX /bin/sh.  I acknowledge that it has the trouble of
> expanding to an alias if the shell had 'nologin' aliases somehow
> (unlikely but not impossible).

I agree; I'll make the change.  Perhaps adjust the other 'type' usages
also (there was only 2).

>   $(unalias nologin; command -v nologin)
>
> It seems all of the options (which, type -P, command -v) has another
> unwanted property: if 'nologin' is not available in the path, these
> commands expand to the empty string, and that empty string gets passed
> to 'useradd -s STR -c ...' and the user gets an ugly error message about
> '-c' not being a proper shell.

Yuck.

> I wonder what all this solves compared to hard-coding "/" as the login
> shell for the guixbuild user?
>
> Here is source code for nologin, which we seem to make some effort to
> use - is this better than 'false'?
>
> https://github.com/shadow-maint/shadow/blob/master/src/nologin.c

It seems marginally better than using 'false' in that it logs something
to syslog when a login is attempted and fail :-).

> At least I'm happy nobody wants to keep using 'which'.
>
> I am sorry for the rabbit hole :)

Thanks for the comments.  I'll send a reworked version.

-- 
Thanks,
Maxim