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

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

[Zack Weinberg]

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

(Note: `${VARIABLE-}` would also be correct here.  When there are no
characters between the `-` and the `}`, `${VARIABLE:-}` and `${VARIABLE-}`
do exactly the same thing.  However, `${VARIABLE:+:}` and `${VARIABLE+:}`
are *not* equivalent, and the former is what we want.  Therefore I think
the code is easier to understand if we consistently use the : variant of
both substitution operators.)

For consistency I also made the same change to the code generated by
'wrap-program'.  (There's an argument for adding `set -euo pipefail`
to the top of those scripts, but that should probably be a separate patch.)

I tried to write tests to verify this new constraint, i.e. that the code
emitted by --search-paths works correctly in set -u mode.  The test suite
got stuck for upwards of six hours, apparently on tests that I didn’t
touch.  I _suspect_ this is a problem with my development machine, which is
a botched chimera of Guix and Debian at the moment, but I’d appreciate it
if someone could look carefully at the test code.  The code change itself
is straightforward.

Also, as far as I can tell, there is no existing package that causes
environment-variable-definition to be invoked with #:kind 'suffix, and
in fact I’m not sure it’s *possible* to write a package definition that
does that.  So that case is currently not being tested.

[1]: https://direnv.net/
[2]: https://direnv.net/man/direnv.toml.1.html#codestrictenvcode
---
 guix/build/utils.scm      |  8 ++++----
 guix/search-paths.scm     |  4 ++--
 tests/guix-environment.sh | 21 +++++++++++++++++++++
 tests/guix-shell.sh       | 20 ++++++++++++++++++++
 4 files changed, 47 insertions(+), 6 deletions(-)

diff --git a/guix/build/utils.scm b/guix/build/utils.scm
index 2352a627e9..f2a18d698d 100644
--- a/guix/build/utils.scm
+++ 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))
       ((var sep 'suffix rest)
-       (format #f "export ~a=\"$~a${~a+~a}~a\""
+       (format #f "export ~a=\"${~a:-}${~a:+~a}~a\""
                var var var sep (string-join rest sep)))
       ((var '= rest)
        (format #f "export ~a=\"~a\""
                var (string-join rest ":")))
       ((var 'prefix rest)
-       (format #f "export ~a=\"~a${~a:+:}$~a\""
+       (format #f "export ~a=\"~a${~a:+:}${~a:-}\""
                var (string-join rest ":") var var))
       ((var 'suffix rest)
-       (format #f "export ~a=\"$~a${~a:+:}~a\""
+       (format #f "export ~a=\"${~a:-}${~a:+:}~a\""
                var var var (string-join rest ":")))))
 
   (when (wrapped-program? prog)
diff --git a/guix/search-paths.scm b/guix/search-paths.scm
index fcbe7b7953..13c96ad692 100644
--- a/guix/search-paths.scm
+++ 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))))
 
 (define* (search-path-definition search-path value
diff --git a/tests/guix-environment.sh b/tests/guix-environment.sh
index 1424ea9a88..d0ffcd91f3 100644
--- a/tests/guix-environment.sh
+++ b/tests/guix-environment.sh
@@ -157,6 +157,27 @@ esac
 # in its profile (e.g., for 'gzip'), but we have to accept them.
 guix environment guix --bootstrap -n
 
+# The code emitted by '--search-paths', when adding to either end of a path
+# variable that might not be set to begin with, should not cause errors,
+# whether or not the variable was set, even if the shell is in `set -u` mode.
+# As far as I can tell, there are currently no packages that add to the end
+# of a path variable, so we cannot test that case.
+printf '%s\n' \
+       'set -u' \
+       'set -e' \
+       'unset PKG_CONFIG_PATH' \
+       'export PATH=/nonexistent' \
+       > "$tmpdir/c"
+guix environment guix --bootstrap -n --search-paths >> "$tmpdir/c"
+printf '%s\n' \
+       'echo "PATH=$PATH"' \
+       'echo "PKG_CONFIG_PATH=$PKG_CONFIG_PATH"' \
+       >> "$tmpdir/c"
+bash -x "$tmpdir/c" > "$tmpdir/d"
+grep -E '^PKG_CONFIG_PATH=/gnu/[^:]+$' "$tmpdir/d"
+grep -E '^PATH=/gnu/[^:]+:/nonexistent$' "$tmpdir/d"
+
+
 # Try program transformation options.
 mkdir "$tmpdir/emacs-36.8"
 drv="`guix environment --ad-hoc emacs -n 2>&1 | grep 'emacs.*\.drv'`"
diff --git a/tests/guix-shell.sh b/tests/guix-shell.sh
index ed368515eb..ec30a06288 100644
--- a/tests/guix-shell.sh
+++ b/tests/guix-shell.sh
@@ -41,6 +41,26 @@ guix shell --ad-hoc guile-bootstrap && false
 # Rejecting unsupported packages.
 guix shell -s armhf-linux intelmetool -n && false
 
+# The code emitted by '--search-paths', when adding to either end of a path
+# variable that might not be set to begin with, should not cause errors,
+# whether or not the variable was set, even if the shell is in `set -u` mode.
+# As far as I can tell, there are currently no packages that add to the end
+# of a path variable, so we cannot test that case.
+printf '%s\n' \
+       'set -u' \
+       'set -e' \
+       'unset PKG_CONFIG_PATH' \
+       'export PATH=/nonexistent' \
+       > "$tmpdir/c"
+guix shell -D guix --bootstrap -n --search-paths >> "$tmpdir/c"
+printf '%s\n' \
+       'echo "PATH=$PATH"' \
+       'echo "PKG_CONFIG_PATH=$PKG_CONFIG_PATH"' \
+       >> "$tmpdir/c"
+bash -x "$tmpdir/c" > "$tmpdir/d"
+grep -E '^PKG_CONFIG_PATH=/gnu/[^:]+$' "$tmpdir/d"
+grep -E '^PATH=/gnu/[^:]+:/nonexistent$' "$tmpdir/d"
+
 # Test approximately that the child process does not inherit extra file
 # descriptors.  Ideally we'd check there's nothing more than 0, 1, and 2, but
 # we cannot do that because (1) we might be inheriting additional FDs, for
-- 
2.40.1

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

[Ludovic Courtès]

Hi Zack,

Zack Weinberg <zack@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’.

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

[Zack Weinberg]

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

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

[Ludovic Courtès]

Hi Zack,

"Zack Weinberg" <zack@owlfolio.org> 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’.

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

[Zack Weinberg]

On Sat, Oct 14, 2023, at 1:04 PM, Ludovic Courtès wrote:
>> 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.

I'm sorry, I still don't understand why a change with no functional effect
on the packages themselves would require a full rebuild.

zw