Quote paths in makeWrapper #23511

abbradar · 2017-03-05T11:05:02Z

Motivation for this change

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
- Linux
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.

With this blink builds correctly. cc @rnhmjoj

mention-bot · 2017-03-05T11:05:04Z

@abbradar, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @Profpatsch and @vcunat to be potential reviewers.

bjornfor · 2017-03-05T11:07:43Z

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

-    local original=$1
-    local wrapper=$2
+    local original="$1"
+    local wrapper="$2"


FYI, someone pointed out to me once that these quotes:

local original="$1"

are unneeded. But I tend to add them anyway :-)

I suspected so because it was unquoted in most places here, but:

[abbradar@abbradarbook:~]$ test() { local a=a b c; echo $a; } [abbradar@abbradarbook:~]$ test a [abbradar@abbradarbook:~]$ test() { local a="a b c"; echo $a; } [abbradar@abbradarbook:~]$ test a b c

Eeeeeh, but:

[abbradar@abbradarbook:~]$ test() { local a=$1; echo $a; } [abbradar@abbradarbook:~]$ test "a b c" a b c

Bash is full of wonders...

Bash is full of wonders...

Indeed :-)

rnhmjoj · 2017-03-05T11:58:45Z

Thank you

Profpatsch · 2017-03-05T17:50:14Z

Would it be possible to add some tests for cases that failed before and are now fixed?

Profpatsch · 2017-03-05T17:48:30Z

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

-    mv $prog $hidden
-    makeWrapper $hidden $prog --argv0 '"$0"' "$@"
+    mv "$prog" "$hidden"
+    makeWrapper "$hidden" "$prog" --argv0 '$0' "$@"


Why remove the "" around $0 here? Doesn’t that change the expansion in some cases?

$argv0 is already quoted now; see the line above (which you've also commented).

Profpatsch · 2017-03-05T17:50:49Z

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

            n=$((n + 1))
        fi
    done

    # Note: extraFlagsArray is an array containing additional flags
    # that may be set by --run actions.
-    echo exec ${argv0:+-a $argv0} "$original" \
-         $flagsBefore '"${extraFlagsArray[@]}"' '"$@"' >> $wrapper
+    echo exec ${argv0:+-a \"$argv0\"} \"$original\" \


Maybe also add tests for non-trivial lines like these.

abbradar · 2017-03-05T17:52:10Z

I'm not sure where nixpkgs tests are supposed to go but we definitely want to run tests for this, with as evil paths as possible :3

Profpatsch · 2017-03-05T21:32:37Z

Sadly nowhere yet, but I might have something in the pipeline about that. ;)

grahamc · 2017-03-06T22:34:51Z

you don't need evil test cases, use shellcheck :)

Can you run shellcheck against the script and see what it says?

Fixes NixOS#22962 (comment) Also run ShellCheck.

abbradar · 2017-03-07T13:28:06Z

@grahamc I feel ashamed always forgetting about this wonderful tool. Helped fix some more problems, thanks!

Fixes #22962 (comment) Also run ShellCheck. (cherry picked from commit 7ff6eec) For ZHF #23253 to fix e.g. blink.

vcunat · 2017-03-20T12:35:55Z

The commit broke firewall test and consequently blocks nixos-unstable*. I've got no idea why ATM (bisected). They fail with:

starting VDE switch for network 1
running the VM test script
error: Global symbol "$walled" requires explicit package name (did you forget to declare "my $walled"?) at (eval 15) line 1.
Global symbol "$walled" requires explicit package name (did you forget to declare "my $walled"?) at (eval 15) line 4.
Global symbol "$walled" requires explicit package name (did you forget to declare "my $walled"?) at (eval 15) line 5.
Global symbol "$walled" requires explicit package name (did you forget to declare "my $walled"?) at (eval 15) line 9.
Global symbol "$walled" requires explicit package name (did you forget to declare "my $walled"?) at (eval 15) line 16.
Global symbol "$walled" requires explicit package name (did you forget to declare "my $walled"?) at (eval 15) line 17.
Global symbol "$walled" requires explicit package name (did you forget to declare "my $walled"?) at (eval 15) line 20.
Global symbol "$walled" requires explicit package name (did you forget to declare "my $walled"?) at (eval 15) line 24.
Global symbol "$walled" requires explicit package name (did you forget to declare "my $walled"?) at (eval 15) line 1.
Global symbol "$walled" requires explicit package name (did you forget to declare "my $walled"?) at (eval 15) line 4.
Global symbol "$walled" requires explicit package name (did you forget to declare "my $walled"?) at (eval 15) line 5.
Global symbol "$walled" requires explicit package name (did you forget to declare "my $walled"?) at (eval 15) line 9.
Global symbol "$walled" requires explicit package name (did you forget to declare "my $walled"?) at (eval 15) line 16.
Global symbol "$walled" requires explicit package name (did you forget to declare "my $walled"?) at (eval 15) line 17.
Global symbol "$walled" requires explicit package name (did you forget to declare "my $walled"?) at (eval 15) line 20.
Global symbol "$walled" requires explicit package name (did you forget to declare "my $walled"?) at (eval 15) line 24.
vde_switch: EOF on stdin, cleaning up and exiting
cleaning up

abbradar · 2017-03-20T12:52:58Z

There is some broken code in lib/testing.nix relying on old makeWrapper behaviour. I'm trying to fix that but stumbled into another Bashism:

$ foo=(a b c)
$ echo ${foo[@]}
a b c
$ echo "${foo[@]}"
$ # ??????

EDIT: no, this works (I can swear echo outputted nothing in the script but now it works -- maybe I typoed) -- something else is broken...

abbradar · 2017-03-20T13:08:16Z

Ugh, so:

foo=(a b c)
wrapProgram --add-flags "${foo[@]}"

results in an empty argument list. My best guess is that becase wrapProgram is a Bash function, not a program, Bash passes arrays instead of strings and they break inside (because code is obviously not suited to handle arrays).

I made a workaround at abbradar@4b36413 which fixes this problem but haven't yet pushed it -- maybe some Bash guru knows how to do this more elegantly? (I argue that elegance is a relative concept so it can be applied even to Bash)

abbradar · 2017-03-20T13:11:00Z

Oh, wait, I was just tricked by Bash again. "${foo[@]}" is interpreted as "expand an array into separate arguments" so my example above expands to wrapProgram a b c which breaks wrapProgram.

abbradar · 2017-03-20T13:16:38Z

So the correct way to do what I wanted to is ${foo[*]}. Fix is pushed to master (1f0ce0e) and release.

globin · 2017-03-27T15:19:36Z

This broke buildDotnetPackage see #24387

abbradar added the 0.kind: bug label Mar 5, 2017

abbradar added the 1.severity: mass-rebuild label Mar 5, 2017

bjornfor reviewed Mar 5, 2017

View reviewed changes

Profpatsch reviewed Mar 5, 2017

View reviewed changes

makeWrapper: quote paths

47af8ce

Fixes NixOS#22962 (comment) Also run ShellCheck.

abbradar force-pushed the makewrapper-quote branch from ce9306c to 47af8ce Compare March 7, 2017 13:27

rnhmjoj mentioned this pull request Mar 15, 2017

Zero Hydra Failures for 17.03 #23253

Closed

50 tasks

globin merged commit 7ff6eec into NixOS:staging Mar 16, 2017

vcunat pushed a commit that referenced this pull request Mar 19, 2017

makeWrapper: quote paths (#23511)

0bd37b4

Fixes #22962 (comment) Also run ShellCheck. (cherry picked from commit 7ff6eec) For ZHF #23253 to fix e.g. blink.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Quote paths in makeWrapper #23511

Quote paths in makeWrapper #23511

abbradar commented Mar 5, 2017

mention-bot commented Mar 5, 2017

bjornfor Mar 5, 2017

abbradar Mar 5, 2017 •

edited

abbradar Mar 5, 2017

bjornfor Mar 5, 2017

rnhmjoj commented Mar 5, 2017

Profpatsch commented Mar 5, 2017

Profpatsch Mar 5, 2017

abbradar Mar 5, 2017

Profpatsch Mar 5, 2017

abbradar commented Mar 5, 2017

Profpatsch commented Mar 5, 2017

grahamc commented Mar 6, 2017

abbradar commented Mar 7, 2017

vcunat commented Mar 20, 2017 •

edited

abbradar commented Mar 20, 2017 •

edited

abbradar commented Mar 20, 2017

abbradar commented Mar 20, 2017 •

edited

abbradar commented Mar 20, 2017

globin commented Mar 27, 2017

Quote paths in makeWrapper #23511

Quote paths in makeWrapper #23511

Conversation

abbradar commented Mar 5, 2017

Motivation for this change

Things done

mention-bot commented Mar 5, 2017

bjornfor Mar 5, 2017

Choose a reason for hiding this comment

abbradar Mar 5, 2017 • edited

Choose a reason for hiding this comment

abbradar Mar 5, 2017

Choose a reason for hiding this comment

bjornfor Mar 5, 2017

Choose a reason for hiding this comment

rnhmjoj commented Mar 5, 2017

Profpatsch commented Mar 5, 2017

Profpatsch Mar 5, 2017

Choose a reason for hiding this comment

abbradar Mar 5, 2017

Choose a reason for hiding this comment

Profpatsch Mar 5, 2017

Choose a reason for hiding this comment

abbradar commented Mar 5, 2017

Profpatsch commented Mar 5, 2017

grahamc commented Mar 6, 2017

abbradar commented Mar 7, 2017

vcunat commented Mar 20, 2017 • edited

abbradar commented Mar 20, 2017 • edited

abbradar commented Mar 20, 2017

abbradar commented Mar 20, 2017 • edited

abbradar commented Mar 20, 2017

globin commented Mar 27, 2017

abbradar Mar 5, 2017 •

edited

vcunat commented Mar 20, 2017 •

edited

abbradar commented Mar 20, 2017 •

edited

abbradar commented Mar 20, 2017 •

edited