Fix makewrapper unsupported args #25185

ahmedtd · 2017-04-24T16:35:35Z

Motivation for this change

Previously, makeWrapper would accept arguments it didn't recognize,
potentially allowing argument misspellings or broken callers.

Now, makeWrapper dies with a backtrace if it is called incorrectly.

Also changes wrapProgram so that it doesn't pass through the first
argument twice --- this was tripping up the argument checking.

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-24T16:35:38Z

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

ahmedtd · 2017-04-24T16:44:47Z

#24944 depends on this, for the die utility function

Ericson2314 · 2017-04-24T17:17:38Z

Is this a mass rebuild? I'd think so.

ahmedtd · 2017-04-24T17:23:59Z

Yes. This PR was split from https://github.com/NixOS/nixpkgs/pull/24944, which might provide more context.

Currently, nox-review wip --against master chokes on it, since nixpkgs includes some problematic attribute names, which this change pulls in. See #25074 and NixOS/nix#1342 for the discussions.

I'm not sure how that will end up.

Ericson2314 · 2017-04-24T18:08:42Z

@ahmedtd Ok that's cool. I guess I don't know whether the policy is to submit known mass-rebuilds to staging or master. I'd guess staging, as that is where they'd go eventually, but I'm not sure.

ahmedtd · 2017-04-24T18:10:07Z

Oh, I thought commits went to staging after passing Hydra on master. Is there some documentation that lays out which branches are what?

…

On Apr 24, 2017 1:08 PM, "John Ericson" ***@***.***> wrote: @ahmedtd <https://github.com/ahmedtd> Ok that's cool. I guess I don't know whether the policy is to submit known mass-rebuilds to staging or master. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#25185 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA-FcgXlya4m1aCKr67k-3wuuJxuvf9gks5rzOUugaJpZM4NGaZN> .

Ericson2314 · 2017-04-24T18:11:22Z

I'm not sure, but the point of staging is to queue up mass rebuilds so they cancel out a bit for master. There is something about not just merging any old untested thing to staging, but I think that means test locally. This stuff indeed ought to be documented better :).

joachifm · 2017-04-24T21:42:21Z

Regarding commit policy, see https://nixos.org/nixpkgs/manual/#idm140737318460880

FRidh · 2017-04-25T06:53:12Z

I've changed the target branch to staging.

zimbatm · 2017-04-25T10:16:51Z

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

@@ -32,26 +32,20 @@ makeWrapper() {
    for ((n = 2; n < ${#params[*]}; n += 1)); do
        p="${params[$n]}"

-        if test "$p" = "--set"; then
+        if [[ "$p" == "--set" ]]; then


wouldn't it make sense to use a switch case here instead?

A previous PR that did something similar (#19061) ran up against the OSX system version of bash being too old to support the case statement.

fair, the fallthrough is only useful for a single pair of arguments which could be worked around: #19061 (comment)

Ok Bash 3.2 is not that bad. Or-patterns can be used instead of fall-through (and in fact are a better solution).

# Works in bash 3.2 case n in p|n) echo yup;; esac

zimbatm · 2017-04-25T10:18:15Z

pkgs/build-support/setup-hooks/die.sh

+    printf "\n"
+
+    exit 1
+}


How does it look like in practice when that happens? Nix's bash is quite convoluted and might have some weird stack trace.

It varies in usefulness, depending on how makeWrapper is being called. Sometimes there's enough information to be helpful, but often the stack trace stops at the point where stdenv/setup.sh evaluates the current phase hook.

peti · 2017-07-26T08:37:48Z

@ahmedtd, this PR has mere conflicts. Would you mind force-pushing an update that is re-based relative to current versions of staging?

Calling `die "Error message"` causes the current script to exit with an error, printing a backtrace

Previously, makeWrapper would accept arguments it didn't recognize, potentially allowing argument misspellings or broken callers. Now, makeWrapper dies with a backtrace if it is called incorrectly. Also changes `wrapProgram` so that it doesn't pass through the first argument twice --- this was tripping up the argument checking.

ahmedtd · 2017-08-08T05:18:53Z

Rebased onto current staging.

FRidh · 2017-08-08T18:58:59Z

This breaks several packages:

installing

Builder called die: makeWrapper doesn't understand the arg /homeless-shelter
Backtrace:
88 makeWrapper /nix/store/ixm1iljz3bgl04z4c2k2nvmq9wian9qh-hook/nix-support/setup-hook
129 wrapProgram /nix/store/ixm1iljz3bgl04z4c2k2nvmq9wian9qh-hook/nix-support/setup-hook
987 genericBuild /nix/store/0ifjmsk5s6nqxz5wlkb2c78xf1213dkc-stdenv/setup
2 main /nix/store/9krlzvny65gdc8s7kpb6lkx8cd02c25b-default-builder.sh

builder for ‘/nix/store/67l8nyc4b7rbyld7clby9lkibraphn9q-nix-prefetch-git.drv’ failed with exit code 1

https://hydra.nixos.org/build/58313566#tabs-summary

Ericson2314 · 2017-08-08T19:16:33Z

While we're at it, if someone could use use case with --suffix-contents | --prefix-contents) alternatives, per https://github.com/NixOS/nixpkgs/pull/25185/files#r129576600?

ahmedtd · 2017-08-08T20:03:40Z

This looks like preexisting brokenness in nix-prefetch-scripts. I can make a pr to fix it tonight.

…

On Aug 8, 2017 12:16 PM, "John Ericson" ***@***.***> wrote: While we're at it, if someone could use use case with --suffix-contents | --prefix-contents) alternatives, per https://github.com/NixOS/ nixpkgs/pull/25185/files#r129576600? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#25185 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA-FckAFay0fxnA6n3kfL8We81C_-ja_ks5sWLQXgaJpZM4NGaZN> .

globin · 2017-08-08T20:03:59Z

@ahmedtd fixed already in c00a95b

Ericson2314 · 2017-08-08T21:37:58Z

staging...obsidiansystems:make-wrapper-clean Here is a draft of that cleanup, if anyone wants to finish it.

FRidh · 2017-08-09T13:14:25Z

Just a note for a future improvement or someone else that encounters this.

The command die() is not accessible when using buildEnv. Therefore, when you wrap things from buildEnv you may get nix-support/setup-hook: line 7: die: command not found.

ahmedtd · 2017-08-09T15:51:41Z

@FRidh That will be a really frustrating experience to debug --- I will send a PR to replace die with its contents in make-wrapper.sh

ahmedtd · 2017-08-09T16:38:46Z

@FRidh Would you mind telling me in which file you observed this problem? Both die.sh and make-wrapper.sh are setup hooks, so I don't understand how one could be accessible but not the other.

globin · 2017-08-09T16:42:23Z

@ahmedtd this happens when using python.withPackages prior to 4495bfe

=> buildEnv with a bad makeWrapper in postInstall should make this reproducible.

FRidh · 2017-08-09T16:54:51Z

Indeed. An example that has scripts in /bin is python.withPackages(ps: [ps.sphinx]).

ahmedtd force-pushed the fix-makewrapper-unsupported-args branch 2 times, most recently from b867050 to a6f0fc8 Compare April 24, 2017 16:44

dezgeg added the 1.severity: mass-rebuild label Apr 24, 2017

FRidh changed the base branch from master to staging April 25, 2017 06:52

zimbatm reviewed Apr 25, 2017

View reviewed changes

ahmedtd mentioned this pull request Jun 4, 2017

makeWrapper: Fail loudly when misused #24944

Merged

7 tasks

peti added the 0.kind: enhancement label Jul 26, 2017

ahmedtd added 2 commits August 8, 2017 00:17

dieHook: Add die utility function

12354b8

Calling `die "Error message"` causes the current script to exit with an error, printing a backtrace

ahmedtd force-pushed the fix-makewrapper-unsupported-args branch from a6f0fc8 to ece5387 Compare August 8, 2017 05:18

peti merged commit b196230 into NixOS:staging Aug 8, 2017

jtojnar mentioned this pull request Dec 4, 2017

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix makewrapper unsupported args #25185

Fix makewrapper unsupported args #25185

ahmedtd commented Apr 24, 2017 •

edited

mention-bot commented Apr 24, 2017

ahmedtd commented Apr 24, 2017 •

edited by FRidh

Ericson2314 commented Apr 24, 2017

ahmedtd commented Apr 24, 2017

Ericson2314 commented Apr 24, 2017 •

edited

ahmedtd commented Apr 24, 2017 via email

Ericson2314 commented Apr 24, 2017

joachifm commented Apr 24, 2017 •

edited

FRidh commented Apr 25, 2017

zimbatm Apr 25, 2017

ahmedtd Apr 27, 2017

zimbatm Apr 27, 2017

Ericson2314 Jul 26, 2017 •

edited

zimbatm Apr 25, 2017

ahmedtd Apr 27, 2017

peti commented Jul 26, 2017 •

edited

ahmedtd commented Aug 8, 2017

FRidh commented Aug 8, 2017

Ericson2314 commented Aug 8, 2017

ahmedtd commented Aug 8, 2017 via email

globin commented Aug 8, 2017 •

edited

Ericson2314 commented Aug 8, 2017

FRidh commented Aug 9, 2017

ahmedtd commented Aug 9, 2017

ahmedtd commented Aug 9, 2017

globin commented Aug 9, 2017 •

edited

FRidh commented Aug 9, 2017

Fix makewrapper unsupported args #25185

Fix makewrapper unsupported args #25185

Conversation

ahmedtd commented Apr 24, 2017 • edited

Motivation for this change

Things done

mention-bot commented Apr 24, 2017

ahmedtd commented Apr 24, 2017 • edited by FRidh

Ericson2314 commented Apr 24, 2017

ahmedtd commented Apr 24, 2017

Ericson2314 commented Apr 24, 2017 • edited

ahmedtd commented Apr 24, 2017 via email

Ericson2314 commented Apr 24, 2017

joachifm commented Apr 24, 2017 • edited

FRidh commented Apr 25, 2017

zimbatm Apr 25, 2017

Choose a reason for hiding this comment

ahmedtd Apr 27, 2017

Choose a reason for hiding this comment

zimbatm Apr 27, 2017

Choose a reason for hiding this comment

Ericson2314 Jul 26, 2017 • edited

Choose a reason for hiding this comment

zimbatm Apr 25, 2017

Choose a reason for hiding this comment

ahmedtd Apr 27, 2017

Choose a reason for hiding this comment

peti commented Jul 26, 2017 • edited

ahmedtd commented Aug 8, 2017

FRidh commented Aug 8, 2017

Ericson2314 commented Aug 8, 2017

ahmedtd commented Aug 8, 2017 via email

globin commented Aug 8, 2017 • edited

Ericson2314 commented Aug 8, 2017

FRidh commented Aug 9, 2017

ahmedtd commented Aug 9, 2017

ahmedtd commented Aug 9, 2017

globin commented Aug 9, 2017 • edited

FRidh commented Aug 9, 2017

ahmedtd commented Apr 24, 2017 •

edited

ahmedtd commented Apr 24, 2017 •

edited by FRidh

Ericson2314 commented Apr 24, 2017 •

edited

joachifm commented Apr 24, 2017 •

edited

Ericson2314 Jul 26, 2017 •

edited

peti commented Jul 26, 2017 •

edited

globin commented Aug 8, 2017 •

edited

globin commented Aug 9, 2017 •

edited