Package: guix-patches;
Reported by: Zack Weinberg <zack <at> owlfolio.org>
Date: Thu, 29 Jun 2023 23:12:01 UTC
Severity: normal
Tags: patch
To reply to this bug, email your comments to 64359 AT debbugs.gnu.org.
Toggle the display of automated, internal messages from the tracker.
View this report as an mbox folder, status mbox, maintainer mbox
guix-patches <at> gnu.org
:bug#64359
; Package guix-patches
.
(Thu, 29 Jun 2023 23:12:01 GMT) Full text and rfc822 format available.Zack Weinberg <zack <at> owlfolio.org>
:guix-patches <at> gnu.org
.
(Thu, 29 Jun 2023 23:12:01 GMT) Full text and rfc822 format available.Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
From: Zack Weinberg <zack <at> owlfolio.org> To: guix-patches <at> gnu.org Subject: [PATCH] [RFC] --search-paths: emit code compatible with set -u Date: Thu, 29 Jun 2023 19:10:56 -0400
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
guix-patches <at> gnu.org
:bug#64359
; Package guix-patches
.
(Wed, 11 Oct 2023 17:08:02 GMT) Full text and rfc822 format available.Message #8 received at 64359 <at> debbugs.gnu.org (full text, mbox):
From: Ludovic Courtès <ludo <at> gnu.org> To: Zack Weinberg <zack <at> owlfolio.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 19:06:32 +0200
Hi Zack, Zack Weinberg <zack <at> 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’.
guix-patches <at> gnu.org
:bug#64359
; Package guix-patches
.
(Wed, 11 Oct 2023 18:58:02 GMT) Full text and rfc822 format available.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
guix-patches <at> gnu.org
:bug#64359
; Package guix-patches
.
(Sat, 14 Oct 2023 17:05:02 GMT) Full text and rfc822 format available.Message #14 received at 64359 <at> debbugs.gnu.org (full text, mbox):
From: Ludovic Courtès <ludo <at> gnu.org> To: "Zack Weinberg" <zack <at> owlfolio.org> Cc: 64359 <at> debbugs.gnu.org Subject: Re: [bug#64359] [PATCH] [RFC] --search-paths: emit code compatible with set -u Date: Sat, 14 Oct 2023 19:04:17 +0200
Hi Zack, "Zack Weinberg" <zack <at> 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’.
guix-patches <at> gnu.org
:bug#64359
; Package guix-patches
.
(Wed, 18 Oct 2023 18:23:02 GMT) Full text and rfc822 format available.Message #17 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, 18 Oct 2023 14:21:34 -0400
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
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.