-1

Consider the following function:

function testForBinary {
    someBin=$(command -v binary)
        # Test if binary is installed
        if [[ -n $someBin ]]; then
            installSuccess="Success"
            echo ${installSuccess}
            return
        else
            # This should never be reached
            return 1
        fi
}

taken from (Context):

function installAndTestForDialog() {
# dialog allows me to create a pretty installer
# It is a required dependency since XXXXX
# Name Removed as I'm attempting modularization
# of a script from Github that's one huge Script

# See https://unix.stackexchange.com/questions/85249/why-not-use-which-what-to-use-then
    local dialogBin
    local updated=false
    local dialogInstallSuccess

    dialogBin=$(command -v dialog)
    if [[ -z $dialogBin ]]; then
        printf "Installing dialog... "
        if [[ $updated -eq 0 ]]; then
            apt-get update > /dev/null 2>&1
            updated=true
        fi
        # Debian Based OS
        apt-get -y install dialog > /dev/null 2>&1

        # Test if dialog is installed to verify installation
        # above was successful.
        dialogBin=$(command -v dialog)
        if [[ -n $dialogBin ]]; then
            # I cannot get this to echo success
            # if I echo from here
            dialogInstallSuccess="Success"
            # Moving it here doesn't help either  Why??
            echo ${dialogInstallSuccess}
            return
        else
            # This should never be reached
            return 1
        fi
    fi    
}    

I'm attempting to treat installSuccess as a boolean, but what am I doing wrong. If I write the function as above, and then add:

isInstalled=$(testForBinary)
echo "$isInstalled"

isInstalled returns a blank line. I know this isn't true because when I run command -v binary outside the function, the path to binary results.

Output (Context):

Function and Variable Output Test
=====================
# These are other tests
# I'm performing, but note
# the blank line, which should
# print Success 
2.9-MODULAR
192.168.1.227
false
false
(blank line)
eyoung100
  • 5,717
  • 21
  • 50
  • 2
    Don't `exit` from a function unless you want the script that's running it to exit. Use `return` instead. see [Difference between return and exit in Bash functions](https://stackoverflow.com/questions/4419952/difference-between-return-and-exit-in-bash-functions) – cas Jul 07 '21 at 07:32
  • 1
    Run your code through https://www.shellcheck.net/ – laubster Jul 07 '21 at 21:13
  • Re: Extra fi, I'll correct that. I copied this from a JetBrainz based IDE. Re: Shellcheck. IntelliJ uses shellcheck on conjunction with [BASH Support Pro](https://plugins.jetbrains.com/plugin/13841-bashsupport-pro). Scripting is not my forte – eyoung100 Jul 07 '21 at 21:23
  • @StephenKitt `binary` is any binary executable. I replaced it to make it generic. I'm actually testing for `dialog` I can provide the output I get for context, along with the entire function – eyoung100 Jul 07 '21 at 21:25
  • 1
    Why should that be echoed? It's after a `return`, the execution has already moved out of the function. – muru Jul 07 '21 at 22:46
  • ... and `dialogInstallSuccess` is never set because `dialogBin` is still empty and you end up in the `else` block. – Freddy Jul 07 '21 at 22:53
  • @Freddy Unless I've missed something `dialogBin` is never empty. I do see now that I need to test it the second time, which I have now fixed. The output in question is still blank. The solution is probably staring me in the face, but I can't see it – eyoung100 Jul 07 '21 at 23:21
  • @ibuprofen checking path both inside and outside of JetBrainz yields: `PATH=/home/username/bin:/home/username/.local/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/opt/bin:/usr/lib/llvm/11/bin:/usr/lib/llvm/12/bin` – eyoung100 Jul 08 '21 at 03:26
  • I figured it out!! The If test for `-z $dialogBiin` can't be nested with the `-n $dialogBin` test. Removing the nesting creates the desired output – eyoung100 Jul 08 '21 at 04:44
  • 1
    You don't need to capture the output of `command -v` into a variable. `command` returns an exit code of 0 on success (i.e. `dialog` is in your path or defined as a function or alias), non-zero on failure. So all you need is `if command -v dialog; then ... ; else ... ; fi` – cas Jul 08 '21 at 04:51
  • It’s really late…but I don’t think you’re escaping $dialogBin correctly. It may be expanding and thus producing a false for non-zero length test. Use ‘if [[ -n “$dialogBin” ]]’ instead. – bretonics Jul 08 '21 at 04:54
  • 1
    Also, you're massively over-complicating this. Just run `apt update` and `apt install dialog` if dialog is not already installed, and be done with it. Or just `apt update` and `apt install dialog` without bothering to check if it's already installed - the worst that will happen is that a newer version of dialog (and its dependencies) might be installed. – cas Jul 08 '21 at 04:57
  • @cas I'm trying to keep the original author's code as intact as possible. I'm sure there are quicker ways to achieve the desired goal... – eyoung100 Jul 08 '21 at 05:04
  • why? if the original code is garbage (and it is), replace it. – cas Jul 08 '21 at 05:07

2 Answers2

5

This doesn't need to be anywhere near as complicated and fragile as you're making it.

installAndTestForDialog() {
  if command -v dialog &> /dev/null; then
    return 0
  else
    apt-get update &> /dev/null
    apt-get -y install dialog &> /dev/null
  fi
}

The function will return 0 if dialog is already installed. Otherwise it will attempt to install it and will return the exit code of the apt-get install command.


As @muru points out in a comment, this can be condensed to just:

if ! command -v dialog; then
    apt-get update
    apt-get install -y dialog
fi > /dev/null 2>&1

Or just try to install it without bothering to check if it's already installed:

{ apt-get update && apt-get -y --no-upgrade install dialog ;} >/dev/null 2>&1

The --no-upgrade option prevents dialog from being upgraded if it's already installed.

cas
  • 1
  • 7
  • 119
  • 185
  • If the shell running this isn't `bash` or some other shell that supports `&>`, you will need to use `>/dev/null 2>&1` instead of `&> /dev/null`. – cas Jul 08 '21 at 05:10
  • I'd probably just condense it to `if ! command -v dialog; then apt-get update; apt-get install -y dialog; fi > /dev/null 2>&1`. The return code of the `if` block as a whole would be 0 if `command -v dialog` didn't fail. – muru Jul 08 '21 at 05:20
  • @muru, yeah, but i wanted it to show handling both cases - even if the success case is basically a no-op. – cas Jul 08 '21 at 05:22
0
function installAndTestForDialog {
# dialog allows me to create a pretty installer
# It is a required dependency since Pinecraft 1.1

# See https://unix.stackexchange.com/questions/85249/why-not-use-which-what-to-use-then
    local dialogBin
    local updated=false
    local dialogInstallSuccess

    dialogBin=$(command -v dialog)
    if [[ -z "$dialogBin" ]]; then
        printf "Installing dialog... "
        if [[ $updated -eq 0 ]]; then
            apt-get update > /dev/null 2>&1
            updated=1
            apt-get -y install dialog > /dev/null 2>&1
        fi
    # This was the issue.  The install of dialog
    # cannot be included with the post install test
    # Moving it up, to "break the nesting" solves the issue 
    fi
        # Test if dialog is installed to verify installation
        # above was successful.
        dialogBin=$(command -v dialog)
        if [[ -n "$dialogBin" ]]; then
            dialogInstallSuccess="Success"
            echo "${dialogInstallSuccess}"
            return
        else
            # This should never be reached
            return 1
        fi
        # Moved the fi that was here up. See comment above
}

The above function produces the desired result. I hate when I write something that is syntactically correct but contains faulty logic

eyoung100
  • 5,717
  • 21
  • 50
  • FYI, in case you're wondering - it wasn't me who down-voted this. I rarely downvote anything (only answers that I think are *dangerously* wrong). Even though this is a bloated, over-complicated pile of steaming garbage, if it solves your problem and achieves your side-goal of minimising changes to the original script then who am i to judge? – cas Jul 08 '21 at 05:20