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

  • Open
  • quality assurance status badge
Details
2 participants
  • Ludovic Courtès
  • Zack Weinberg
Owner
unassigned
Submitted by
Zack Weinberg
Severity
normal
Z
Z
Zack Weinberg wrote on 30 Jun 2023 01:10
(address . guix-patches@gnu.org)
877crlyidb.wl-zack@owlfolio.org
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.

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

Toggle diff (110 lines)
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
L
L
Ludovic Courtès wrote on 11 Oct 2023 19:06
(name . Zack Weinberg)(address . zack@owlfolio.org)(address . 64359@debbugs.gnu.org)
87bkd5t7fr.fsf@gnu.org
Hi Zack,

Zack Weinberg <zack@owlfolio.org> skribis:

Toggle quote (31 lines)
> 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.


[...]

Toggle quote (9 lines)
> +++ 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?

Toggle quote (13 lines)
> +++ 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.

Toggle quote (3 lines)
> --- 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’.
Z
Z
Zack Weinberg wrote on 11 Oct 2023 20:56
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 64359@debbugs.gnu.org)
b4cfa4bd-d50b-462b-8dcd-1c2846c788fa@app.fastmail.com
On Wed, Oct 11, 2023, at 1:06 PM, Ludovic Courtès wrote:
Toggle quote (12 lines)
>> 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!

Toggle quote (13 lines)
>> +++ 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?

Toggle quote (4 lines)
>> --- 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
L
L
Ludovic Courtès wrote on 14 Oct 2023 19:04
(name . Zack Weinberg)(address . zack@owlfolio.org)(address . 64359@debbugs.gnu.org)
87lec5dtke.fsf@gnu.org
Hi Zack,

"Zack Weinberg" <zack@owlfolio.org> skribis:

Toggle quote (22 lines)
>>> +++ 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.

Toggle quote (8 lines)
>>> --- 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’.
Z
Z
Zack Weinberg wrote on 18 Oct 2023 20:21
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 64359@debbugs.gnu.org)
7ab35fdd-f495-469e-b7fb-004faae6c74b@app.fastmail.com
On Sat, Oct 14, 2023, at 1:04 PM, Ludovic Courtès wrote:
Toggle quote (7 lines)
>> 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
?
Your comment

Commenting via the web interface is currently disabled.

To comment on this conversation send an email to 64359@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 64359
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch