makeWrapper: Fail loudly when misused #24944

ahmedtd · 2017-04-16T05:38:17Z

Motivation for this change

On Wednesday, I tried to run hexl-mode in emacs, and four days later, here I am.

This pull request does two things to makeWrapper and wrapProgram:

First, makeWrapper checks that the file it is trying to wrap is a normal, executable file. If not, it dies with a (sometimes) useful backtrace. This prevents us from trying to wrap insane things like directories and .pyc files.
Secondly, makeWrapper dies with a backtrace on unsupported arguments.

Additionally, there are some follow-on commits that resolve misuses of makeWrapper that I ran into while getting emacs to build.

This pull request resolves #24445 and #3277.

Open question: This pull request will convert a lot of currently-silent breakage into build failures. Will nox-review wip be sufficient to shake all of these issues out?

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.

mention-bot · 2017-04-16T05:38:19Z

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

ahmedtd · 2017-04-16T05:58:40Z

Hmmm, it looks like the set of packages that nox-review pr 24944 wants to build is a little large for me to be comfortable doing so from my laptop. If need be, I can spin up a VM somewhere to crank it out.

FRidh · 2017-04-16T07:11:54Z

I'm okay with the changes in pkgs/development/interpreters/python/wrapper.nix. I have no opinion on the other changes.

Mic92 · 2017-04-16T07:22:57Z

pkgs/build-support/setup-hooks/wrap-gapps-hook.sh

+    # Loop over files, without falling prey to filenames containing
+    # newlines, backslashes, or other horrible things.
+    find "${prefix}/bin" "${prefix}/libexec" -type f -executable -print0 \
+    | while IFS= read -r -d '' file; do


Are you sure -d '' is correct here and not -d $'\0'?

Ok. Probably works. I was just reading this FAQ, where variables were set in the loop, which is not the case here.

Mic92 · 2017-04-16T07:24:16Z

pkgs/development/interpreters/python/wrapper.nix

+      for srcstore in "''${srcstores[@]}"; do
+        if [[ -d "$srcstore"/bin ]]; then
+          find "$srcstore"/bin -type f -executable -print0 \
+          | while read -d "" srcfile; do


same question here regarding -d

The delimiter is probably ok, but I wonder why IFS is not unset here.

You're correct, according to that link, IFS should be set.

ahmedtd · 2017-04-18T21:06:16Z

Hmmm, it looks like nox-review chokes on this PR, because it tries to pull in lispPackages.hu.dwim.asdf, which doesn't exist (the correct attribute is lispPackages."hu.dwim.asdf").

The fundamental problem seems to be that nix-env doesn't properly quote that attribute when dumping it with -qa.

ahmedtd · 2017-04-19T16:48:22Z

According to nix issue 1342, dots are not allowed in attribute names. I guess I will open a PR to fix the packages identified there as being problematic.

ahmedtd · 2017-04-23T22:05:39Z

@volth, I would be fine adding that to this commit series, but I'm worried that making it affect more packages will make it even harder to merge.

What can I do to help move this PR forward?

peti

I am very much in favor of this modification. I'm not thrilled by the fact that seemingly unrelated changes are mixed up in one PR though.

peti · 2017-04-24T14:26:21Z

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

-        fi
-
-        if test "$p" = "--suffix" -o "$p" = "--prefix"; then
+        elif [[ ("$p" == "--suffix") || ("$p" == "--prefix") ]]; then


It's very unfortunate that unrelated changes likes this one are mixed into a PR that's supposed to be about makeWrapper, really. These changes needlessly increase the size of the patch and make it harder to review, which is going to delay a successful merge (or even prevent it from happening altogether). 😞

I don't think this increases the size of the rebuild... the PR would include that first hunk (calling die for improper targets) regardless, so the rebuild set already includes all packages using makeWrapper.

I can split the argument strictness commit into a separate PR, though.

I am not talking about rebuilds. I'm talking about the size of the patch that constitutes this PR i.e. the diff you're suggesting be committed to Nixpkgs.

Sure, it can be split out

peti · 2017-04-24T14:28:58Z

pkgs/build-support/setup-hooks/wrap-gapps-hook.sh

-    for i in $prefix/bin/* $prefix/libexec/*; do
-      echo "Wrapping app $i"
-      wrapProgram "$i" "${gappsWrapperArgs[@]}"
+    # Loop over files, without falling prey to filenames containing


I don't believe the problem this change attempts to fix actually exists:

$ touch "this is a test" $ for n in *this*; do echo "$n"; done this is a test

Before, it was attempting to wrap all files in $prefix/bin and $prefix/libexec, including non-executable files and directories. Additionally, it ignored files in subdirectories of these roots.

emacs, in particular, was broken because of the attempt to wrap directories.

Well, then maybe the comment is misleading since it suggests the change is intended to fix handling of file names that contain white space? Am I missing something?

I agree, the comment is misleading. I will remove it.

peti · 2017-04-24T14:30:46Z

pkgs/development/interpreters/python/wrapper.nix

-              rm -f "$out/bin/$prg"
-              makeWrapper "$path/bin/$prg" "$out/bin/$prg" --set PYTHONHOME "$out"
-            fi
+      # Generate new wrapper scripts for the all the executables found in our


I don't understand why this modification is a part of this PR. Is this change necessary after the modification of makeWrapper? Does it depend on the change of makeWrapper? If either of these answers is "no", then this modification should be a separate PR.

It is necessary --- before, this code would try to wrap anything at top level in $path/bin, including non-executable files (in this case, .pyc files) and directories.

Sorry, it wouldn't try to wrap directories, but it would ignore any files in subdirectories of $path/bin (strange, but some packages do this).

Could this change conceivably be applied to Nixpkgs without having the change to makeWrapper first?

Yes, it could. Is that your preferred route?

ahmedtd · 2017-04-24T16:37:26Z

OK, everything is separated. This PR now depends on #25185, for the die utility function.

peterhoeg · 2017-07-26T04:25:30Z

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

@@ -24,6 +24,10 @@ makeWrapper() {
    local params varName value command separator n fileNames
    local argv0 flagsBefore flags

+    # We should only be wrapping executable files.
+    [[ -f "${original}" && -x "${original}" ]] || \
+        die "makeWrapper cannot be invoked on ${prog} because it is not an executable file"


Shouldn't that be ${original} instead of ${prog} ?

peterhoeg · 2017-07-26T04:26:03Z

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

+    # We should only be wrapping executable files.
+    [[ -f "${prog}" && -x "${prog}" ]] || \
+        die "wrapProgram cannot be invoked on ${prog} because it is not an executable file"
+


I would personally prefer moving this into a function to avoid the duplication.

peti

Once #24944 (comment) is addressed, this patch is ready for merging (into staging) IMHO.

ahmedtd · 2017-07-30T21:22:42Z

OK, the patch is rebased on staging and the check has been broken out into its own function, assertExecutable.

nox is broken right now, but I built a few packages that use makeWrapper and they seem to be fine.

peti · 2017-08-07T19:36:16Z

@ahmedtd, we cannot merge this patch without #25185, and that PR has had merge conflicts and hasn't seen any update since a long time. I would really like to see both PRs merged, like, soon, but I need your help to get that done. If we cannot make progress with these patches in the next couple of days, then I am inclined to declare this a lost cause and to close both PRs.

ahmedtd · 2017-08-07T19:42:53Z

Oh, it looks like I missed the activity on the other PR. I will update it tonight.

…

On Aug 7, 2017 12:36 PM, "Peter Simons" ***@***.***> wrote: @ahmedtd <https://github.com/ahmedtd>, we cannot merge this patch without #25185 <#25185>, and that PR has had merge conflicts and hasn't seen any update since a long time. I would really like to see both PRs merged, like, soon, but I need your help to get that done. If we cannot make progress with these patches in the next couple of days, then I am inclined to declare this a lost cause and to close both PRs. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#24944 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA-FcpM4_t5XQb9E8A9olgfSZKIXs4rAks5sV2czgaJpZM4M-jBF> .

peti

I'll merge once that typo pointed out in https://github.com/NixOS/nixpkgs/pull/24944/files#r129479438 is fixed.

peti · 2017-08-08T06:35:51Z

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

@@ -24,6 +24,10 @@ makeWrapper() {
    local params varName value command separator n fileNames
    local argv0 flagsBefore flags

+    # We should only be wrapping executable files.
+    [[ -f "${original}" && -x "${original}" ]] || \
+        die "makeWrapper cannot be invoked on ${prog} because it is not an executable file"


Please address https://github.com/NixOS/nixpkgs/pull/24944/files#r129479438.

`makeWrapper` and `wrapProgram` are being invoked on all kinds of wacky things (usually with the help of bash globs or other machine assistance). So far, I have come across `wrapProgram` being invoked on a directory, as well as on the empty string. As far as I can tell, it's only valid to invoke these utilities on a normal (non-directory, non-device) executable file. This commit enforces that precondition.

ahmedtd · 2017-08-08T08:50:26Z

I'm sorry, it looks like I messed up my rebase on to staging. I had meant to address that in my last push to the branch.

The branch is now properly rebased on staging, and has the check broken out into an assertExecutable function.

peti · 2017-08-08T09:32:49Z

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

+# Assert that FILE exists and is executable
+#
+# assertExecutable FILE
+assertExecutable() {


What happens if the given path is a symlink to an executable?

It is handled properly. Cases I just tested:

non-executable file: Assertion failure

symlink to non-executable file: Assertion failure

executable file: Success

symlink to executable file: Success

traversable directory: Assertion failure

symlink to traversable directory: Assertion failure

FRidh · 2017-08-09T07:25:39Z

help2man:

Builder called die: Cannot wrap  because it is not an executable file
Backtrace:
7 assertExecutable /nix/store/rp2928gdi9x9n8ragk7804nf4msmhrr8-hook/nix-support/setup-hook
36 makeWrapper /nix/store/rp2928gdi9x9n8ragk7804nf4msmhrr8-hook/nix-support/setup-hook
143 wrapProgram /nix/store/rp2928gdi9x9n8ragk7804nf4msmhrr8-hook/nix-support/setup-hook
73 _callImplicitHook /nix/store/0ifjmsk5s6nqxz5wlkb2c78xf1213dkc-stdenv/setup
86 _eval /nix/store/0ifjmsk5s6nqxz5wlkb2c78xf1213dkc-stdenv/setup
36 runHook /nix/store/0ifjmsk5s6nqxz5wlkb2c78xf1213dkc-stdenv/setup
821 installPhase /nix/store/0ifjmsk5s6nqxz5wlkb2c78xf1213dkc-stdenv/setup
977 genericBuild /nix/store/0ifjmsk5s6nqxz5wlkb2c78xf1213dkc-stdenv/setup
2 main /nix/store/9krlzvny65gdc8s7kpb6lkx8cd02c25b-default-builder.sh

builder for ‘/nix/store/dy9xw4d9swsj5ynjkw2q2snz5n01hn3v-help2man-1.47.4.drv’ failed with exit code 1
error: build of ‘/nix/store/dy9xw4d9swsj5ynjkw2q2snz5n01hn3v-help2man-1.47.4.drv’ failed

https://nix-cache.s3.amazonaws.com/log/dy9xw4d9swsj5ynjkw2q2snz5n01hn3v-help2man-1.47.4.drv

FRidh · 2017-08-09T09:38:02Z

The same happens with wrapPython that is used by buildPythonPackage.
In this case its bootstrapped-pip that fails which is essential for building Python packages.

patching sources
configuring
no configure script, doing nothing
building
no Makefile, doing nothing
installing

Builder called die: Cannot wrap  because it is not an executable file
Backtrace:
7 assertExecutable /nix/store/rp2928gdi9x9n8ragk7804nf4msmhrr8-hook/nix-support/setup-hook
36 makeWrapper /nix/store/rp2928gdi9x9n8ragk7804nf4msmhrr8-hook/nix-support/setup-hook
143 wrapProgram /nix/store/rp2928gdi9x9n8ragk7804nf4msmhrr8-hook/nix-support/setup-hook
986 genericBuild /nix/store/0ifjmsk5s6nqxz5wlkb2c78xf1213dkc-stdenv/setup
2 main /nix/store/9krlzvny65gdc8s7kpb6lkx8cd02c25b-default-builder.sh

builder for ‘/nix/store/xv54v5jjdyb4q59my0sikdpy8mkpp2vq-python2.7-bootstrapped-pip-9.0.1.drv’ failed with exit code 1
cannot build derivation ‘/nix/store/6nnj7rishdc06gflj2j4pmi2aq9bx48g-python2.7-pytest-3.0.7.drv’: 1 dependencies couldn't be built
error: build of ‘/nix/store/6nnj7rishdc06gflj2j4pmi2aq9bx48g-python2.7-pytest-3.0.7.drv’ failed

The wrapper is defined at https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/interpreters/python/wrap.sh

peti · 2017-08-09T09:49:50Z

@ahmedtd, I wonder why those error message don't contain the path that was supposed to be wrapped?

FRidh · 2017-08-09T09:57:43Z

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

@@ -24,6 +33,8 @@ makeWrapper() {
    local params varName value command separator n fileNames
    local argv0 flagsBefore flags

+    assertExecutable "${file}"


I think this should be original instead of file

Oh, how embarrassing. Sorry.

In some cases wrappers could not be made. See e.g. NixOS#24944 (comment)

In some cases wrappers could not be made. See e.g. #24944 (comment)

Mic92 · 2017-08-11T14:01:43Z

Dropbox build is affected by this change:

Builder called die: Cannot wrap /nix/store/g7fnci9zvasw0jqb6a33f49hx72y58hy-dropbox-32.4.23/opt/dropbox/dropbox because it is not an executable file
Backtrace:
7 assertExecutable /nix/store/4j3i3dggkig9hy6mgf9iwraf1y932s1q-hook/nix-support/setup-hook
36 makeWrapper /nix/store/4j3i3dggkig9hy6mgf9iwraf1y932s1q-hook/nix-support/setup-hook
1001 genericBuild /nix/store/xa5a2jlryx7mwadpxg0zgn83rpf66qxy-stdenv/setup
2 main /nix/store/9krlzvny65gdc8s7kpb6lkx8cd02c25b-default-builder.sh

UPDATE: fixed in 516ba12

ahmedtd changed the title ~~Make makewrapper picky~~ makeWrapper: Fail loudly when misused Apr 16, 2017

FRidh added the 1.severity: mass-rebuild label Apr 16, 2017

Mic92 reviewed Apr 16, 2017

View reviewed changes

ahmedtd force-pushed the make-makewrapper-picky branch from b301ffa to 63c0813 Compare April 16, 2017 08:22

peti requested changes Apr 24, 2017

View reviewed changes

ahmedtd force-pushed the make-makewrapper-picky branch 2 times, most recently from 1639154 to 0ee7952 Compare April 24, 2017 16:31

ahmedtd force-pushed the make-makewrapper-picky branch from 0ee7952 to 5e15cf8 Compare April 24, 2017 16:41

FRidh mentioned this pull request Apr 24, 2017

python/wrapper.nix: Don't try to wrap pyc files #25184

Closed

7 tasks

grahamc added this to the 17.09 milestone Jul 26, 2017

peterhoeg reviewed Jul 26, 2017

View reviewed changes

peti approved these changes Jul 26, 2017

View reviewed changes

peti requested changes Jul 26, 2017

View reviewed changes

ahmedtd changed the base branch from master to staging July 30, 2017 18:41

ahmedtd force-pushed the make-makewrapper-picky branch from 5e15cf8 to 0f67517 Compare July 30, 2017 19:06

peti requested changes Aug 8, 2017

View reviewed changes

ahmedtd force-pushed the make-makewrapper-picky branch from 0f67517 to e1d46c0 Compare August 8, 2017 08:47

peti reviewed Aug 8, 2017

View reviewed changes

peti merged commit 67f37b7 into NixOS:staging Aug 8, 2017

ahmedtd mentioned this pull request Aug 9, 2017

Fix makewrapper unsupported args #25185

Merged

7 tasks

FRidh reviewed Aug 9, 2017

View reviewed changes

FRidh added a commit to FRidh/nixpkgs that referenced this pull request Aug 9, 2017

makeWrapper: fix regression introduced in NixOS#24944

4f4d905

In some cases wrappers could not be made. See e.g. NixOS#24944 (comment)

FRidh mentioned this pull request Aug 9, 2017

makeWrapper: fix regression introduced in #24944 #28067

Merged

8 tasks

globin pushed a commit that referenced this pull request Aug 9, 2017

makeWrapper: fix regression introduced in #24944

0ff782e

In some cases wrappers could not be made. See e.g. #24944 (comment)

globin added the 9.needs: changelog label Aug 11, 2017

disassembler mentioned this pull request Aug 12, 2017

system-config-printer wrapper broken #28175

Closed

steveej mentioned this pull request Oct 6, 2017

make-wrapper: add --no-assert to skip assertExecutable() #30161

Closed

8 tasks

jtojnar mentioned this pull request Dec 4, 2017

make-wrapper.sh should fail if unknown option is given #3277

Closed

orivej mentioned this pull request Dec 12, 2017

[WIP] makeWrapper: Use case not elif-chaining, and other cleanups #32552

Draft

8 tasks

makeWrapper: Fail loudly when misused #24944

makeWrapper: Fail loudly when misused #24944

Conversation

ahmedtd commented Apr 16, 2017 • edited

Motivation for this change

Things done

mention-bot commented Apr 16, 2017

ahmedtd commented Apr 16, 2017

FRidh commented Apr 16, 2017

Mic92 Apr 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmedtd commented Apr 18, 2017

ahmedtd commented Apr 19, 2017

ahmedtd commented Apr 23, 2017

peti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmedtd commented Apr 24, 2017 • edited by FRidh

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peti left a comment

Choose a reason for hiding this comment

ahmedtd commented Jul 30, 2017

peti commented Aug 7, 2017

ahmedtd commented Aug 7, 2017 via email

peti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmedtd commented Aug 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FRidh commented Aug 9, 2017

FRidh commented Aug 9, 2017

peti commented Aug 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mic92 commented Aug 11, 2017 • edited

ahmedtd commented Apr 16, 2017 •

edited

Mic92 Apr 16, 2017 •

edited

ahmedtd commented Apr 24, 2017 •

edited by FRidh

Mic92 commented Aug 11, 2017 •

edited