guix-patches
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Zack Weinberg
Subject: [bug#64359] [PATCH] [RFC] --search-paths: emit code compatible with set -u
Date: Wed, 11 Oct 2023 14:56:55 -0400
User-agent: Cyrus-JMAP/3.9.0-alpha0-1019-ged83ad8595-fm-20231002.001-ged83ad85

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





reply via email to

[Prev in Thread] Current Thread [Next in Thread]