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

Previous Next

Full log

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

From: "Zack Weinberg" <zack <at> owlfolio.org>
To: Ludovic Courtès <ludo <at> gnu.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 14:56:55 -0400

On Wed, Oct 11, 2023, at 1:06 PM, Ludovic Courtès wrote:
>> 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.

Glad to hear!

>> +++ 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?

It's not strictly necessary to fix the bug, no.  I made this change because
it's the only other appearance of 'export VAR="additional-value${VAR:+:}$VAR"'
in the guix source code and I thought it would be better to change both of
them the same way. If only so that years from now someone doesn't waste any
time wondering why they're not quite the same and whether it matters.

Why is it a full-rebuild change?  As you point out, it should not actually
change anything?

>> --- a/tests/guix-environment.sh
>> +++ b/tests/guix-environment.sh
> You can remove this change and keep only the ‘tests/guix-shell.sh’ part.

I know "guix environment" is obsolete, but isn't it appropriate to test it
thoroughly for as long as it still exists?  (and again, years from now someone
might waste time wondering why this is only tested for "guix shell")

zw

Previous Next

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