makeWrapper: quote variables #31497

abbradar · 2017-11-10T19:29:11Z

Motivation for this change

Fix things like --prefix FOO ' ' 'bar baz'

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- other Linux distributions
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

orivej · 2017-11-10T19:40:03Z

Could you post an example of the resulting wrapper before and after this change?

abbradar · 2017-11-10T19:50:29Z

Before:

export NIX_CFLAGS_COMPILE=-isystem /nix/store/3cv0q9g9zyg5alzzz6072rhdmvzhk5yy-cuda-floatn.h/include${NIX_CFLAGS_COMPILE:+ }$NIX_CFLAGS_COMPILE

After:

export NIX_CFLAGS_COMPILE="-isystem /nix/store/3cv0q9g9zyg5alzzz6072rhdmvzhk5yy-cuda-floatn.h/include${NIX_CFLAGS_COMPILE:+ }$NIX_CFLAGS_COMPILE"

orivej · 2017-11-11T01:52:36Z

This swaps no quoting for approximately correct quoting. Please do not add double quotes, use "${value@Q}" and "${separator@Q}" instead of $value and $separator. The result will look like this:

export NIX_CFLAGS_COMPILE='-isystem /nix/store/3cv0q9g9zyg5alzzz6072rhdmvzhk5yy-cuda-floatn.h/include'${NIX_CFLAGS_COMPILE:+' '}$NIX_CFLAGS_COMPILE

abbradar · 2017-11-11T10:17:32Z

Excellent, thank you for the catch! To be honest I didn't know about @Q Bash feature before, it makes many things which I have in mind simpler.

Can you review this version? It passes my new test for me with the following wrapper:

export FOO='ahaha $ \'
export BAR='ahaha $ \'
export BAZ='ahaha $ \'
export QUZ='ahaha $ \'
export FOO='bar '\''$baz'${FOO:+' '}$FOO
export BAR=$BAR${BAR:+'' ''}'bar '\''$baz'
export BAZ=$'test1\n test3\ntest4'${BAZ:+$}$BAZ
export QUZ=$QUZ${QUZ:+$}$'test1\n test3\ntest4'

abbradar · 2017-11-11T10:25:22Z

@grahamc Bot forgot to remove old labels after a force-push.

abbradar · 2017-11-11T10:27:03Z

How can I quote @Q in a commit message so that to not ping @Q the member with it? @q, sorry for the noise!

EDIT: fixed it with a rewording for now.

orivej · 2017-11-11T13:06:34Z

pkgs/build-support/setup-hooks/make-wrapper.sh

@@ -63,9 +63,9 @@ makeWrapper() {
            n=$((n + 3))
            if test -n "$value"; then
                if test "$p" = "--suffix"; then
-                    echo "export $varName=\$$varName\${$varName:+$separator}$value" >> "$wrapper"
+                    echo "export $varName=\$$varName\${$varName:+'${separator@Q}'}${value@Q}" >> "$wrapper"


These ' should not be added.

Oops, leftover. Fixed.

This uses Bash ${foo@Q} feature to quote values properly, which allows us to handle values containing spaces, dollars etc. Thanks orivej for the idea!

orivej · 2017-11-11T13:10:29Z

To be honest I didn't know about @Q Bash feature before, it makes many things which I have in mind simpler.

I've learnt about it not long ago; before that I'd have suggested to use $(printf %q "$var") which achieves the same a bit more verbosely.

orivej

Now this looks good. (But I've not tested it.)

orivej · 2017-11-14T00:10:02Z

I have set up a Hydra cluster and tested this. Thank you!

orivej · 2017-11-17T09:47:55Z

Somehow I missed some failures… This is causing a bit of trouble in staging in cases such as --set CC "$""{CC:-$CC""}" and --set RETROFE_PATH "\${RETROFE_PATH:-\$PWD}". I'm going to introduce --set-default for the first kind of usage (set to a constant unless already set) and --set-eval for the latter (set to an arbitrary expression).
/cc @vcunat

After NixOS#31497 starter quoting all values, there arouse the need to left some values evaluated. `--set-default var value` expands to `export var=${var-value}`, where value is not evaluated and literally assigned to var unless it is already set. `--set-eval var value` expands to `export var=$(eval echo value)`, where value is evaluated by `eval`.

vcunat · 2017-11-18T09:11:46Z

The docs refer to the source files for such details, so I see no direct need to update the manual. It seems too low-level detail for release notes, too, not affecting any "normal" uses of the wrappers.

orivej · 2017-11-20T00:01:42Z

I've reviewed all usages of makeWrapper / wrapProgram in Nixpkgs and found 6 additional expressions that had to be updated in fb703ad (and eb3d207). It is not unlikely that when this reaches master there will be a couple runtime breakages invisible on Hydra and missed during the manual review.

infinisil · 2017-12-01T00:37:26Z

Well that explains why a few of my wrappers suddenly stopped working..

running pinta after NixOS#31497 results in: ``` Unhandled Exception: System.IO.FileNotFoundException: Could not load file or assembly 'glib-sharp, Version=2.12.0.0, Culture=neutral, PublicKeyToken=35e10195dab3c99f' or one of its dependencies. File name: 'glib-sharp, Version=2.12.0.0, Culture=neutral, PublicKeyToken=35e10195dab3c99f' [ERROR] FATAL UNHANDLED EXCEPTION: System.IO.FileNotFoundException: Could not load file or assembly 'glib-sharp, Version=2.12.0.0, Culture=neutral, PublicKeyToken=35e10195dab3c99f' or one of its dependencies. File name: 'glib-sharp, Version=2.12.0.0, Culture=neutral, PublicKeyToken=35e10195dab3c99f' ```

Fixes #33625 after #31497

Does not rebuild `mercurial`, only `mercurialFull`. Fixes #33625 after #31497

Fixes #33625 after #31497

abbradar added 0.kind: bug 1.severity: mass-rebuild labels Nov 10, 2017

GrahamcOfBorg added 10.rebuild-darwin: 501+ 10.rebuild-linux: 501+ 10.rebuild-darwin-stdenv labels Nov 10, 2017

abbradar mentioned this pull request Nov 11, 2017

Fixes for CUDA and Tensorflow update #31507

Merged

8 tasks

abbradar force-pushed the quote-makewrapper branch 2 times, most recently from 3387d98 to cfd9693 Compare November 11, 2017 10:15

GrahamcOfBorg added 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Nov 11, 2017

abbradar force-pushed the quote-makewrapper branch from cfd9693 to e842566 Compare November 11, 2017 10:26

abbradar force-pushed the quote-makewrapper branch from e842566 to 6020e18 Compare November 11, 2017 10:28

orivej reviewed Nov 11, 2017

View reviewed changes

makeWrapper: quote values properly

29c0591

This uses Bash ${foo@Q} feature to quote values properly, which allows us to handle values containing spaces, dollars etc. Thanks orivej for the idea!

abbradar force-pushed the quote-makewrapper branch from 6020e18 to 29c0591 Compare November 11, 2017 13:09

orivej approved these changes Nov 11, 2017

View reviewed changes

orivej merged commit cd0e2f5 into NixOS:staging Nov 14, 2017

orivej mentioned this pull request Nov 17, 2017

makeWrapper: add --set-default #31767

Merged

8 tasks

orivej mentioned this pull request Dec 1, 2017

fix makeWrapper args infinisil/nix-xdg#1

Merged

kamilchm mentioned this pull request Dec 8, 2017

pinta: FIX unhandled glib-sharp load exception #32445

Merged

8 tasks

orivej added a commit that referenced this pull request Jan 9, 2018

hplip: fix quoting for makeWrapper

fe05782

Fixes #33625 after #31497

orivej added a commit that referenced this pull request Jan 9, 2018

mercurial: fix quoting with guiSupport for makeWrapper

2e285a3

Does not rebuild `mercurial`, only `mercurialFull`. Fixes #33625 after #31497

orivej added a commit that referenced this pull request Jan 9, 2018

neovim: fix quoting for makeWrapper

f4afbf6

Fixes #33625 after #31497

This was referenced Jan 18, 2019

gstreamer: Plugins are not found #10559

Closed

gstreamer: fix plugin detection #54340

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

makeWrapper: quote variables #31497

makeWrapper: quote variables #31497

abbradar commented Nov 10, 2017

orivej commented Nov 10, 2017

abbradar commented Nov 10, 2017

orivej commented Nov 11, 2017 •

edited

Loading

abbradar commented Nov 11, 2017 •

edited

Loading

abbradar commented Nov 11, 2017

abbradar commented Nov 11, 2017 •

edited

Loading

orivej Nov 11, 2017

abbradar Nov 11, 2017

orivej commented Nov 11, 2017

orivej left a comment

orivej commented Nov 14, 2017

orivej commented Nov 17, 2017

vcunat commented Nov 18, 2017

orivej commented Nov 20, 2017

infinisil commented Dec 1, 2017

makeWrapper: quote variables #31497

makeWrapper: quote variables #31497

Conversation

abbradar commented Nov 10, 2017

Motivation for this change

Things done

orivej commented Nov 10, 2017

abbradar commented Nov 10, 2017

orivej commented Nov 11, 2017 • edited Loading

abbradar commented Nov 11, 2017 • edited Loading

abbradar commented Nov 11, 2017

abbradar commented Nov 11, 2017 • edited Loading

orivej Nov 11, 2017

Choose a reason for hiding this comment

abbradar Nov 11, 2017

Choose a reason for hiding this comment

orivej commented Nov 11, 2017

orivej left a comment

Choose a reason for hiding this comment

orivej commented Nov 14, 2017

orivej commented Nov 17, 2017

vcunat commented Nov 18, 2017

orivej commented Nov 20, 2017

infinisil commented Dec 1, 2017

orivej commented Nov 11, 2017 •

edited

Loading

abbradar commented Nov 11, 2017 •

edited

Loading

abbradar commented Nov 11, 2017 •

edited

Loading