-
-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
stdenv/setup.sh: fix consumeEntire in bash5 #108101
stdenv/setup.sh: fix consumeEntire in bash5 #108101
Conversation
Unfortunately, bash5 has changed how read -N 0 works, meaning that consumeEntire no longer works as expected. Compare: $ nix shell github:nixos/nixpkgs\?ref=nixos-20.03#bash -c bash -c 'read -N 0 contents < /bin/sh; echo $?' 1 $ nix shell github:nixos/nixpkgs\?ref=nixos-20.03#bash_5 -c bash -c 'read -N 0 contents < /bin/sh; echo $?' 0 Bash 5 no longer reaches EOF when nchars=0. I could not find this documented anywhere. The best substitute I can find is to provide the max read nchars value (2147483647). This appears to have the same effect as nchars=0 in practice.
Not that it really adds much here, but I looked this up in the bash source:
|
pkgs/stdenv/generic/setup.sh
Outdated
@@ -711,7 +711,7 @@ substituteStream() { | |||
|
|||
consumeEntire() { | |||
# read returns non-0 on EOF, so we want read to fail | |||
if IFS='' read -r -N 0 $1; then | |||
if IFS='' read -r -N 2147483647 $1; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment on this magic number perhaps?
I wonder (but have not extensively tested) if you can do something like |
suggested by @abathur
if ! IFS='' read -r -d $'\x04' $1; then | ||
echo "consumeEntire(): ERROR: Input null bytes, won't process" >&2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn’t actually reject null bytes as claimed, nor does it preserve them, since Bash is not capable of storing a null byte in a variable. How about -d ''
?
Please forgive me for coming a little late to the party. The other solution that was mentioned in this thread is using the I understand if you are busy. No need to feel any pressure to reply to this. Thanks for documenting this in the first place! (I was going insane through the changelogs trying to find why the behavior changed). |
@happysalada |
Thank you for the explanation! The idea behind the nullbyte thing is that perhaps there are hooks provided by users which might contain a null byte ? |
@happysalada This is used in the implementation of |
@andersk Thank you for the clarification! also if anybody is interested, I'm trying the bash upgrade in this PR #131681 |
Pure Bash is better, both for performance, and for minimizing unnecessary rebuilds—we wouldn’t want every moreutils upgrade to trigger a full rebuild of every package. And we only care about null bytes, which Bash is unable to handle correctly; we don’t care about invalid UTF-8, which it doesn’t have any particular issues with. |
@andersk Thank you for your answer. It makes sense Just a small update on this. The bash_5 update is stuck with an error that doesn't make sense to me (one of the nix tests failing). I'm going to try to get some more information on it. |
This was just merged to staging in #133495 |
Unfortunately, bash5 has changed how read -N 0 works, meaning that
consumeEntire no longer works as expected. Compare:
Bash 5 no longer reaches EOF when nchars=0. I could not find this
documented anywhere.
The best substitute I can find is to provide the max read nchars
value (2147483647). This appears to have the same effect as nchars=0
in practice.