GNU bug report logs - #64359
[PATCH] [RFC] --search-paths: emit code compatible with set -u

Previous Next

Full log

Message #8 received at 64359 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Zack Weinberg <zack <at> owlfolio.org>
Cc: 64359 <at> debbugs.gnu.org
Subject: Re: [bug#64359] [PATCH] [RFC] --search-paths: emit code compatible
 with set -u
Date: Wed, 11 Oct 2023 19:06:32 +0200

Hi Zack,

Zack Weinberg <zack <at> owlfolio.org> skribis:

> The shell script fragment emitted by the --search-paths option to
> ‘guix shell’, etc., uses this construct to prepend a directory to
> a search-path environment variable:
>
>     export VAR="/gnu/store/...${VAR:+:}$VAR"
>
> If this is evaluated by a shell in set -u mode, and VAR was previously
> unset, it will error out:
>
>     $ bash -c '
>         set -u
>         unset PKG_CONFIG_PATH
>         export PKG_CONFIG_PATH="/example/lib/pkgconfig${PKG_CONFIG_PATH:+:}$PKG_CONFIG_PATH"
>         echo $PKG_CONFIG_PATH
>     '
>     bash: line 4: PKG_CONFIG_PATH: unbound variable
>
> This happens in real life, for instance, if direnv[1] is being used in its
> “strict_env” mode[2], which the documentation says will become the default
> in a future release.
>
> This patch makes the code emitted by --search-paths compatible with set -u,
> by changing each use of bare `$VARIABLE` to `${VARIABLE:-}`, e.g.
>
>     $ bash -c '
>         set -u
>         unset PKG_CONFIG_PATH
>         export PKG_CONFIG_PATH="/example/lib/pkgconfig${PKG_CONFIG_PATH:+:}${PKG_CONFIG_PATH:-}"
>         echo $PKG_CONFIG_PATH'
>     /example/lib/pkgconfig

This change makes a lot of sense to me.


[...]

> +++ b/guix/build/utils.scm
> @@ -1384,19 +1384,19 @@ (define (export-variable lst)
>         (format #f "export ~a=\"~a\""
>                 var (string-join rest sep)))
>        ((var sep 'prefix rest)
> -       (format #f "export ~a=\"~a${~a:+~a}$~a\""
> +       (format #f "export ~a=\"~a${~a:+~a}${~a:-}\""
>                 var (string-join rest sep) var sep var))

This part is a full-rebuild change, so it’d have to wait.  However, it’s
within ‘wrap-program’; the script generated by ‘wrap-program’ does *not*
use ‘set -u’, so I think this change is unnecessary.  Am I right?

> +++ b/guix/search-paths.scm
> @@ -225,10 +225,10 @@ (define* (environment-variable-definition variable value
>      ('exact
>       (format #f "export ~a=\"~a\"" variable value))
>      ('prefix
> -     (format #f "export ~a=\"~a${~a:+~a}$~a\""
> +     (format #f "export ~a=\"~a${~a:+~a}${~a:-}\""
>               variable value variable separator variable))
>      ('suffix
> -     (format #f "export ~a=\"$~a${~a:+~a}~a\""
> +     (format #f "export ~a=\"${~a:-}${~a:+~a}~a\""
>               variable variable variable separator value))))

LGTM.

> --- a/tests/guix-environment.sh
> +++ b/tests/guix-environment.sh

You can remove this change and keep only the ‘tests/guix-shell.sh’ part.

Could you send an updated patch?

Thanks, and apologies for the long delay!

Ludo’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.