make-wrapper: add __NIX_ARGV0 promise for interpreted shebang scripts #126248
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
"exec -a NAME" calls to interpreted shebang scripts are substituted as execve(shbangbin, [<evtl only shebang arg>, <scriptfile>]). As you can see, NAME is lost. `man execve` makes it clear (w.r.t. interpreted scripts): > Note that there is > no way to get the argv[0] that was passed to the execve() call. This commit adds a promise / contract to the make wrapper, so that scripts can have **at least a chance** of knowing what their author has originally had intended for argv[0] to be in the first place.
ofborg
bot
added
10.rebuild-darwin-stdenv
|
Result of 295 packages marked as broken and skipped:
25208 packages skipped due to time constraints:
145 packages built successfully:
Result of 214 packages marked as broken and skipped:
28445 packages skipped due to time constraints:
270 packages built successfully:
|
Member
|
@blaggacao talked about this for a while on Matrix, and (correct me if I am mistaken) we agreed to close it. One one hand, it is a shame that make-wrapper causes the On the other hand, this PR wasn't about the above issue, but trying to provide an out-of-band way to get make-wrapper-set |
Member
...unless the wrapped script is itself a wrapper script! I am running into this problem because Thunar happens to be wrapped twice (once by If we kept the proposed change and changed the bottom line of makeWrapper "$hidden" "$prog" --argv0 '${__NIX_ARGV0-$0}' "${@:2}"it would at least fix this issue. |
Contributor
Author
Bash scripts, themselves (being shebang script) suffer from the problem. The correct way, here too, would be a C-wrapper that interpretes shebang scripts appropriately (as we would expect them to be interpreted for the purpose of our wild usage of wrappers). However your use case might be a weight on the scale in favor of implementing such C wrapper. |
Member
Yes, otherwise we wouldn't have this issue. In my proposed solution, both wrappers would contain the lines export __NIX_ARGV0="${__NIX_ARGV0-$0}"
exec -a "${__NIX_ARGV0-$0}" ...so the first
Sure, but while we wait for that to happen, it might make sense to merge this 2-line fix. |
Contributor
Author
|
I know there is a history of work around in nixpkgs, but I agree with @Ericson2314 that accepting this PR would be akin to a layer violation. Maybe we can just take the unix implementation of It it is only one argument that should be handled differently from the current unix implementation of |
Member
|
The problem is that
|
Contributor
Author
|
It seems like bash behaves no different than python or any other script. The only difference I'm noticing is that I don't see the shebang substitution happening. It seems to be implicit. ➜ /tmp bat bash-test
───────┬────────────────────────────────────────────────────────────────────────────────────────────
│ File: bash-test
───────┼────────────────────────────────────────────────────────────────────────────────────────────
1 │ #! /bin/bash
2 │
3 │ echo I am $0
───────┴────────────────────────────────────────────────────────────────────────────────────────────
➜ /tmp ./bash-test
I am ./bash-test
➜ /tmp strace -s512 -e execve -f bash -c 'exec -a NAME ./bash-test'
execve("/usr/bin/bash", ["bash", "-c", "exec -a NAME ./bash-test"], 0x7ffed81a9710 /* 49 vars */) = 0
execve("/tmp/bash-test", ["NAME"], 0x5654fc755720 /* 49 vars */) = 0
I am /tmp/bash-test
+++ exited with 0 +++Interestingly the zeroth arguments gets skipped by ➜ /tmp bat bash-test
───────┬────────────────────────────────────────────────────────────────────────────────────────────
│ File: bash-test
───────┼────────────────────────────────────────────────────────────────────────────────────────────
1 │ #! /bin/sh
2 │
3 │ echo I am $1
───────┴────────────────────────────────────────────────────────────────────────────────────────────
➜ /tmp strace -s512 -e execve -f bash -c 'exec -a NAME ./bash-test 8'
execve("/usr/bin/bash", ["bash", "-c", "exec -a NAME ./bash-test 8"], 0x7ffd5f8c0b70 /* 49 vars */) = 0
execve("/tmp/bash-test", ["NAME", "8"], 0x55ebf403f720 /* 49 vars */) = 0
I am 8
+++ exited with 0 +++ |
Member
|
I'm not sure what you mean by "the zeroth argument gets skipped". In any case, the kernel certainly doesn't know about bash. |
Contributor
Author
Regardless, The only reasoning that I have for this observation is that bash gets evaluated by the stdenv (of whatever, execve?) and does not need any interpreter invokation, while it seems to actively mimic the behaviour observed during interpreter invocation. In conclusion, bash behaves no different, so the right solution (without "layer violation") shoudn't be any different. q.e.d. |
13 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
exec -a NAME-calls to interpreted shebang scripts are substitutedas
execve(shbangbin, [<evtl only shebang arg>, <scriptfile>]). As youcan see,
NAMEis lost.man execvemakes it clear (w.r.t. interpretedscripts):
This commit adds a promise / contract to the make wrapper, so that
scripts can have at least a chance of knowing what their author
has originally had intended for
argv[0]to be in the first place.Motivation for this change
my-script) instead of an "UX lie" (.my-script-wrapped).@name@substitution is unclean: it rebuilds (the script) for arbitrary wrappers however thin they areThings done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)