-
-
Notifications
You must be signed in to change notification settings - Fork 14.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
General fixupPhase patchShebangs strategy is in conflict with OS X (and BSD flavor) for wrapped interpreter #2146
Comments
Fix BSD! ;-) (Being more serious from now on.) If we keep using ".../bin/env" we must have wrappers that set $PATH correctly (unless we haven't really fixed the interpreter). I don't like that. But I understand breaking BSD/OS X isn't an option either. How about we change "/usr/bin/env python" to "/nix/store/...coreutils/bin/env /nix/store/...python/bin/python"? (Who said that "env" had to do $PATH lookup at all?) |
Yes. That's what I implied. To be precise,
To
(note that we still use python wrapper as an argument of env) |
Then why use |
@vizanto: Because sometimes they do use some flags or features of "env", and that causes breakage when patchShebangs replaces .../env foo with /path/to/foo. |
@bjornfor, |
@peti: When I said "breakage" I meant that patchShebangs bails out and prints an error. It seems there is a simple solution here, that stops patchShebangs from bailing out and makes it work on OSX (as per this issue). Simply patch |
The way I read it, the issue has nothing to do with additional arguments to env. It's concerned with the fact that scripts cannot be used as an interpreter of other scripts on BSD. That limitation shouldn't exist, btw, when those scripts are interpreted by bash. |
What about Of course, the change would only be done on platforms that need it. |
I don't see why we need to provide something "lighter" than |
My meaning was that on Linux (at least) coreutils might be a new runtime dependency in some cases. |
Some packages seem to have difficulties with the long (~128 byte) shebang that the above fix gives.
It snipped the path to bash :-( |
Haha, no, it's coreutils / env that snips the path:
|
Or wait... could it be the linux kernel? |
No, it's env:
|
I'd think it's the kernel, but it might be cut on multiple places at once. http://stackoverflow.com/questions/10813538/shebang-line-limit-in-bash-and-linux-kernel |
I believe /usr/bin/env bash should be replaced directly by nix-store/bash On Sat, Jul 18, 2015 at 12:59 PM, Vladimír Čunát [email protected]
NixOS Linux http://nixos.org |
Ah, right. |
@lethalman: That's what we have today. But according to this issue, it breaks on BSD (shebang interpreters cannot be shell scripts). |
Well, yes, it'd probably work for the "bash" case, but not for e.g. python (which is a wrapper script). |
FIXME: The linux kernel limits the shebang to 128 bytes ("#define BINPRM_BUF_SIZE 128"), which is too small for "/nix/store/.../env /nix/store/.../interpreter". Currently, something like "/usr/bin/env python" is changed into "/nix/store/<hash>/bin/python" ("env" is removed). This works in linux systems, but does not work on BSD flavored systems like OS X since they do not allow another shell script to play a role as an interpreter. (Interpreters built with nixpkgs are typically shell script wrappers for the real interpreter binaries.) The new behaviour is to rewrite "/usr/bin/env python" to "/nix/store/<hash>/bin/env /nix/store/<hash>/bin/python".
ping |
One option is to increase the kernel shebang buffer from 128 to 256 (or something) bytes. Then we can use |
Anyone brave enough to suggest upstream linux increase shebang buffer from 128 to 256 bytes? :-) |
(triage) I assume the problem hasn't changed in any way? |
Only if we can convince BSD people too! I have been trying to figure out a solution for this. I have potential two solutions, neither one very ideal:
|
You can't use more than 1 argument in a |
What's the behaviour of the BSD/Linux/Darwin kernels when the shebang line is too long? Does it silently truncate it or refuse to execute it? If it just truncates it we could write a small C program that ignores the actual argument in the shebang passed by the kernel and just reads it from the file itself (by opening
|
I ran into this issue on macOS and now I manually patch some of the stuff to go via From all of the above comments, I do not understand why rewriting This should still be less than 128 characters, and Since this is only an issue when the interpreter being called via the shebang line is itself a script, the I can submit a PR for this change, unless there is a reason why this solution does not work. |
AFAIK, |
Does the sandboxed building execute any of the scripts that goes through the Because if not, it would still be able to “build” them, they just wouldn’t run in the sandbox, but unless it’s related to running tests, I don’t see why the builder would run them. |
Right, the scripts themself would be built successfully in the sandbox (patchShebangs does not execute them). But if those scripts get used inside another build, then there is a problem. |
Thank you for your contributions.
|
Still an issue |
A stdenv derivation replaces shebang line in shell scripts during
fixupPhase
. Current strategy is that when this meets#!/usr/bin/env (interpreter command)
, it replaces the whole line with actual command for the interpreter. Therefore, for example, the first line of a python script starting withis changed to
This works in linux systems, but does not work on BSD flavored systems like OS X since they do not allow another shell script to play a role as an interpreter.
A relevant link is here:
http://www.in-ulm.de/~mascheck/various/shebang/#interpreter-script
I suggest to keep
/usr/bin/env
as/nix/store/...-coreutils-../bin/env
when being replaced bypatchShebangs
duringfixupPhase
The text was updated successfully, but these errors were encountered: