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

From: Ludovic Courtès <[email-masked]>
Date: Sat, 14 Oct 2023 19:04:17 +0200

Hi Zack,

"Zack Weinberg" <[email-masked]> skribis:

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

It’s a full-rebuild change because every single package depends on (guix
build utils).  When we change it, we have to rebuild literally every
package.

>>> --- 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")

No, I think it’s unnecessary because the two share the same code.
Eventually we’ll merge the two tests (and remove ‘guix environment’,
someday).

Could you send an updated patch?

Thanks,
Ludo’.