Skip to content

Fix: gor, sbt, hub, zip, has, R, and pipefail#95

Open
goto-dev-null wants to merge 4 commits intokdabir:masterfrom
goto-dev-null:bugfix-gor-sbt-hub-zip-has-R-pipefail
Open

Fix: gor, sbt, hub, zip, has, R, and pipefail#95
goto-dev-null wants to merge 4 commits intokdabir:masterfrom
goto-dev-null:bugfix-gor-sbt-hub-zip-has-R-pipefail

Conversation

@goto-dev-null
Copy link
Copy Markdown
Contributor

@goto-dev-null goto-dev-null commented Feb 20, 2026

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 pipefail is 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 zip isn't present when running zip -v 2>&1 | sed -n 2p | grep -Eo "${REGEX_SIMPLE_VERSION}" | head -1:

  • zip: 127 ✗
  • sed: 0 ✓
  • grep: 1 ✗ ← This is the rightmost failure
  • head: 0 ✓

Hence $? will contain 1, not 127.

This only appeared in commands that used sed; all the ones that just use grep exited with 127, but just kinda by sheer luck (i.e. idfk), not because of set -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 $PIPESTATUS which 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, $PIPESTATUS is 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.

      cmd_output=$( zip -v 2>&1)
      status=$?
      version=$(echo "$cmd_output" | sed -n 2p | grep -Eo "${REGEX_SIMPLE_VERSION}" | head -1)

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_detect functions, it should, amiright?

gor

Incorrectly parsed.

Expected: ✓ gor 1.3.0
Actual: ✓ gor 2026

sbt

Version was never parsed so always absent.

Expected: ✓ sbt 1.12.3
Actual: ✓ sbt

sbt 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 use sbt --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 --version so 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.

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
Comment thread has

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)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-empty

Adding 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.

Comment thread has
Comment on lines +9 to +13
# 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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread has
Comment on lines +96 to +102
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
Copy link
Copy Markdown
Contributor Author

@goto-dev-null goto-dev-null Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant