tr: Illegal byte sequence on macOS when installing JRuby

[eregon]

tr: Illegal byte sequence on macOS when installing JRuby

[eregon]

@mislav Thank you for the super quick fix!

I'd like to keep this open to track the 2 leftover items:

Also, more problematic is that even though it says BUILD FAILED the exit code seems 0 and so GitHub Actions continues (I checked and I don't have continue-on-error at all in that workflow).

I suppose this is due to the position of the tr in if [ "$(head -c 20 "$file" | LC_CTYPE=C tr -d '\0')" = "#!/usr/bin/env jruby" ]; then? Maybe we can somehow fail in that case if tr fails?
Given we do see BUILD FAILED I guess that error is somehow half-propagated, but not the exit status.
Or maybe the problem is in

ruby-build/bin/ruby-build

Line 779 in c3bae8b

fix_jruby_shebangs

and it should be fix_jruby_shebangs || return 1 and the same for function calls above?

Finally I think there is no point to install ri/rdoc for jruby-launcher, so it would be best to gem install --no-document jruby-launcher.

#2281

[mislav]

I suspect that BUILD FAILED happened due to the bash ERR trap function propagating into sub-shells (that includes command-capturing expressions) and one of those subshells failing due to tr exiting with nonzero. The code in question didn't have a guard against tr failing because it was never meant to fail. The fact it did was a bug, which is now fixed, and so I do not think we need a || return 1 construct.

In the "main thread" of ruby-build execution, the ERR trap will mostly ensure that the ruby-build process exits with a nonzero status. However, if the BUILD FAILED accidentally happens in a non-fatal sub shell, ruby-build will exit with 0 status because it didn't technically fail. I think that your JRuby build succeeded even if it printed "BUILD FAILED". Of course, that is a bug which I now hope is fixed.

Bash error handling (either through the ERR trap or through set -e) is arcane and difficult to get right. However, I do not think that the solution should be to append || return 1 to every line of code. Instead, command-capturing expressions that anticipate that the last command in a pipe could potentially exit with nonzero should append || true to avoid having them inherit the ERR behavior.

[eregon]

OK, thanks for the explanation, let's close this then.

[joerixaop]

I still had this issue after this patch, it turns out that I set LC_ALL in my profile (I shouldn't have done that). LC_ALL overrides LC_CTYPE so tr was still assuming UTF-8 input.

I'm adding this comment just in case anyone else is confused about still seeing the issue, double check whether LC_ALL is set to something, and see if the issue still persists after unsetting LC_ALL (and stop setting LC_ALL In your profile if you did so)

[mislav]

@joerixaop Thanks for reporting. That's a new bug and you're welcome to report it as such! Even better, you could open a PR that blanks out LC_ALL when invoking tr.

[eregon]

Looking at https://github.com/rbenv/ruby-build/pull/2280/files, why do we use tr at all there?
There shouldn't be any \0 byte in a shebang line, isn't it? So why trying to remove them?
locale env vars are notorious for being brittle unfortunately (if we need to set anything we should set LC_ALL around the tr).

[mislav]

There shouldn't be any \0 byte in a shebang line, isn't it? So why trying to remove them?

We don't know for sure if an executable is a plain-text shim (or script) or an actually binary file. So this acts as a guard against binary files, because it appears that the [ builtin doesn't deal well when testing strings that have a null byte in them.

if we need to set anything we should set LC_ALL around the tr

Agreed!