Repository navigation
Fix: gor, sbt, hub, zip, has, R, and pipefail - #95
goto-dev-null wants to merge 4 commits into
Conversation
|
|
||
| sbt) | ||
| version=$( sbt about 2>&1 | grep -Eo "([[:digit:]]{1,4}\.){2}[[:digit:]]{1,4}" | head -1) | ||
| version=$( sbt --version 2>&1 | grep -Eo "([[:digit:]]{1,4}\.){2}[[:digit:]]{1,4}" | head -1) |
There was a problem hiding this comment.
sbt about is not ideal for this. As it was before, it did not actually find a version at all (or maybe it did if sbt is set up somehow? I know nothing about it) because this is the output:
⋊> ~ sbt about 01:33:24
[error] Neither build.sbt nor a 'project' directory in the current directory: /home/jade
[error] run 'sbt new', touch build.sbt, or run 'sbt --allow-empty'.
[error]
[error] To opt out of this check, create /home/jade/.config/sbt/sbtopts with:
[error] --allow-emptyAdding the --allow-empty flag is tempting, except when you try...
⋊> ~ sbt --allow-empty 01:33:26
[info] [launcher] getting org.scala-sbt sbt 1.12.3 (this may take some time)...Let's not "take some time"; sbt --version is much better suited.
⋊> ~ sbt --version 01:33:57
sbt runner version: 1.12.3
[info] sbt runner (sbt-the-shell-script) is a runner to run any declared version of sbt.
[info] Actual version of the sbt is declared using project/build.properties for each build.
| # Suppress null byte warnings from command substitution | ||
| if [[ "${BASH_VERSINFO[0]}" -ge 4 ]]; then | ||
| exec 2> >(grep -v "warning: command substitution: ignored null byte" >&2) | ||
| fi | ||
|
|
There was a problem hiding this comment.
This is the extra bit needed to solve the issue identified in #94, but it doesn't really make sense without looking at __dynamic_detect below.
| cmd_output=$(eval "${cmd}" "${params}" "2>&1" </dev/null) | ||
| status=$? | ||
| if [ ${status} -eq 0 ]; then | ||
| version=$(echo "$cmd_output" | grep -Eo "${REGEX_SIMPLE_VERSION}" | head -1) | ||
| else | ||
| version="" | ||
| fi |
There was a problem hiding this comment.
So #94 taught us that we have to close stdin or newer versions of bzip2 will hang. But even when you do do that, bzip2 stills spits some binary into stdin.
Unfortunately, subshells ($()) don't deal well with non-UTF-8 in stdin, meaning that attempting to do
cmd_output=$(eval "${cmd}" "${params}" "2>&1" </dev/null)
will cause bash to complain:
warning: command substitution: ignored null byte
Ideally we'd like to just do something like
cmd_output=$(eval "${cmd}" "${params}" "2>&1" </dev/null | tr -d '\0')
to remove the offending null byte, but alas, then we are back to tr causing $? to be 1 instead of 127.
The if/else could totally be made into a one-liner, but that makes it really hard to read imo, so I figured it is better to be explicit.
If you wanted to, it would be something like:
version=$([ ${status} -eq 0 ] && echo "$cmd_output" | grep -Eo "${REGEX_SIMPLE_VERSION}" | head -1 || echo "")So the solution I settled on is what is up at the top of the file, some weird-ass way to tell bash to suppress that specific warning message. That chunk could easily be moved down to inside __dynamic_detect, it just only technically needs to be run once in the script, and it feels to me kinda like set -o pipefail, which generally one puts at the top of the file.
|
Hey @goto-dev-null — thanks for this, sorry it sat unreviewed so long. I reviewed it with some help from claude.
The __dynamic_detect, gor, sbt, hub, zip, and R changes are all good — I rebased them onto current master locally and the full test suite + shellcheck pass clean. The branch just predates a few things that landed on master since February, so a rebase will now conflict on:
One more thing I noticed while checking this: openssl and ssh use the exact same "pipe through grep/cut, trust the tail exit code" pattern as the old hub/zip/gor, so they likely have the same latent bug — didn't want to scope-creep your PR, but flagging in case you want a follow-up. If you can rebase onto master and resolve those four spots, this should be good to merge. Appreciate the detailed write-up, made this easy to verify. |
Note that `set -o pipefail` is NOT inherited by subshells so any time $() is used, it would have to be reenabled in each subshell The fact that all the commands that use grep exit with 127 when the program doesn't exist is kinda just sheer luck. No such luck with sed.
which was removed in 92b37d3
9b6803f to
193a603
Compare
|
Thanks for the response, appreciate it! :D I performed the rebase & did not see conflicts for any of those 4 things so I think we should be good. 👍 Ooh, good catch on openssl and ssh! And Claude caught that sbt has it too; I fixed the command it used but missed it suffering the same pipefail bug. I appreciate the consideration on scope creep. Looking back I actually think this entire PR could have (and perhaps should have) been split since it's addressing several issues:
So if you're okay with it, I don't at all mind bringing in the fixes for openssl and ssh. Or I would also understand a request to split this. 😅 I defer to your organizational preference. |
2 participants
Apologies if this is a lot for one PR. Just a lot of fixes bugs I encountered whilst working on my little Rust port ^^
pipefail
This issue affects multiple programs, primarily being falsely identified as present when they should not be.
set -o pipefailis not inherited by subshells ($()) so it isn't accomplishing what the comment on line 6 said it was. Unfortunately I don't think there's a way to tell bash to make it be inherited, which means it would have to be added for every single individual subshell isntance.Additionally,
$?does not behave like one would think; bash assigns it as the rightmost non-zero exit status in the pipeline.For example, say
zipisn't present when runningzip -v 2>&1 | sed -n 2p | grep -Eo "${REGEX_SIMPLE_VERSION}" | head -1:zip: 127 ✗sed: 0 ✓grep: 1 ✗ ← This is the rightmost failurehead: 0 ✓Hence
$?will contain 1, not 127.This only appeared in commands that used
sed; all the ones that just usegrepexited with 127, but just kinda by sheer luck (i.e. idfk), not because ofset -e pipefail. That being said, if it works, it works, so for the ones just with grep, I left them as is.(What you'd want to do, I think, is instead use
$PIPESTATUSwhich actually captures what we want: the status of the leftmost command in the pipeline. Unfortunately, this again falls victim to subshells; if you do a pipeline in a subshell,$PIPESTATUSis only defined within that subshell)With all that said, I just removed it entirely so that the comment is not misleading.
To solve this, I just used some intermediary variables, which is not as pretty as the oneliner, but oh well.
The end result of this mistake of pipefail is that commands that were not present were reported as present. Again, just for all the ones that use
sed. Frikkin sed, man...gulp
I don't think there was actually a bug with gulp, I just figure that if a program can use one of the
__dynamic_detectfunctions, it should, amiright?gor
Incorrectly parsed.
Expected:
✓ gor 1.3.0Actual:
✓ gor 2026sbt
Version was never parsed so always absent.
Expected:
✓ sbt 1.12.3Actual:
✓ sbtsbt was also falsely detected as present when it was not. It doesn't use sed so it's for some other reason, I think because it had false positives in the output of
sbt about. I switched it to usesbt --version; I'll put more about that in a comment.hub
hub uses sed so it was falsely reported as present when it was not.
zip
zip uses sed so it was falsely reported as present when it was not.
has
has never reported its version; it just has
--versionso it can just use__dynamic_detect--version, not a special case.R
R was removed 92b37d3, and I do not know if that was intentional or not. I say the more the merrier, so I added it back.
BONUS ITEM
I brought over the fix from #94 that I found earlier tonight! The solution here for it slightly more involved, which I will explain in a comment.
I would still recommend reviewing that #94, even if it's not merged, because explaining that in this PR would make it way tooooo long for a human to be expected to read.