patchShebangs: Allow multiple args #60559
Conversation
JohnAZoidberg
changed the title
JohnAZoidberg
changed the title
| rm "$timestamp" | ||
| fi | ||
| fi | ||
| done < <(find "$dir" -type f -perm -0100 -print0) |
There was a problem hiding this comment.
Could you just put find "$@" here? I think that could be more efficient
There was a problem hiding this comment.
Oh, that would remove the need for my loop, right?
There was a problem hiding this comment.
Yeah would just have to remove the [ -e "$dir" ] || return 0 part as well.
There was a problem hiding this comment.
Thanks! I tried the function out on its own and it seems to be doing the right things.
JohnAZoidberg
force-pushed
the
patchshebangs-multiple-args
branch
from
0712474
to
d12276b
Compare
|
Waiting for reviews before merging. |
| @@ -95,7 +94,7 @@ patchShebangs() { | |||
| rm "$timestamp" | |||
| fi | |||
| fi | |||
| done < <(find "$dir" -type f -perm -0100 -print0) | |||
| done < <(find "$@" -type f -perm -0100 -print0) | |||
There was a problem hiding this comment.
This change, combined with the removal of line 43, means the function will fail clumsily if no arguments are passed to it. But considering that the old version silently and surprisingly ignores nonexistent files, this could be considered an improvement. ;) However, since it's a change in behavior, might it cause build failures? Should we let them happen? (Serious question.)
There was a problem hiding this comment.
What about we check if there are no arguments supplied in line 43?
if [ $# -eq 0 ]; then
echo "No arguments supplied to patchShebangs" >&2
return 3141592 # Not sure which return code is best, maybe just 1
fiThere was a problem hiding this comment.
I made it exit with 0 like it was before so that existing scripts don't break.
It's tempting to think patchShebangs supports multiple arguments. Without this patch it just silently ignores all but the first. Now it patches the shebangs in all of its arguments. Fixes: NixOS#57695
JohnAZoidberg
force-pushed
the
patchshebangs-multiple-args
branch
from
d12276b
to
6ac979d
Compare
|
@matthewbauer Do you still approve? |
|
Lgtm, yep |
JohnAZoidberg
force-pushed
the
patchshebangs-multiple-args
branch
from
6ac979d
to
283ed6f
Compare
| [ -e "$dir" ] || return 0 | ||
| if [ $# -eq 0 ]; then | ||
| echo "No arguments supplied to patchShebangs" >&2 | ||
| exit 0 |
There was a problem hiding this comment.
This should be >0 so bash recognizes it as an error
There was a problem hiding this comment.
Why >0? That just pipes the string to a file with name "0"?
There was a problem hiding this comment.
Oops meant "greater than 0". That is non-zero code.
| local dir="$1" | ||
|
|
||
| header "patching script interpreter paths in $dir" | ||
| echo "patching script interpreter paths in $@" |
There was a problem hiding this comment.
I think header should be kept too
There was a problem hiding this comment.
But it just calls echo and is marked as obsolete. https://github.com/NixOS/nixpkgs/blob/master/pkgs/stdenv/generic/setup.sh#L115
Commit "patchShebangs: Allow for multiple arguments" 4a1e51f removed the check. We don't want to break existing usages so this introduces it again with a successful exit code.
JohnAZoidberg
force-pushed
the
patchshebangs-multiple-args
branch
from
283ed6f
to
7c3d752
Compare
Motivation for this change
It's tempting to think patchShebangs supports multiple arguments.
Without this patch it just silently ignores all but the first. Now it
patches the shebangs in all of its arguments.
Fixes: #57695
Obviously I haven't tested it on nixpkgs because it rebuilds everything. Please check whether there might be some edge cases that I forgot which could break with these changes.
I tried the function on its own (copied tothe
patchPhaseof a derivation) and it seems to work as intended.Seems like there's no docbook documentation for this function. I think I'll do that next.
Things done
sandboxinnix.confon non-NixOS)nix-shell -p nix-review --run "nix-review wip"./result/bin/)nix path-info -Sbefore and after)